[Spice-devel] [PATCH vdagent 2/2] vdagent-x11: replace WM_NAME related code with GDK

Frediano Ziglio fziglio at redhat.com
Mon Oct 30 09:19:08 UTC 2017


> 
> remove from x11:
> - vdagent_x11_get_wm_name()
> - vdagent_x11_has_icons_on_desktop()
> - char *new_wm_name
> 
> vdagent_x11_has_icons_on_desktop() handling has been
> moved from x11.c to vdagent.c and now uses GDK.
> ---
>  src/vdagent/vdagent.c  | 46 +++++++++++++++++++++--
>  src/vdagent/x11-priv.h |  1 -
>  src/vdagent/x11.c      | 99
>  --------------------------------------------------
>  src/vdagent/x11.h      |  2 -
>  4 files changed, 42 insertions(+), 106 deletions(-)
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 4710a44..a19a77e 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -37,6 +37,10 @@
>  #include <poll.h>
>  #include <glib-unix.h>
>  #include <gtk/gtk.h>
> +#include <gdk/gdk.h>
> +#ifdef GDK_WINDOWING_X11
> +#include <gdk/gdkx.h>
> +#endif
>  
>  #include "udscs.h"
>  #include "vdagentd-proto.h"
> @@ -51,6 +55,8 @@ typedef struct VDAgent {
>      struct udscs_connection *conn;
>      GIOChannel *x11_channel;
>  
> +    gboolean has_icons_on_desktop;
> +
>      GMainLoop *loop;
>  } VDAgent;
>  
> @@ -92,6 +98,36 @@ static GOptionEntry entries[] = {
>      { NULL }
>  };
>  
> +static gboolean vdagent_has_icons_on_desktop()
> +{
> +#ifdef GDK_WINDOWING_X11
> +    const char * const wms_with_icons[] = {
> +        "Metacity", /* GNOME-2 or GNOME-3 fallback */
> +        "Xfwm4",    /* Xfce */
> +        "Marco",    /* Mate */
> +    };
> +
> +    GdkDisplay *display = gdk_display_get_default();
> +    if (!GDK_IS_X11_DISPLAY(display))
> +        return FALSE;
> +
> +    for (int i = 0; i < 10; i++) {
> +        const char *wm_name = gdk_x11_screen_get_window_manager_name(
> +            gdk_display_get_default_screen(display));
> +
> +        if (strcmp(wm_name, "unknown")) {
> +            for (i = 0; i < G_N_ELEMENTS(wms_with_icons); i++)
> +                if (!strcmp(wm_name, wms_with_icons[i]))
> +                    return TRUE;
> +            return FALSE;
> +        }
> +
> +        usleep(100000);
> +    }
> +#endif
> +    return FALSE;
> +}
> +
>  /**
>   * xfer_get_download_directory
>   *
> @@ -109,7 +145,7 @@ static const gchar *xfer_get_download_directory(VDAgent
> *agent)
>          return fx_dir;
>      }
>  
> -    return
> g_get_user_special_dir(vdagent_x11_has_icons_on_desktop(agent->x11) ?
> +    return g_get_user_special_dir(agent->has_icons_on_desktop ?
>                                    G_USER_DIRECTORY_DESKTOP :
>                                    G_USER_DIRECTORY_DOWNLOAD);
>  }
> @@ -123,6 +159,7 @@ static const gchar *xfer_get_download_directory(VDAgent
> *agent)
>  static gboolean vdagent_init_file_xfer(VDAgent *agent)
>  {
>      const gchar *xfer_dir;
> +    gboolean open_dir;
>  
>      if (agent->xfers != NULL) {
>          syslog(LOG_DEBUG, "File-xfer already initialized");
> @@ -137,11 +174,10 @@ static gboolean vdagent_init_file_xfer(VDAgent *agent)
>          return FALSE;
>      }
>  
> -    if (fx_open_dir == -1)
> -        fx_open_dir = !vdagent_x11_has_icons_on_desktop(agent->x11);
> +    open_dir = fx_open_dir == -1 ? !agent->has_icons_on_desktop :
> fx_open_dir;
>  
>      agent->xfers = vdagent_file_xfers_create(agent->conn, xfer_dir,
> -                                             fx_open_dir, debug);
> +                                             open_dir, debug);
>      return (agent->xfers != NULL);
>  }
>  
> @@ -372,6 +408,8 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
>                     x11_io_channel_cb,
>                     agent);
>  
> +    agent->has_icons_on_desktop = vdagent_has_icons_on_desktop();
> +
>      if (!vdagent_init_file_xfer(agent))
>          syslog(LOG_WARNING, "File transfer is disabled");
>  
> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
> index 677a44d..3776098 100644
> --- a/src/vdagent/x11-priv.h
> +++ b/src/vdagent/x11-priv.h
> @@ -87,7 +87,6 @@ struct vdagent_x11 {
>      Window root_window[MAX_SCREENS];
>      Window selection_window;
>      struct udscs_connection *vdagentd;
> -    char *net_wm_name;
>      int debug;
>      int fd;
>      int screen_count;
> diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> index 6e47ea1..7387a00 100644
> --- a/src/vdagent/x11.c
> +++ b/src/vdagent/x11.c
> @@ -37,7 +37,6 @@
>  #include <string.h>
>  #include <syslog.h>
>  #include <assert.h>
> -#include <unistd.h>
>  #include <X11/Xatom.h>
>  #include <X11/Xlib.h>
>  #include <X11/extensions/Xfixes.h>
> @@ -111,68 +110,6 @@ int vdagent_x11_restore_error_handler(struct vdagent_x11
> *x11)
>      return error;
>  }
>  
> -static void vdagent_x11_get_wm_name(struct vdagent_x11 *x11)
> -{
> -    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);
> -}
> -
>  struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
>      int debug, int sync)
>  {
> @@ -275,18 +212,6 @@ struct vdagent_x11 *vdagent_x11_create(struct
> udscs_connection *vdagentd,
>      }
>      vdagent_x11_send_daemon_guest_xorg_res(x11, 1);
>  
> -    /* 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) {
> -        usleep(100000);
> -        vdagent_x11_get_wm_name(x11);
> -    }
> -    if (x11->debug && x11->net_wm_name)
> -        syslog(LOG_DEBUG, "net_wm_name: \"%s\", has icons: %d",
> -               x11->net_wm_name, vdagent_x11_has_icons_on_desktop(x11));
> -
>      /* Flush output buffers and consume any pending events */
>      vdagent_x11_do_read(x11);
>  
> @@ -308,7 +233,6 @@ void vdagent_x11_destroy(struct vdagent_x11 *x11, int
> vdagentd_disconnected)
>      }
>  
>      XCloseDisplay(x11->display);
> -    g_free(x11->net_wm_name);
>      free(x11->randr.failed_conf);
>      free(x11);
>  }
> @@ -1331,26 +1255,3 @@ void vdagent_x11_client_disconnected(struct
> vdagent_x11 *x11)
>              vdagent_x11_clipboard_release(x11, sel);
>      }
>  }
> -
> -/* Function used to determine the default location to save file-xfers,
> -   xdg desktop dir or xdg download dir. We err on the safe side and use a
> -   whitelist approach, so any unknown desktop will end up with saving
> -   file-xfers to the xdg download dir, and opening the xdg download dir with
> -   xdg-open when the file-xfer completes. */
> -int vdagent_x11_has_icons_on_desktop(struct vdagent_x11 *x11)
> -{
> -    const char * const wms_with_icons_on_desktop[] = {
> -        "Metacity", /* GNOME-2 or GNOME-3 fallback */
> -        "Xfwm4",    /* Xfce */
> -        "Marco",    /* Mate */
> -        NULL
> -    };
> -    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;
> -
> -    return 0;
> -}
> diff --git a/src/vdagent/x11.h b/src/vdagent/x11.h
> index 4fd0380..2060655 100644
> --- a/src/vdagent/x11.h
> +++ b/src/vdagent/x11.h
> @@ -48,6 +48,4 @@ void vdagent_x11_clipboard_release(struct vdagent_x11 *x11,
> uint8_t selection);
>  
>  void vdagent_x11_client_disconnected(struct vdagent_x11 *x11);
>  
> -int vdagent_x11_has_icons_on_desktop(struct vdagent_x11 *x11);
> -
>  #endif

Looks like this patch is doing too much thing:
- move code from x11.c to vdagent.c, why?
- change cache from name to boolean, seems fine;
- use GDK instead of direct X11 calls.

I personally like code dealing with X11 being in x11.c instead of vdagent.c.

Frediano


More information about the Spice-devel mailing list