Re: [PATCH vdagent v3 3/3] refetch _NET_WM_NAME in vdagent_x11_has_icons_on_desktop()

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

 



Hi,

On Mon, Nov 13, 2017 at 06:50:07PM +0100, Jakub Janků wrote:
> Don't cache _NET_WM_NAME, instead refetch it with each
> vdagent_x11_has_icons_on_desktop() call so that correct result is returned
> even if the window manager changes during the lifetime of vdagent_x11 struct.
> Remove char *net_wm_name from vdagent_x11.

I'm not sure if mind too much with WM changing, I guess other things
could break in that situation... but not caching makes sense anyway.

> Remove x11 argument from vdagent_x11_has_icons_on_desktop().
> 
> Since the name is refetched, don't log it in vdagent_x11_create().
> ---
>  src/vdagent/vdagent.c  |  8 ++++----
>  src/vdagent/x11-priv.h |  1 -
>  src/vdagent/x11.c      | 20 ++++++--------------
>  src/vdagent/x11.h      |  2 +-
>  4 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 4710a44..d86ee25 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -109,7 +109,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(vdagent_x11_has_icons_on_desktop() ?
>                                    G_USER_DIRECTORY_DESKTOP :
>                                    G_USER_DIRECTORY_DOWNLOAD);
>  }
> @@ -123,6 +123,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 +138,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 ? !vdagent_x11_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);
>  }
>  
> 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 4ea86e9..9afce80 100644
> --- a/src/vdagent/x11.c
> +++ b/src/vdagent/x11.c
> @@ -229,18 +229,10 @@ 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;
> -    x11->net_wm_name = (gchar *)vdagent_x11_get_wm_name();
> -    while (!strcmp(x11->net_wm_name, "unknown") && --i > 0) {
> +    /* Since we are started at the same time as the wm,
> +       sometimes we need to wait a bit for the _NET_WM_NAME to show up. */
> +    for (int i = 0; i < 9 && !strcmp(vdagent_x11_get_wm_name(), "unknown"); i++)

- int i is already declared, just use i here;
- I would move the strcmp() check inside with a break, before the
  usleep()

>          usleep(100000);
> -        x11->net_wm_name = (gchar *)vdagent_x11_get_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));

I would keep the log adding __func__ name (meaning that at creation
time, this is WM being used) or we could move it to
vdagent_x11_get_wm_name() but it would be a bit too verbose.

>  
>      /* Flush output buffers and consume any pending events */
>      vdagent_x11_do_read(x11);
> @@ -263,7 +255,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);
>  }
> @@ -1292,7 +1283,7 @@ void vdagent_x11_client_disconnected(struct vdagent_x11 *x11)
>     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)
> +int vdagent_x11_has_icons_on_desktop()
>  {
>      const char * const wms_with_icons_on_desktop[] = {
>          "Metacity", /* GNOME-2 or GNOME-3 fallback */
> @@ -1300,10 +1291,11 @@ int vdagent_x11_has_icons_on_desktop(struct vdagent_x11 *x11)
>          "Marco",    /* Mate */
>          NULL
>      };
> +    const gchar *net_wm_name = vdagent_x11_get_wm_name();
>      int i;
>  
>      for (i = 0; wms_with_icons_on_desktop[i]; i++)
> -        if (!strcmp(x11->net_wm_name, wms_with_icons_on_desktop[i]))
> +        if (!strcmp(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..a8ceb08 100644
> --- a/src/vdagent/x11.h
> +++ b/src/vdagent/x11.h
> @@ -48,6 +48,6 @@ 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);
> +int vdagent_x11_has_icons_on_desktop();

Cheers,
        toso

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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]