[vlc-devel] [PATCH] ActiveX FullScreen support

[Jean-Baptiste Kempf]

On Wed, May 25, 2011 at 10:31:44PM +0700, Sergey Radionov wrote :
> Reformated version

Review:

> +class VLCHolderWnd
> +{
> +public:
> +    static void RegisterWndClassName(HINSTANCE hInstance);
> +    static void UnRegisterWndClassName();
> +    static VLCHolderWnd* CreateHolderWindow(VLCPlugin* p_instance);
> +    void DestroyWindow()
> +        {::DestroyWindow(_hWnd);};
Ok.

> +private:
> +    static LPCTSTR getClassName(void)  { return TEXT("VLC ActiveX Media Window Holder Class"); };
Text isn't good. "VLC ActiveX Window Holder Class" or something similar.
"Media" is out of context here.

> +private:
Why again a private: ?

> +private:
idem?

> +public:
idem?

> +HINSTANCE VLCHolderWnd::_hinstance=0;
> +ATOM VLCHolderWnd::_holder_wndclass_atom=0;
spaces around = , please.

> +void VLCHolderWnd::RegisterWndClassName(HINSTANCE hInstance)
Shouldn't implementations be in .cpp?

> +void VLCHolderWnd::UnRegisterWndClassName()
idem

> +VLCHolderWnd* VLCHolderWnd::CreateHolderWindow(VLCPlugin* p_instance)
idem.

> +LRESULT CALLBACK VLCHolderWnd::VLCHolderClassWndProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
> +{
> +    VLCHolderWnd* h_data = reinterpret_cast<VLCHolderWnd*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));
> +    VLCPlugin* p_instance = h_data ? h_data->_p_instance : 0;

Please use NULL for pointers;

> +class VLCFullScreenWnd
Exactly the same remarks as above :D

> +#define ID_FS_SWITCH_FS 1
> +#define ID_FS_PLAY_PAUSE 2
> +#define ID_FS_VIDEO_POS_SCROLL 3
> +#define ID_FS_MUTE 4
> +#define ID_FS_VOLUME 5
> +#define ID_FS_LABEL 5
No. Use enums.

> diff --git a/projects/activex/axvlc.idl b/projects/activex/axvlc.idl
> index de7562d..21a3495 100644
> --- a/projects/activex/axvlc.idl
> +++ b/projects/activex/axvlc.idl
> @@ -644,6 +644,11 @@ library AXVLC
>  
>          [propget, helpstring("Returns the audio object.")]
>          HRESULT video([out, retval] IVLCVideo** obj);
> +
> +        [propget]
> +        HRESULT NewMessageFlag([out, retval] VARIANT_BOOL* vYes);
> +        [propput]
> +        HRESULT NewMessageFlag([in] VARIANT_BOOL vYes);
>      };
>  
>      [

I still think this part should be in a separate commit, but well...

> diff --git a/projects/activex/axvlc_rc.rc.in b/projects/activex/axvlc_rc.rc.in
> index f3cf3a8..274aa47 100644
> --- a/projects/activex/axvlc_rc.rc.in
> +++ b/projects/activex/axvlc_rc.rc.in
> @@ -25,6 +25,12 @@ BEGIN
>  END
>  
>  2 BITMAP DISCARDABLE "inplace.bmp"
> +3 BITMAP DISCARDABLE "defullscreen.bmp"
> +4 BITMAP DISCARDABLE "play.bmp"
> +5 BITMAP DISCARDABLE "pause.bmp"
> +6 BITMAP DISCARDABLE "volume.bmp"
> +7 BITMAP DISCARDABLE "volume-muted.bmp"
> +8 BITMAP DISCARDABLE "new_message.bmp"
>  
>  1 TYPELIB "axvlc.tlb"

Yep.

> +        case WM_SIZE:{
> +            int new_client_width = LOWORD(lParam);
> +            int new_client_height = HIWORD(lParam);
> +            //first child will be resized to client area
> +            HWND hChildWnd = GetWindow(hWnd, GW_CHILD);
> +            if(hChildWnd){
> +                MoveWindow(hChildWnd, 0, 0, new_client_width, new_client_height, FALSE);
> +            }
> +            return 0L;
> +        }
> +        case WM_LBUTTONDBLCLK:{
> +            p_instance->toggleFullscreen();
> +            return 0L;
> +        }
Ok, I thnk.

I must say that is addition is a really good thing and quite difficult
to merge, I know.
Thanks many times.

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device