[PATCH weston v2] xwm: use always a valid 'primary view' for an X window

[Bryce Harrington]

On Tue, Sep 29, 2015 at 04:51:58PM -0500, Derek Foreman wrote:
> On 23/09/15 12:15 PM, Giulio Camuffo wrote:
> > 2015-09-23 19:46 GMT+03:00 Derek Foreman <derekf at osg.samsung.com>:
> >> On 01/02/15 08:18 AM, Giulio Camuffo wrote:
> >>> The xwm gets a primary view for a X window using the get_primary_view
> >>> vfunc of the shell_interface struct. Storing it is dangerous though because
> >>> it doesn't listen for its destruction so it may end up using the old stored
> >>> view pointer after that view was freed, or after the primary view for that
> >>> window was changed to another one.
> >>> Fetch the primary view just before using it every time and try to not
> >>> abuse this 'primary view' concept which may map badly to some shells:
> >>> iterate over all the views instead when it makes sense.
> >>> ---
> >>
> >> Good commit log, makes it obvious that we should be doing this...
> >>
> >>> v2: check window->shsurf
> >>>
> >>> -                     if (window->view)
> >>> -                             weston_view_geometry_dirty(window->view);
> >>> +                     wl_list_for_each(view, &window->surface->views, surface_link)
> >>> +                             weston_view_geometry_dirty(view);
> >>
> >> Same as above...
> >>
> >> It seems like these two hunks are actually a separate bug fix?  Or is
> >> this because some shells might not have a get_primary_view?  Either way,
> >> it's a functional change from before?
> > 
> > That's the "and try to not abuse this 'primary view' concept which may
> > map badly to some shells: iterate over all the views instead when it
> > makes sense." part of the commit message.
> > I guess it may be a bug fix in some cases but the main reason was to
> > reduce the usage of the primary view. I can split it out though.
> 
> Maybe I'm just being silly, it's not like landing the two parts
> separately would ever let us back out just the functional change, since
> the other bit depends on it.
> 
> 
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>

Agreed it would be nice to have this as two separate patches but no
biggie.  It all LGTM too, so:

Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>

And pushed:

To ssh://git.freedesktop.org/git/wayland/weston
   8e1efcd..aa97478  master -> master

Btw Giulio don't forget to add a Signed-off-by tag in your patches, and
thanks!

Bryce

> > Thanks,
> > Giulio
> > 
> >>
> >> I'd like to see this as two patches, the one that's removing the view
> >> caching is already
> >> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> >>
> >> (CC me and I'll review the other quickly, I just want to see a commit
> >> log and understand the change)
> >>
> >>>               }
> >>>               return;
> >>>       }
> >>> @@ -1430,7 +1439,6 @@ surface_destroy(struct wl_listener *listener, void *data)
> >>>        * Don't try to use it later. */
> >>>       window->shsurf = NULL;
> >>>       window->surface = NULL;
> >>> -     window->view = NULL;
> >>>  }
> >>>
> >>>  static void
> >>> @@ -2409,9 +2417,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
> >>>       if (!shell_interface->create_shell_surface)
> >>>               return;
> >>>
> >>> -     if (!shell_interface->get_primary_view)
> >>> -             return;
> >>> -
> >>>       if (window->surface->configure) {
> >>>               weston_log("warning, unexpected in %s: "
> >>>                          "surface's configure hook is already set.\n",
> >>> @@ -2423,8 +2428,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
> >>>               shell_interface->create_shell_surface(shell_interface->shell,
> >>>                                                     window->surface,
> >>>                                                     &shell_client);
> >>> -     window->view = shell_interface->get_primary_view(shell_interface->shell,
> >>> -                                                      window->shsurf);
> >>>
> >>>       if (window->name)
> >>>               shell_interface->set_title(window->shsurf, window->name);
> >>>
> >>
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel