Re: [PATCH vdagent v3 2/3] retrieve _NET_WM_NAME using GDK

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2017-12-01 at 17:11 +0100, Victor Toso wrote:
> Hi,
> 
> On Fri, Dec 01, 2017 at 04:29:32PM +0100, Jakub Janků wrote:
> > Hi,
> > 
> > On Fri, 2017-12-01 at 15:11 +0100, Victor Toso wrote:
> > > Hi,
> > > 
> > > On Mon, Nov 13, 2017 at 06:50:06PM +0100, Jakub Janků wrote:
> > > > Get _NET_WM_NAME using
> > > > gdk_x11_screen_get_window_manager_name().
> > > > vdagent_x11_get_wm_name(): return the name instead of setting
> > > > it.
> > > > Return string specifics:
> > > > - "unsupported", when not running on X11
> > > > - "unknown", when the name cannot be retrieved
> > > 
> > > I'm actually getting:
> > > * "GNOME Shell", either on Wayland/X11 GNOME
> > > * "unknown", for XFCE
> > > * "Metacity (Marco)", for MATE
> > > 
> > > Based on values in the array wms_with_icons_on_desktop, we might
> > > have
> > > some regressions or we need to update the array too.
> > 
> > The value for GNOME is alright. I've tested XFCE and I'm getting
> > correct value "Xfwm4". (Haven't tested Mate yet.)
> > 
> > Gtk should use practically the same approach to get the
> > _NET_WM_NAME as
> > we did, so it's weird that it doesn't work. Could you please try
> > the
> > current vdagent to see what name does it return?	
> 
> Tried again, both:
> 
> Current (master) vdagent:
> * XFCE: Xfwm4
> * MATE: Metacity (Marco)
> * GNOME x11: GNOME Shell
> * GNOME wayland: GNOME Shell
> 
> With yours:
> * XFCE: Xfwm4
> * MATE: Metacity (Marco)
> * GNOME x11: GNOME Shell
> * GNOME wayland: GNOME Shell
> 
> So, not sure what I did wrong in the previous test. This time I
> rebooted
> the VM entirely, I'm not sure that I did that the first time.
> 
> As long as we keep some log (next patch) I think this should be fine
> 

Great! However, since Mate returns "Metacity (Marco)", the
vdagent_x11_has_icons_on_desktop() will return FALSE. I don't know
whether this used to be just "Marco", but maybe it would be reasonable
to check if _NET_WM_NAME contains one of the names from
wms_with_icons_on_desktop array (instead of looking for exact match)?
> > > 
> > > > Since the return value is never NULL, remove obsolete checks.
> > > 
> > > Yep, seems better this way.
> > > 
> > > > ---
> > > >  src/vdagent/x11.c | 88 +++++++++++++------------------------
> > > > ----
> > > > --------------
> > > >  1 file changed, 21 insertions(+), 67 deletions(-)
> > > > 
> > > > diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> > > > index 6e47ea1..4ea86e9 100644
> > > > --- a/src/vdagent/x11.c
> > > > +++ b/src/vdagent/x11.c
> > > > @@ -32,6 +32,10 @@
> > > >     pending writes are flushed. */
> > > >  
> > > >  #include <glib.h>
> > > > +#include <gdk/gdk.h>
> > > > +#ifdef GDK_WINDOWING_X11
> > > > +#include <gdk/gdkx.h>
> > > > +#endif
> > > >  #include <stdlib.h>
> > > >  #include <limits.h>
> > > >  #include <string.h>
> > > > @@ -111,66 +115,16 @@ int
> > > > vdagent_x11_restore_error_handler(struct
> > > > vdagent_x11 *x11)
> > > >      return error;
> > > >  }
> > > >  
> > > > -static void vdagent_x11_get_wm_name(struct vdagent_x11 *x11)
> > > > +static const gchar *vdagent_x11_get_wm_name()
> > > >  {
> > > > -    Atom type_ret;
> > > > -    int format_ret;
> > > > -    unsigned long len, remain;
> > > > -    unsigned char *data = NULL;
> > > > -    Window sup_window = None;
> > > > -
> > > > -    /* XGetWindowProperty can throw a BadWindow error. One way
> > > > we
> > > > can trigger
> > > > -       this is when the display-manager (ie gdm) has set, and
> > > > not
> > > > cleared the
> > > > -       _NET_SUPPORTING_WM_CHECK property, and the window
> > > > manager
> > > > running in
> > > > -       the user session has not yet updated it to point to its
> > > > window, so its
> > > > -       pointing to a nonexistent window. */
> > > > -    vdagent_x11_set_error_handler(x11,
> > > > vdagent_x11_ignore_bad_window_handler);
> > > > -
> > > > -    /* Get the window manager SUPPORTING_WM_CHECK window */
> > > > -    if (XGetWindowProperty(x11->display, x11->root_window[0],
> > > > -            XInternAtom(x11->display,
> > > > "_NET_SUPPORTING_WM_CHECK",
> > > > False), 0,
> > > > -            LONG_MAX, False, XA_WINDOW, &type_ret,
> > > > &format_ret,
> > > > &len,
> > > > -            &remain, &data) == Success) {
> > > > -        if (type_ret == XA_WINDOW)
> > > > -            sup_window = *((Window *)data);
> > > > -        XFree(data);
> > > > -    }
> > > > -    if (sup_window == None &&
> > > > -        XGetWindowProperty(x11->display, x11->root_window[0],
> > > > -            XInternAtom(x11->display,
> > > > "_WIN_SUPPORTING_WM_CHECK",
> > > > False), 0,
> > > > -            LONG_MAX, False, XA_CARDINAL, &type_ret,
> > > > &format_ret,
> > > > &len,
> > > > -            &remain, &data) == Success) {
> > > > -        if (type_ret == XA_CARDINAL)
> > > > -            sup_window = *((Window *)data);
> > > > -        XFree(data);
> > > > -    }
> > > > -    /* So that we can get the net_wm_name */
> > > > -    if (sup_window != None) {
> > > > -        Atom utf8 = XInternAtom(x11->display, "UTF8_STRING",
> > > > False);
> > > > -        if (XGetWindowProperty(x11->display, sup_window,
> > > > -                XInternAtom(x11->display, "_NET_WM_NAME",
> > > > False),
> > > > 0,
> > > > -                LONG_MAX, False, utf8, &type_ret, &format_ret,
> > > > &len,
> > > > -                &remain, &data) == Success) {
> > > > -            if (type_ret == utf8) {
> > > > -                x11->net_wm_name =
> > > > -                    g_strndup((char *)data, (format_ret / 8) *
> > > > len);
> > > > -            }
> > > > -            XFree(data);
> > > > -        }
> > > > -        if (x11->net_wm_name == NULL &&
> > > > -            XGetWindowProperty(x11->display, sup_window,
> > > > -                XInternAtom(x11->display, "_NET_WM_NAME",
> > > > False),
> > > > 0,
> > > > -                LONG_MAX, False, XA_STRING, &type_ret,
> > > > &format_ret, &len,
> > > > -                &remain, &data) == Success) {
> > > > -            if (type_ret == XA_STRING) {
> > > > -                x11->net_wm_name =
> > > > -                    g_strndup((char *)data, (format_ret / 8) *
> > > > len);
> > > > -            }
> > > > -            XFree(data);
> > > > -        }
> > > > -    }
> > > > -
> > > > -    vdagent_x11_restore_error_handler(x11);
> > > > +#ifdef GDK_WINDOWING_X11
> > > > +    GdkDisplay *display = gdk_display_get_default();
> > > > +    if (GDK_IS_X11_DISPLAY(display))
> > > > +        return gdk_x11_screen_get_window_manager_name(
> > > > +            gdk_display_get_default_screen(display));
> > > > +    else
> > > 
> > > No need for else
> > > 
> > > > +#endif
> > > > +    return "unsupported";
> > > >  }
> > > >  
> > > >  struct vdagent_x11 *vdagent_x11_create(struct udscs_connection
> > > > *vdagentd,
> > > > @@ -278,12 +232,13 @@ struct vdagent_x11
> > > > *vdagent_x11_create(struct
> > > > udscs_connection *vdagentd,
> > > >      /* Get net_wm_name, since we are started at the same time
> > > > as
> > > > the wm,
> > > >         sometimes we need to wait a bit for it to show up. */
> > > >      i = 10;
> > > > -    vdagent_x11_get_wm_name(x11);
> > > > -    while (x11->net_wm_name == NULL && --i > 0) {
> > > > +    x11->net_wm_name = (gchar *)vdagent_x11_get_wm_name();
> > > > +    while (!strcmp(x11->net_wm_name, "unknown") && --i > 0) {
> > > >          usleep(100000);
> > > > -        vdagent_x11_get_wm_name(x11);
> > > > +        x11->net_wm_name = (gchar *)vdagent_x11_get_wm_name();
> > > >      }
> > > > -    if (x11->debug && x11->net_wm_name)
> > > > +    x11->net_wm_name = g_strdup(x11->net_wm_name);
> > > > +    if (x11->debug)
> > > >          syslog(LOG_DEBUG, "net_wm_name: \"%s\", has icons:
> > > > %d",
> > > >                 x11->net_wm_name,
> > > > vdagent_x11_has_icons_on_desktop(x11));
> > > >  
> > > > @@ -1347,10 +1302,9 @@ int
> > > > vdagent_x11_has_icons_on_desktop(struct
> > > > vdagent_x11 *x11)
> > > >      };
> > > >      int i;
> > > >  
> > > > -    if (x11->net_wm_name)
> > > > -        for (i = 0; wms_with_icons_on_desktop[i]; i++)
> > > > -            if (!strcmp(x11->net_wm_name,
> > > > wms_with_icons_on_desktop[i]))
> > > > -                return 1;
> > > > +    for (i = 0; wms_with_icons_on_desktop[i]; i++)
> > > > +        if (!strcmp(x11->net_wm_name,
> > > > wms_with_icons_on_desktop[i]))
> > > > +            return 1;
> > > 
> > > Sorry for taking time to review this patch ;)
> > > 
> > > Cheers,
> > >         toso
> > 
> > Thanks,
> > Jakub
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]