[MPlayer-dev-eng] [PATCH] 2-Win Fullscreen new method + fixes for virtual desktop + application changing

[Attila Kinali]

On Thu, Jul 29, 2004 at 12:38:04AM +0200, Fabian Franz wrote:
> Ok, here is -pre7.
> 
> It should work with virtual desktops, Application switching and as soon as 
> Video-Window looses the focus, the fullscreen-mode is quit until it gets the 
> focus again ...

I had a look at the code, it looks promising, but it has imho
a few places for improvment:


> Index: libvo/vo_x11.c
> ===================================================================
> RCS file: /cvsroot/mplayer/main/libvo/vo_x11.c,v
> retrieving revision 1.139
> diff -u -r1.139 vo_x11.c
> --- libvo/vo_x11.c	25 Jul 2004 12:49:01 -0000	1.139
> +++ libvo/vo_x11.c	28 Jul 2004 22:36:17 -0000
> @@ -415,6 +415,16 @@
>                  while (xev.type != MapNotify
>                         || xev.xmap.event != vo_window);
>  
> +                // we cannot grab mouse events on root window :(
> +                vo_x11_selectinput_witherr(mDisplay, vo_window,
> +                                           StructureNotifyMask | KeyPressMask |
> +                                           PropertyChangeMask | ExposureMask |
> +                                           ((WinID ==
> +                                             0) ? 0 : (ButtonPressMask |
> +                                                       ButtonReleaseMask |
> +                                                       PointerMotionMask)));
> +
> +
>                  if (fullscreen)
>                      vo_x11_fullscreen();
>  #ifdef HAVE_XINERAMA
> @@ -428,15 +438,6 @@
>          XFlush(mDisplay);
>          XSync(mDisplay, False);
>  
> -        // we cannot grab mouse events on root window :(
> -        vo_x11_selectinput_witherr(mDisplay, vo_window,
> -                                   StructureNotifyMask | KeyPressMask |
> -                                   PropertyChangeMask | ExposureMask |
> -                                   ((WinID ==
> -                                     0) ? 0 : (ButtonPressMask |
> -                                               ButtonReleaseMask |
> -                                               PointerMotionMask)));
> -


I dont see exactly why this move of vo_x11_selectinput_witherr()
is needed, but if it's necessary, did you check all other x11 vo's
whether they need it too ?

> Index: libvo/x11_common.c
> ===================================================================
> RCS file: /cvsroot/mplayer/main/libvo/x11_common.c,v
> retrieving revision 1.175
> diff -u -r1.175 x11_common.c
> --- libvo/x11_common.c	11 Jul 2004 22:27:17 -0000	1.175
> +++ libvo/x11_common.c	28 Jul 2004 22:36:17 -0000
  #if 0
> @@ -1045,6 +1062,8 @@
>                                            &vo_dx, &vo_dy, &win);
>                  }
>  #endif
> +                printf("[xevents] Resizing %s to x: %d y: %d width: %d height: %d\n",(Event.xany.window!=vo_fs_window)?"Video window":"Fullscreen window",vo_dx,vo_dy,vo_dwidth,vo_dheight);

all printfs should be changed to mp_msgs, a
:s%/printf(/mp_msg(MSGT_VO, MSGL_V, /g
should do the job.

> @@ -1126,6 +1145,109 @@
>                  vo_hint.win_gravity = old_gravity;
>                  XSetWMNormalHints(mDisplay, vo_window, &vo_hint);
>                  vo_fs_flip = 0;
> +                printf("[xevents] %s got mapped.\n", (Event.xany.window!=vo_fs_window)?"Video window":"Fullscreen window");
> +
> +                if (vo_fs && vo_fs_window_unmapped && (Event.xany.window==vo_norm_window))
> +                {
> +{
> +  int x,y,w,h;
> +        x = 0;
> +        y = 0;

this should be rather:
#ifdef HAVE_XINERAMA
x = xinerama_x
y = xinerama_y
#else
x = 0
y = 0
#endif


> +        w = vo_screenwidth;
> +        h = vo_screenheight;

These two are already set by the xinerama code to the right values

> +        
> +        XMoveResizeWindow(mDisplay, vo_fs_window, x,y,w,h);


> +}
> +                  XMapRaised(mDisplay, vo_fs_window);
> +                  XRaiseWindow(mDisplay, vo_fs_window);

Is there a reason why you raise an already raised window ?

> +            case UnmapNotify:
> +                printf("[xevents] %s got unmapped.\n", (Event.xany.window!=vo_fs_window)?"Video window":"Fullscreen window");
> +                if (vo_fs && (Event.xany.window==vo_norm_window))
> +                {
> +                  XUnmapWindow(mDisplay, vo_fs_window);
> +                  vo_window=vo_norm_window;
> +{
> +  int x,y,w,h;
> +        x = vo_old_x;
> +        y = vo_old_y;
> +        w = vo_old_width;
> +        h = vo_old_height;
> +
> +        XMoveResizeWindow(mDisplay, vo_fs_window, x,y,w,h);
> +}

Isnt it enough to just unmap the window and leave it where it is ?

> +
> +                  vo_fs_window_unmapped=1;
> +                }
> +                break;
> +            case FocusIn:
> +                if (vo_fs && vo_fs_window_unmapped && (Event.xfocus.window==vo_norm_window))
> +                {
> +
> +{
> +  int x,y,w,h;
> +        x = 0;
> +        y = 0;
> +        w = vo_screenwidth;
> +        h = vo_screenheight;

Here again the #ifdev HAVE_XINERAMA code from above

The whole xinerama code works pretty simple. The main functionality
is done at line 397, where it sets the window position to a place on
the selected screen. Later, when the window is mapped,
vo_x11_xinerama_move() moves the window to the desired place.
(note that the function should be changed to move the window into the middle
of the screen instead of the upper left corner)
Xinerama is from a programmers point of view
just a call to XineramaIsActive and XineramaQueryScreens. Where
as the later returns an array with the positions and dimensions
of the available screens. And dont fear that you dont have any
Xinerama to test it, i wrote the whole code while not having
Xinerama myself :)

				Attila Kinali