[PATCH v2 1/2] shell: update shsurf->output when it might changed

[Marek Chalupa]

Hi,

On 5 September 2014 12:07, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Mon,  1 Sep 2014 17:20:32 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > When we are moving window or its state changed, update the
> > output, so that configure event and maximizing/fullscreening actions
> > have up-to-date information.
> >
> > Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> > ---
> >  desktop-shell/shell.c | 39 ++++++++++++++++++++-------------------
> >  1 file changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index 26f13cc..d5996a3 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -474,6 +474,22 @@ shell_surface_state_changed(struct shell_surface
> *shsurf)
> >  }
> >
> >  static void
> > +shell_surface_set_output(struct shell_surface *shsurf,
> > +                         struct weston_output *output)
> > +{
> > +     struct weston_surface *es = shsurf->surface;
> > +
> > +     /* get the default output, if the client set it as NULL
> > +        check whether the ouput is available */
> > +     if (output)
> > +             shsurf->output = output;
> > +     else if (es->output)
> > +             shsurf->output = es->output;
> > +     else
> > +             shsurf->output = get_default_output(es->compositor);
> > +}
> > +
> > +static void
> >  shell_grab_end(struct shell_grab *grab)
> >  {
> >       if (grab->shsurf) {
> > @@ -484,6 +500,8 @@ shell_grab_end(struct shell_grab *grab)
> >                       grab->shsurf->resize_edges = 0;
> >                       shell_surface_state_changed(grab->shsurf);
> >               }
> > +
> > +             shell_surface_set_output(grab->shsurf, NULL);
>
> Why does grab_end() reset the output? That looks strange


Sorry, my mistake here, I should have explicitly written what output. When
we give NULL as the
argument, it sets the output to the surface->output if the surface is
mapped.


> Surely if
> something happened during a grab that would need shsurf->output change,
> shouldn't it change there and not blindly here?
>
>
Yes, probably yes.


> >       }
> >
> >       weston_pointer_end_grab(grab->grab.pointer);
> > @@ -2371,22 +2389,6 @@ shell_surface_set_parent(struct shell_surface
> *shsurf,
> >  }
> >
> >  static void
> > -shell_surface_set_output(struct shell_surface *shsurf,
> > -                         struct weston_output *output)
> > -{
> > -     struct weston_surface *es = shsurf->surface;
> > -
> > -     /* get the default output, if the client set it as NULL
> > -        check whether the ouput is available */
> > -     if (output)
> > -             shsurf->output = output;
> > -     else if (es->output)
> > -             shsurf->output = es->output;
> > -     else
> > -             shsurf->output = get_default_output(es->compositor);
> > -}
> > -
> > -static void
> >  surface_clear_next_states(struct shell_surface *shsurf)
> >  {
> >       shsurf->next_state.maximized = false;
> > @@ -5227,9 +5229,6 @@ configure(struct desktop_shell *shell, struct
> weston_surface *surface,
> >       if (surface->output) {
> >               wl_list_for_each(view, &surface->views, surface_link)
> >                       weston_view_update_transform(view);
> > -
> > -             if (shsurf->state.maximized)
> > -                     surface->output = shsurf->output;
>
> When you maximize a window, you want its synced-to-output to be the
> output it was maximized for. Why remove this here, and what fills the
> purpose now?
>
>
Because we set shsurf->output to surface->output before, so this is kind of
cycle. Now I realized that this is true only if
the surface was mapped - if it was not, the surface->output is NULL which
does not need to be the
case of shsurf->output.


> >       }
> >  }
> >
> > @@ -5277,6 +5276,8 @@ shell_surface_configure(struct weston_surface *es,
> int32_t sx, int32_t sy)
> >               float from_x, from_y;
> >               float to_x, to_y;
> >
> > +             shell_surface_set_output(shsurf, NULL);
>
> Why would a client committing new content to a surface always cause the
> shsurf->output to be reset?
>
> ... what is shsurf->output, anyway?
>
> > +
> >               if (shsurf->resize_edges) {
> >                       sx = 0;
> >                       sy = 0;
>
> Sorry, I just don't understand anything this patch does, and it all
> looks strange to me.
>
> I think we may also be missing some concepts here...
>
> A wl_surface is always synced to one output, that is the
> weston_surface::output. The sync is implemented in Weston core, the
> repaint machinery, that sends the frame callbacks.
>
> Then we have the complication, that there may be multiple weston_views
> on different outputs, and I am unsure what their meaning wrt. to output
> is.
>
> In shell, we have shell_surface::output and
> shell_surface::fullscreen_output.
>
> I have assumed that shell_surface::output is the output that the client
> says the surface should be on, like when maximizing or fullscreening on
> a specific output. When the surface state becomes maximized or
> fullscreened, its sync should be locked to the client specified output.
>
>
Here is what I missed. I thought that the shell_surface::output is the
output
the shell_surface is really on (sort of copy of surface->output),
so that is why I tried to update it whenever it might changed.


> I don't know what shell_surface::output is nowadays, and why we need
> fullscreen_output to be separate, or when/how those are assigned to the
> weston_surface::output.
>

Hmm, I don't know why it is separated either. The shell_surface::output is
set when fullscreening and maximizing, but not in xdg-shell. So the best
solution
probably would be to set the output in xdg_surface_set_maximized().


> Have we lost track on what the principles here should be?
>
> Only when a client does not explicitly specify an output, we should use
> all the heuristics, but when a client has specified an output, we
> should not overwrite that, in which case it should be impossible to
> move the surface away from the specified output. Except then we have
> the corner-case, where the surface is basically hidden, but has a
> secondary view on a different output (e.g. a thumbnail), which means
> the wl_surface should be temporarily synced to the outputs it is
> actually on, when it is not on the intended output.
>
> Complicated...
>
> The patch 2/2 I can understand, but this patch I would like to hear
> more justification for.
>
>
> Thanks,
> pq
>

Thanks for review,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140908/2d61619a/attachment.html>