From 3022985ba797a061df8c8ac33b2dfb77bf20dcb3 Mon Sep 17 00:00:00 2001 From: John Lindgren Date: Tue, 26 Sep 2023 01:35:36 -0400 Subject: [PATCH] view: try to reduce confusion in focused_view tracking Our current approach to handling the focused/active view is a bit confusing. In particular, it's hard to be sure when server->focused_view is or isn't in sync with the real wlroots keyboard focus. Try to clean things up a bit. In particular: - Add comments to server->focused_view and desktop_focused_view() to clarify that they should match, but it's not guaranteed. - desktop_focused_view() now prints a warning if it detects that server->focused_view is out of sync. We should keep an eye out for this warning, and if we see it, try to figure out why it happened. - For consistency, use only "focus/defocus" as the verbs in function names rather than "activate". This is a bit arbitrary, but the idea is that focus is the primary action while the active/inactive state is a side effect. - view_focus/defocus() replace view_set_activated() and now update both focus and active/inactive state, to try to keep them in sync. - Add comments at view_focus/defocus() to warn against calling them directly (we should generally call the desktop.c functions). - desktop_focus_view(NULL) is now forbidden and is no longer handled as a special case to clear the focus. This was (at least to me) a surprising behavior and caused trouble when working on another change. - To maintain existing behavior, desktop_focus_topmost_mapped_view() now explicitly clears the focus if there are no mapped views. There should be no behavioral change here. --- include/labwc.h | 16 ++++++++++- include/view.h | 3 +- src/action.c | 2 +- src/cursor.c | 4 +-- src/desktop.c | 65 +++++++++++++++++++++++------------------- src/foreign.c | 2 +- src/keyboard.c | 2 +- src/view-impl-common.c | 2 +- src/view.c | 60 +++++++++++++++++++++++++------------- src/xdg.c | 2 +- src/xwayland.c | 2 +- 11 files changed, 100 insertions(+), 60 deletions(-) diff --git a/include/labwc.h b/include/labwc.h index d83d8a00..91c9cfdc 100644 --- a/include/labwc.h +++ b/include/labwc.h @@ -246,6 +246,13 @@ struct server { uint32_t resize_edges; /* SSD state */ + /* + * Currently focused view (cached value). This pointer is used + * primarily to track which view is being drawn with "active" + * SSD coloring. We consider it a bug if this gets out of sync + * with the actual keyboard focus (according to wlroots). See + * also desktop_focused_view(). + */ struct view *focused_view; struct ssd_hover_state *ssd_hover_state; @@ -370,7 +377,7 @@ void foreign_toplevel_update_outputs(struct view *view); * cannot assume this means that the window actually has keyboard * or pointer focus, in this compositor are they called together. */ -void desktop_focus_and_activate_view(struct seat *seat, struct view *view); +void desktop_focus_view(struct view *view); void desktop_arrange_all_views(struct server *server); void desktop_focus_output(struct output *output); struct view *desktop_topmost_mapped_view(struct server *server); @@ -388,6 +395,13 @@ enum lab_cycle_dir { */ struct view *desktop_cycle_view(struct server *server, struct view *start_view, enum lab_cycle_dir dir); +/** + * desktop_focused_view - return the view that is currently focused + * (determined on-the-fly from the wlroots keyboard focus). The return + * value should ideally match server->focused_view (we consider it a + * bug otherwise) but is guaranteed to be up-to-date even if + * server->focused_view gets out of sync. + */ struct view *desktop_focused_view(struct server *server); void desktop_focus_topmost_mapped_view(struct server *server); diff --git a/include/view.h b/include/view.h index ce6a6ce9..7ff69015 100644 --- a/include/view.h +++ b/include/view.h @@ -257,7 +257,8 @@ bool view_isfocusable(struct view *view); bool view_inhibits_keybinds(struct view *view); void view_toggle_keybinds(struct view *view); -void view_set_activated(struct view *view); +void view_focus(struct view *view); +void view_defocus(struct view *view); void view_set_output(struct view *view, struct output *output); void view_close(struct view *view); diff --git a/src/action.c b/src/action.c index e114d193..8433268d 100644 --- a/src/action.c +++ b/src/action.c @@ -634,7 +634,7 @@ actions_run(struct view *activator, struct server *server, break; case ACTION_TYPE_FOCUS: if (view) { - desktop_focus_and_activate_view(&server->seat, view); + desktop_focus_view(view); } break; case ACTION_TYPE_ICONIFY: diff --git a/src/cursor.c b/src/cursor.c index ce102502..4a5b30cd 100644 --- a/src/cursor.c +++ b/src/cursor.c @@ -478,7 +478,7 @@ process_cursor_motion(struct server *server, uint32_t time) } if (ctx.view && rc.focus_follow_mouse) { - desktop_focus_and_activate_view(seat, ctx.view); + desktop_focus_view(ctx.view); if (rc.raise_on_focus) { view_move_to_front(ctx.view); } @@ -522,7 +522,7 @@ _cursor_update_focus(struct server *server) && !rc.focus_follow_mouse_requires_movement && !server->osd_state.cycle_view) { /* Prevents changing keyboard focus during A-Tab */ - desktop_focus_and_activate_view(&server->seat, ctx.view); + desktop_focus_view(ctx.view); if (rc.raise_on_focus) { view_move_to_front(ctx.view); } diff --git a/src/desktop.c b/src/desktop.c index c5a6657e..58177224 100644 --- a/src/desktop.c +++ b/src/desktop.c @@ -35,13 +35,9 @@ desktop_arrange_all_views(struct server *server) } void -desktop_focus_and_activate_view(struct seat *seat, struct view *view) +desktop_focus_view(struct view *view) { - if (!view) { - seat_focus_surface(seat, NULL); - return; - } - + assert(view); /* * Guard against views with no mapped surfaces when handling * 'request_activate' and 'request_minimize'. @@ -51,8 +47,9 @@ desktop_focus_and_activate_view(struct seat *seat, struct view *view) return; } - if (input_inhibit_blocks_surface(seat, view->surface->resource) - || seat->server->session_lock) { + struct server *server = view->server; + if (input_inhibit_blocks_surface(&server->seat, view->surface->resource) + || server->session_lock) { return; } @@ -69,16 +66,7 @@ desktop_focus_and_activate_view(struct seat *seat, struct view *view) return; } - struct wlr_surface *prev_surface; - prev_surface = seat->seat->keyboard_state.focused_surface; - - /* Do not re-focus an already focused surface. */ - if (prev_surface == view->surface) { - return; - } - - view_set_activated(view); - seat_focus_surface(seat, view->surface); + view_focus(view); } static struct wl_list * @@ -210,28 +198,45 @@ struct view * desktop_focused_view(struct server *server) { struct seat *seat = &server->seat; - struct wlr_surface *focused_surface; - focused_surface = seat->seat->keyboard_state.focused_surface; - if (!focused_surface) { - return NULL; - } - struct view *view; - wl_list_for_each(view, &server->views, link) { - if (view->surface == focused_surface) { - return view; + struct wlr_surface *focused_surface = + seat->seat->keyboard_state.focused_surface; + struct view *focused_view = NULL; + + if (focused_surface) { + struct view *view; + wl_list_for_each(view, &server->views, link) { + if (view->surface == focused_surface) { + focused_view = view; + break; + } } } - return NULL; + /* warn so we can identify cases where this occurs */ + if (focused_view != server->focused_view) { + wlr_log(WLR_ERROR, "server->focused_view is out of sync"); + } + + return focused_view; } void desktop_focus_topmost_mapped_view(struct server *server) { struct view *view = desktop_topmost_mapped_view(server); - desktop_focus_and_activate_view(&server->seat, view); if (view) { + desktop_focus_view(view); view_move_to_front(view); + } else { + /* + * Defocus previous focused surface/view if no longer + * focusable (e.g. unmapped or on a different workspace). + * Note than a non-view surface may have been focused. + */ + seat_focus_surface(&server->seat, NULL); + if (server->focused_view) { + view_defocus(server->focused_view); + } } } @@ -257,7 +262,7 @@ desktop_focus_output(struct output *output) } if (wlr_output_layout_intersects(layout, output->wlr_output, &view->current)) { - desktop_focus_and_activate_view(&output->server->seat, view); + desktop_focus_view(view); wlr_cursor_warp(output->server->seat.cursor, NULL, view->current.x + view->current.width / 2, view->current.y + view->current.height / 2); diff --git a/src/foreign.c b/src/foreign.c index 32d09e19..fd5237d7 100644 --- a/src/foreign.c +++ b/src/foreign.c @@ -37,7 +37,7 @@ handle_request_activate(struct wl_listener *listener, void *data) if (view->workspace != view->server->workspace_current) { workspaces_switch_to(view->workspace); } - desktop_focus_and_activate_view(&view->server->seat, view); + desktop_focus_view(view); view_move_to_front(view); } diff --git a/src/keyboard.c b/src/keyboard.c index 01756516..0d5b4c2a 100644 --- a/src/keyboard.c +++ b/src/keyboard.c @@ -41,8 +41,8 @@ keyboard_any_modifiers_pressed(struct wlr_keyboard *keyboard) static void end_cycling(struct server *server) { - desktop_focus_and_activate_view(&server->seat, server->osd_state.cycle_view); if (server->osd_state.cycle_view) { + desktop_focus_view(server->osd_state.cycle_view); view_move_to_front(server->osd_state.cycle_view); } diff --git a/src/view-impl-common.c b/src/view-impl-common.c index 64db4991..1c7a1e71 100644 --- a/src/view-impl-common.c +++ b/src/view-impl-common.c @@ -51,7 +51,7 @@ view_impl_move_sub_views(struct view *parent, enum z_direction z_direction) void view_impl_map(struct view *view) { - desktop_focus_and_activate_view(&view->server->seat, view); + desktop_focus_view(view); view_move_to_front(view); view_update_title(view); view_update_app_id(view); diff --git a/src/view.c b/src/view.c index d43aeafe..ed88739a 100644 --- a/src/view.c +++ b/src/view.c @@ -187,19 +187,50 @@ _view_set_activated(struct view *view, bool activated) } } +/* + * Give the view keyboard focus and mark it active. This function should + * only be called by desktop_focus_view(), which contains additional + * checks to make sure it's okay to give focus. + */ void -view_set_activated(struct view *view) +view_focus(struct view *view) { assert(view); - struct view *last = view->server->focused_view; - if (last == view) { - return; + /* Update seat focus */ + struct seat *seat = &view->server->seat; + if (view->surface != seat->seat->keyboard_state.focused_surface) { + seat_focus_surface(seat, view->surface); } - if (last) { - _view_set_activated(last, false); + /* Update active view */ + if (view != view->server->focused_view) { + if (view->server->focused_view) { + _view_set_activated(view->server->focused_view, false); + } + _view_set_activated(view, true); + view->server->focused_view = view; + } +} + +/* + * Take keyboard focus from the view and mark it inactive. It's rarely + * necessary to call this function directly; usually it's better to + * focus a different view instead by calling something like + * desktop_focus_topmost_mapped_view(). + */ +void +view_defocus(struct view *view) +{ + assert(view); + /* Update seat focus */ + struct seat *seat = &view->server->seat; + if (view->surface == seat->seat->keyboard_state.focused_surface) { + seat_focus_surface(seat, NULL); + } + /* Update active view */ + if (view == view->server->focused_view) { + _view_set_activated(view, false); + view->server->focused_view = NULL; } - _view_set_activated(view, true); - view->server->focused_view = view; } void @@ -377,18 +408,7 @@ _minimize(struct view *view, bool minimized) view->minimized = minimized; if (minimized) { view->impl->unmap(view, /* client_request */ false); - _view_set_activated(view, false); - if (view == view->server->focused_view) { - /* - * Prevents clearing the active view when - * we don't actually have keyboard focus. - * - * This may happen when using a custom mouse - * focus configuration or by using the foreign - * toplevel protocol via some panel. - */ - view->server->focused_view = NULL; - } + view_defocus(view); } else { view->impl->map(view); } diff --git a/src/xdg.c b/src/xdg.c index 5cddf44a..2f429aff 100644 --- a/src/xdg.c +++ b/src/xdg.c @@ -615,7 +615,7 @@ xdg_activation_handle_request(struct wl_listener *listener, void *data) if (view->workspace != view->server->workspace_current) { workspaces_switch_to(view->workspace); } - desktop_focus_and_activate_view(&view->server->seat, view); + desktop_focus_view(view); view_move_to_front(view); } diff --git a/src/xwayland.c b/src/xwayland.c index 604edfad..3a0a8a9a 100644 --- a/src/xwayland.c +++ b/src/xwayland.c @@ -256,7 +256,7 @@ handle_request_activate(struct wl_listener *listener, void *data) return; } - desktop_focus_and_activate_view(&view->server->seat, view); + desktop_focus_view(view); view_move_to_front(view); }