<br><br><div class="gmail_quote">2011/6/1 Jean-Baptiste Kempf <span dir="ltr"><<a href="mailto:jb@videolan.org">jb@videolan.org</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
On Wed, May 25, 2011 at 10:31:44PM +0700, Sergey Radionov wrote :<br>
> Reformated version<br>
<br>
Review:<br>
<br>
> +class VLCHolderWnd<br>
> +{<br>
> +public:<br>
> + š šstatic void RegisterWndClassName(HINSTANCE hInstance);<br>
> + š šstatic void UnRegisterWndClassName();<br>
> + š šstatic VLCHolderWnd* CreateHolderWindow(VLCPlugin* p_instance);<br>
> + š švoid DestroyWindow()<br>
> + š š š š{::DestroyWindow(_hWnd);};<br>
Ok.<br>
<br>
> +private:<br>
> + š šstatic LPCTSTR getClassName(void) š{ return TEXT("VLC ActiveX Media Window Holder Class"); };<br>
Text isn't good. "VLC ActiveX Window Holder Class" or something similar.<br>
"Media" is out of context here.<br></blockquote><div>Ok, <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">accepted</span></span>. <br><br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
> +private:<br>
Why again a private: ?<br>
<br>
> +private:<br>
idem?<br>
<br>
> +public:<br>
idem?<br></blockquote><div>I am <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">usually split class members into </span></span>logical groups, and private/protected/public <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">serves as</span></span><span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps"></span></span> separators... I think it helps <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">understanding code...</span></span><br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
> +HINSTANCE VLCHolderWnd::_hinstance=0;<br>
> +ATOM VLCHolderWnd::_holder_wndclass_atom=0;<br>
spaces around = , please.<br></blockquote><div>Ok, <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">accepted</span></span>. <br>š
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
> +void VLCHolderWnd::RegisterWndClassName(HINSTANCE hInstance)<br>
Shouldn't implementations be in .cpp?<br>
<br>
> +void VLCHolderWnd::UnRegisterWndClassName()<br>
idem<br>
<br>
> +VLCHolderWnd* VLCHolderWnd::CreateHolderWindow(VLCPlugin* p_instance)<br>
idem.<br></blockquote><div>Ok, <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">accepted</span></span>.š <br><br></div><div></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">

> +LRESULT CALLBACK VLCHolderWnd::VLCHolderClassWndProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)<br>
> +{<br>
> + š šVLCHolderWnd* h_data = reinterpret_cast<VLCHolderWnd*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));<br>
> + š šVLCPlugin* p_instance = h_data ? h_data->_p_instance : 0;<br>
<br>
Please use NULL for pointers;<br></blockquote><div>šBjarne Stroustrup: <a href="http://www2.research.att.com/~bs/bs_faq2.html#null">http://www2.research.att.com/~bs/bs_faq2.html#null</a><br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">

<br>
> +class VLCFullScreenWnd<br>
Exactly the same remarks as above :D<br>
<br>
> +#define ID_FS_SWITCH_FS 1<br>
> +#define ID_FS_PLAY_PAUSE 2<br>
> +#define ID_FS_VIDEO_POS_SCROLL 3<br>
> +#define ID_FS_MUTE 4<br>
> +#define ID_FS_VOLUME 5<br>
> +#define ID_FS_LABEL 5<br>
No. Use enums.<br></blockquote><div>Ok, <span id="result_box" class="short_text" lang="en"><span title="îÁÖÍÉÔÅ, ÞÔÏÂÙ Õ×ÉÄÅÔØ ÁÌØÔÅÒÎÁÔÉ×ÎÙÊ ÐÅÒÅ×ÏÄ" class="hps">accepted</span></span>.š <br>š</div><div></div><div></div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
> diff --git a/projects/activex/axvlc.idl b/projects/activex/axvlc.idl<br>
> index de7562d..21a3495 100644<br>
> --- a/projects/activex/axvlc.idl<br>
> +++ b/projects/activex/axvlc.idl<br>
> @@ -644,6 +644,11 @@ library AXVLC<br>
><br>
> š š š š š[propget, helpstring("Returns the audio object.")]<br>
> š š š š šHRESULT video([out, retval] IVLCVideo** obj);<br>
> +<br>
> + š š š š[propget]<br>
> + š š š šHRESULT NewMessageFlag([out, retval] VARIANT_BOOL* vYes);<br>
> + š š š š[propput]<br>
> + š š š šHRESULT NewMessageFlag([in] VARIANT_BOOL vYes);<br>
> š š š};<br>
><br>
> š š š[<br>
<br>
I still think this part should be in a separate commit, but well...<br>
<br></blockquote><div>Ok, I'll try split it to different commits (if I can)...<br>š<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">

> diff --git a/projects/activex/<a href="http://axvlc_rc.rc.in" target="_blank">axvlc_rc.rc.in</a> b/projects/activex/<a href="http://axvlc_rc.rc.in" target="_blank">axvlc_rc.rc.in</a><br>
> index f3cf3a8..274aa47 100644<br>
> --- a/projects/activex/<a href="http://axvlc_rc.rc.in" target="_blank">axvlc_rc.rc.in</a><br>
> +++ b/projects/activex/<a href="http://axvlc_rc.rc.in" target="_blank">axvlc_rc.rc.in</a><br>
> @@ -25,6 +25,12 @@ BEGIN<br>
> šEND<br>
><br>
> š2 BITMAP DISCARDABLE "inplace.bmp"<br>
> +3 BITMAP DISCARDABLE "defullscreen.bmp"<br>
> +4 BITMAP DISCARDABLE "play.bmp"<br>
> +5 BITMAP DISCARDABLE "pause.bmp"<br>
> +6 BITMAP DISCARDABLE "volume.bmp"<br>
> +7 BITMAP DISCARDABLE "volume-muted.bmp"<br>
> +8 BITMAP DISCARDABLE "new_message.bmp"<br>
><br>
> š1 TYPELIB "axvlc.tlb"<br>
<br>
Yep.<br>
<br>
> + š š š šcase WM_SIZE:{<br>
> + š š š š š šint new_client_width = LOWORD(lParam);<br>
> + š š š š š šint new_client_height = HIWORD(lParam);<br>
> + š š š š š š//first child will be resized to client area<br>
> + š š š š š šHWND hChildWnd = GetWindow(hWnd, GW_CHILD);<br>
> + š š š š š šif(hChildWnd){<br>
> + š š š š š š š šMoveWindow(hChildWnd, 0, 0, new_client_width, new_client_height, FALSE);<br>
> + š š š š š š}<br>
> + š š š š š šreturn 0L;<br>
> + š š š š}<br>
> + š š š šcase WM_LBUTTONDBLCLK:{<br>
> + š š š š š šp_instance->toggleFullscreen();<br>
> + š š š š š šreturn 0L;<br>
> + š š š š}<br>
Ok, I thnk.<br>
<br>
I must say that is addition is a really good thing and quite difficult<br>
to merge, I know.<br>
Thanks many times.<br>
<br>
Best Regards,<br>
<font color="#888888"><br>
--<br>
Jean-Baptiste Kempf<br>
<a href="http://www.jbkempf.com/" target="_blank">http://www.jbkempf.com/</a> - +33 672 704 734<br>
Sent from my Electronic Device<br>
_______________________________________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="http://mailman.videolan.org/listinfo/vlc-devel" target="_blank">http://mailman.videolan.org/listinfo/vlc-devel</a><br>
</font></blockquote></div><br>