<div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">On 5 September 2014 12:07, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On Mon, 1 Sep 2014 17:20:32 +0200<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> When we are moving window or its state changed, update the<br>
> output, so that configure event and maximizing/fullscreening actions<br>
> have up-to-date information.<br>
><br>
> Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br>
> ---<br>
> desktop-shell/shell.c | 39 ++++++++++++++++++++-------------------<br>
> 1 file changed, 20 insertions(+), 19 deletions(-)<br>
><br>
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c<br>
> index 26f13cc..d5996a3 100644<br>
> --- a/desktop-shell/shell.c<br>
> +++ b/desktop-shell/shell.c<br>
> @@ -474,6 +474,22 @@ shell_surface_state_changed(struct shell_surface *shsurf)<br>
> }<br>
><br>
> static void<br>
> +shell_surface_set_output(struct shell_surface *shsurf,<br>
> + struct weston_output *output)<br>
> +{<br>
> + struct weston_surface *es = shsurf->surface;<br>
> +<br>
> + /* get the default output, if the client set it as NULL<br>
> + check whether the ouput is available */<br>
> + if (output)<br>
> + shsurf->output = output;<br>
> + else if (es->output)<br>
> + shsurf->output = es->output;<br>
> + else<br>
> + shsurf->output = get_default_output(es->compositor);<br>
> +}<br>
> +<br>
> +static void<br>
> shell_grab_end(struct shell_grab *grab)<br>
> {<br>
> if (grab->shsurf) {<br>
> @@ -484,6 +500,8 @@ shell_grab_end(struct shell_grab *grab)<br>
> grab->shsurf->resize_edges = 0;<br>
> shell_surface_state_changed(grab->shsurf);<br>
> }<br>
> +<br>
> + shell_surface_set_output(grab->shsurf, NULL);<br>
<br>
</div></div>Why does grab_end() reset the output? That looks strange</blockquote><div> </div><div>Sorry, my mistake here, I should have explicitly written what output. When we give NULL as the<br>argument, it sets the output to the surface->output if the surface is mapped.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Surely if<br>
something happened during a grab that would need shsurf->output change,<br>
shouldn't it change there and not blindly here?<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div>Yes, probably yes.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="h5">
> }<br>
><br>
> weston_pointer_end_grab(grab->grab.pointer);<br>
> @@ -2371,22 +2389,6 @@ shell_surface_set_parent(struct shell_surface *shsurf,<br>
> }<br>
><br>
> static void<br>
> -shell_surface_set_output(struct shell_surface *shsurf,<br>
> - struct weston_output *output)<br>
> -{<br>
> - struct weston_surface *es = shsurf->surface;<br>
> -<br>
> - /* get the default output, if the client set it as NULL<br>
> - check whether the ouput is available */<br>
> - if (output)<br>
> - shsurf->output = output;<br>
> - else if (es->output)<br>
> - shsurf->output = es->output;<br>
> - else<br>
> - shsurf->output = get_default_output(es->compositor);<br>
> -}<br>
> -<br>
> -static void<br>
> surface_clear_next_states(struct shell_surface *shsurf)<br>
> {<br>
> shsurf->next_state.maximized = false;<br>
> @@ -5227,9 +5229,6 @@ configure(struct desktop_shell *shell, struct weston_surface *surface,<br>
> if (surface->output) {<br>
> wl_list_for_each(view, &surface->views, surface_link)<br>
> weston_view_update_transform(view);<br>
> -<br>
> - if (shsurf->state.maximized)<br>
> - surface->output = shsurf->output;<br>
<br>
</div></div>When you maximize a window, you want its synced-to-output to be the<br>
output it was maximized for. Why remove this here, and what fills the<br>
purpose now?<br>
<span class=""><br></span></blockquote><div><br></div><div>Because we set shsurf->output to surface->output before, so this is kind of cycle. Now I realized that this is true only if<br></div><div>the surface was mapped - if it was not, the surface->output is NULL which does not need to be the<br>case of shsurf->output.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
> }<br>
> }<br>
><br>
> @@ -5277,6 +5276,8 @@ shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)<br>
> float from_x, from_y;<br>
> float to_x, to_y;<br>
><br>
> + shell_surface_set_output(shsurf, NULL);<br>
<br>
</span>Why would a client committing new content to a surface always cause the<br>
shsurf->output to be reset?<br>
<br>
... what is shsurf->output, anyway?<br>
<span class=""><br>
> +<br>
> if (shsurf->resize_edges) {<br>
> sx = 0;<br>
> sy = 0;<br>
<br>
</span>Sorry, I just don't understand anything this patch does, and it all<br>
looks strange to me.<br>
<br>
I think we may also be missing some concepts here...<br>
<br>
A wl_surface is always synced to one output, that is the<br>
weston_surface::output. The sync is implemented in Weston core, the<br>
repaint machinery, that sends the frame callbacks.<br>
<br>
Then we have the complication, that there may be multiple weston_views<br>
on different outputs, and I am unsure what their meaning wrt. to output<br>
is.<br>
<br>
In shell, we have shell_surface::output and<br>
shell_surface::fullscreen_output.<br>
<br>
I have assumed that shell_surface::output is the output that the client<br>
says the surface should be on, like when maximizing or fullscreening on<br>
a specific output. When the surface state becomes maximized or<br>
fullscreened, its sync should be locked to the client specified output.<br>
<br></blockquote><div><br></div><div>Here is what I missed. I thought that the shell_surface::output is the output<br>the shell_surface is really on (sort of copy of surface->output),<br>so that is why I tried to update it whenever it might changed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I don't know what shell_surface::output is nowadays, and why we need<br>
fullscreen_output to be separate, or when/how those are assigned to the<br>
weston_surface::output.<br></blockquote><div><br></div><div>Hmm, I don't know why it is separated either. The shell_surface::output is<br></div><div>set when fullscreening and maximizing, but not in xdg-shell. So the best solution<br>probably would be to set the output in xdg_surface_set_maximized().<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Have we lost track on what the principles here should be?<br>
<br>
Only when a client does not explicitly specify an output, we should use<br>
all the heuristics, but when a client has specified an output, we<br>
should not overwrite that, in which case it should be impossible to<br>
move the surface away from the specified output. Except then we have<br>
the corner-case, where the surface is basically hidden, but has a<br>
secondary view on a different output (e.g. a thumbnail), which means<br>
the wl_surface should be temporarily synced to the outputs it is<br>
actually on, when it is not on the intended output.<br>
<br>
Complicated...<br>
<br>
The patch 2/2 I can understand, but this patch I would like to hear<br>
more justification for.<br>
<br>
<br>
Thanks,<br>
pq<br></blockquote><div><br></div><div>Thanks for review,<br>Marek <br></div></div><br></div></div>