On 04.07.2016 14:14, Pekka Paalanen wrote: Hi Pekka,
All your points below are good, so I don't think I need to reply for each one of them with "OK". I'll implement all your suggestions and reply again if I encounter any issues with your proposals. Thanks, Armin. > On Thu, 30 Jun 2016 19:55:01 +0200 > Armin Krezović <[email protected]> wrote: > >> When a surface gets created and no outputs are present, >> it won't get properly configured. This happens because >> call to configure_presented_surface() depends on a call >> to fs_output_set_surface() which requires a valid output. >> >> This patch makes the code save the surface when no >> outputs are present and reconfigure it when an output >> gets attached, so it gets properly displayed. >> >> Signed-off-by: Armin Krezović <[email protected]> >> --- >> fullscreen-shell/fullscreen-shell.c | 31 ++++++++++++++++++++++++++++++- >> 1 file changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/fullscreen-shell/fullscreen-shell.c >> b/fullscreen-shell/fullscreen-shell.c >> index 9716dc5..f58df2f 100644 >> --- a/fullscreen-shell/fullscreen-shell.c >> +++ b/fullscreen-shell/fullscreen-shell.c >> @@ -46,6 +46,8 @@ struct fullscreen_shell { >> struct wl_listener output_created_listener; >> >> struct wl_listener seat_created_listener; >> + >> + struct wl_list unmapped_surfaces; > > Hi Armin, > > this member should have a comment saying what link element it contains. > > You mentioned that you would have another patch for handling the case > where a client uses present_surface with a NULL output first, and then > the final output get unplugged and re-plugged. Would it make sense if > this was a list of surfaces presented with NULL output instead? > > You'd go through those surfaces every time an output gets plugged in, > to create a copy of the view on that output. I suppose that case is not > implemented at all yet in upstream? > > Might be better to have both patches in the same series, since I think > they could share a lot. > >> }; >> >> struct fs_output { >> @@ -83,6 +85,12 @@ struct pointer_focus_listener { >> struct wl_listener seat_destroyed; >> }; >> >> +struct fs_surface_list { > > More like an item than a list, yes? > >> + struct weston_surface *surface; > > This member needs to be accompanied with a destroy listener, I believe. > A client request (wl_surface.destroy) may cause weston_surface to be > destroyed, and you do not want to be left with a stale pointer. > >> + enum zwp_fullscreen_shell_v1_present_method method; >> + struct wl_list link; >> +}; >> + >> static void >> pointer_focus_changed(struct wl_listener *listener, void *data) >> { >> @@ -245,10 +253,15 @@ pending_surface_destroyed(struct wl_listener >> *listener, void *data) >> fsout->pending.surface = NULL; >> } >> >> +static void >> +configure_presented_surface(struct weston_surface *surface, int32_t sx, >> + int32_t sy); >> + >> static struct fs_output * >> fs_output_create(struct fullscreen_shell *shell, struct weston_output >> *output) >> { >> struct fs_output *fsout; >> + struct fs_surface_list *surf, *next; >> >> fsout = zalloc(sizeof *fsout); >> if (!fsout) >> @@ -271,6 +284,14 @@ fs_output_create(struct fullscreen_shell *shell, struct >> weston_output *output) >> weston_layer_entry_insert(&shell->layer.view_list, >> &fsout->black_view->layer_link); >> wl_list_init(&fsout->transform.link); >> + >> + wl_list_for_each_safe(surf, next, &shell->unmapped_surfaces, link) { >> + fs_output_set_surface(fsout, surf->surface, surf->method, 0, 0); > > This actually shows that unmapped_surfaces can only ever contain one > surface, or at least that is what makes sense for this piece of code. > > If fullscreen-shell was written to support the client switching like > the protocol spec defines, you could have at most one surface for the > NULL-output per client. It doesn't, but we can still keep the list as > you wrote it, since that is probably how someone adding multi-client > support would do it. > > To be really correct, you'd have to check the surface is from the right > client. You could just add a XXX comment, like below. > >> + configure_presented_surface(surf->surface, 0, 0); >> + wl_list_remove(&surf->link); > > If the list gathered the surfaces presented for a NULL-output, you > wouldn't remove the link here. > >> + free(surf); > > Otherwise the code looks fine for what you meant it to do. > >> + } >> + >> return fsout; >> } >> >> @@ -676,6 +697,7 @@ fullscreen_shell_present_surface(struct wl_client >> *client, >> struct weston_surface *surface; >> struct weston_seat *seat; >> struct fs_output *fsout; >> + struct fs_surface_list *surf; >> >> surface = surface_res ? wl_resource_get_user_data(surface_res) : NULL; >> >> @@ -692,7 +714,13 @@ fullscreen_shell_present_surface(struct wl_client >> *client, >> "Invalid presentation method"); >> } >> >> - if (output_res) { >> + if (wl_list_empty(&shell->output_list)) { > > I think this should happen only if output_res is NULL. > > If output_res is not NULL, then the client thinks the output still > exists. If our output list is empty however, the client just has not > had the chance to process the wl_registry.global_remove event yet. The > client should process it soon, and when it does, it should fix the > situation with the wl_surface on its own. > > If output_res is not NULL, the surface is not for the "show everywhere" > case, but for a specific output. Once a global has been removed, it > cannot come back, so the wl_output will be permanently inert too. It > doesn't matter if the same monitor gets plugged back to the same > connector, we don't maintain that association at the protocol level. > >> + surf = zalloc(sizeof *surf); >> + surf->surface = surface; >> + surf->method = method; >> + wl_list_init(&surf->link); > > Init not needed, wl_list_insert() overwrites it anyway. > >> + wl_list_insert(shell->unmapped_surfaces.prev, &surf->link); > > Here it is possible that several surfaces end up in the list. However, > only the latest one should stay there per client. > > Fullscreen-shell does not support multiple clients atm. anyway, so just > having a XXX comment would be enough. > >> + } else if (output_res) { >> output = wl_resource_get_user_data(output_res); >> fsout = fs_output_for_output(output); >> fs_output_set_surface(fsout, surface, method, 0, 0); >> @@ -831,6 +859,7 @@ module_init(struct weston_compositor *compositor, >> return -1; >> >> shell->compositor = compositor; >> + wl_list_init(&shell->unmapped_surfaces); >> >> shell->client_destroyed.notify = client_destroyed; >> > > Essentially a good patch with some details to fix, but I'd like to see > the second use case addressed in the same series too. > > > Thanks, > pq >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
