Hi, On 13 July 2015 at 11:26, Pekka Paalanen <[email protected]> wrote: > On Sun, 12 Jul 2015 10:52:32 +0300 > Giulio Camuffo <[email protected]> wrote: >> @@ -3888,9 +3891,15 @@ WL_EXPORT void >> weston_output_destroy(struct weston_output *output) >> { >> struct wl_resource *resource; >> + struct weston_view *view; >> >> output->destroying = 1; >> >> + wl_list_for_each(view, &output->compositor->view_list, link) { >> + if (view->output_mask & (1 << output->id)) >> + weston_view_assign_output(view); >> + } > > This now happens before the shell has a chance to react to the output > removed signal and move windows to remaining outputs, does it not? > > The least, this means that clients will get first leave/enter events > from this, and another set from the shell's move. The first round > might even leave the view and the surface completely without an output, > right? > > The shell calls weston_view_geometry_dirty() when it does the move. > This leads to weston_view_update_transform() later, likely as part of > the next repaint. > > weston_{view,surface}_assign_output() uses the derived state, which is > updated by weston_view_update_transform(). This means that simply > moving the weston_view_assign_output() calls in weston_output_destroy() > after the signal emissions does not really help, because it will be > using outdated state. > > Did you consider all this? Do you see the double-events? > > I tried to see if there was any harm from having the output assignment > be NULL temporarily, but it didn't seem like > weston_compositor_build_view_list() machinery would break because of > it. Not for non-subsurface views, anyway... > > Considering outdated state, I suppose weston_output_destroy() may > happen at any time... particularly while view->transform.dirty is true. > Maybe the call should be weston_view_update_transform() instead? If > transform.dirty, then also the output assignments are out-of-date, I > think. > > I'm not sure what the best course of action is. > > > Hmm, I wonder if we should sprinkle assert(!view->transform.dirty) > around things that use weston_view::transform fields... > > Sprinkling weston_view_update_transform() around is not the right way, > because then we may end up sending several leave/enter events to > clients, which again may respond by redrawing the wl_surface (if e.g. > the applicable output_scale changes). > > Tricky.
Indeed. These are all good questions, but not necessarily possible to resolve in bounded time, and this patch was an objective improvement on the previous situation. I reviewed it pretty carefully, so pushed with both mine & Jonas's review. Resolving those questions at some stage would be good, however. :) Cheers, Daniel _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
