From 9d233b73a30a4e4d45a579e1f60c23b107d3d08f Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Thu, 5 Apr 2018 19:50:56 +0200 Subject: [PATCH] nitpicker: improve 'Session::focus' handling Nitpicker's 'Session:focus' call used to trigger a one-off focus change at call time. This focus change did not pass the same code paths as a focus change triggered by a "focus" ROM update, which led to inconsistencies. This patch changes the implementation of 'Session::focus' such that the relationship of the caller and the focused session is preserved after call time. Whenever the calling session is focused in the future, the specified session will receive the focus instead. So 'Session::focus' represents no longer a single operation but propagates the information about the inter-session relationship. This information is taken into account whenever the focus is evaluated regardless of how the change is triggered. This makes the focus handling in scenarios like the window manager more robust. Issue #2746 --- .../nitpicker_session/nitpicker_session.h | 19 +++++----- repos/os/src/server/nitpicker/focus.h | 13 +------ repos/os/src/server/nitpicker/main.cc | 15 +++++--- .../src/server/nitpicker/session_component.cc | 36 +++++++++++++++++- .../src/server/nitpicker/session_component.h | 18 ++++++--- repos/os/src/server/nitpicker/user_state.cc | 37 ++----------------- repos/os/src/server/nitpicker/user_state.h | 28 +------------- repos/os/src/server/nitpicker/view_owner.h | 5 +++ 8 files changed, 76 insertions(+), 95 deletions(-) diff --git a/repos/os/include/nitpicker_session/nitpicker_session.h b/repos/os/include/nitpicker_session/nitpicker_session.h index faddff839..38f74e51c 100644 --- a/repos/os/include/nitpicker_session/nitpicker_session.h +++ b/repos/os/include/nitpicker_session/nitpicker_session.h @@ -297,21 +297,22 @@ struct Nitpicker::Session : Genode::Session /** * Set focused session * - * Normally, the focused session is defined by the user by clicking on a - * view. The 'focus' method allows a client to set the focus without user - * action. However, the change of the focus is performed only is the - * currently focused session belongs to a child or the same process as the - * called session. This relationship is checked by comparing the session - * labels of the currently focused session and the caller. This way, a - * common parent can manage the focus among its child processes. But a - * session cannot steal the focus from an unrelated session. + * Normally, the focused session is defined by the 'focus' ROM, which is + * driven by an external policy component. However, in cases where one + * application consists of multiple nitpicker sessions, it is desirable to + * let the application manage the focus among its sessions by applying an + * application-local policy. The 'focus' RPC function allows a client to + * specify another client that should receive the focus whenever the + * session becomes focused. As the designated receiver of the focus is + * referred to by its session capability, a common parent can manage the + * focus among its children. But unrelated sessions cannot interfere. */ virtual void focus(Genode::Capability focused) = 0; typedef Genode::String<160> Label; enum Session_control { SESSION_CONTROL_HIDE, SESSION_CONTROL_SHOW, - SESSION_CONTROL_TO_FRONT }; + SESSION_CONTROL_TO_FRONT }; /** * Perform control operation on one or multiple sessions diff --git a/repos/os/src/server/nitpicker/focus.h b/repos/os/src/server/nitpicker/focus.h index 49dde7067..28351c5c7 100644 --- a/repos/os/src/server/nitpicker/focus.h +++ b/repos/os/src/server/nitpicker/focus.h @@ -19,7 +19,7 @@ namespace Nitpicker { struct Focus; - struct Focus_controller; + struct Focus_updater : Interface { virtual void update_focus() = 0; }; } @@ -74,15 +74,4 @@ class Nitpicker::Focus : Noncopyable } }; - -/** - * Interface used by a nitpicker client to assign the focus to a session of - * one of its child components (according to the session labels) - */ -struct Nitpicker::Focus_controller : Interface -{ - virtual void focus_view_owner(View_owner const &caller, - View_owner &next_focused) = 0; -}; - #endif /* _FOCUS_H_ */ diff --git a/repos/os/src/server/nitpicker/main.cc b/repos/os/src/server/nitpicker/main.cc index 36da7adb4..3917dd95a 100644 --- a/repos/os/src/server/nitpicker/main.cc +++ b/repos/os/src/server/nitpicker/main.cc @@ -33,9 +33,6 @@ #include "domain_registry.h" namespace Nitpicker { - - struct Focus_updater : Interface { virtual void update_focus() = 0; }; - template class Root; struct Main; } @@ -106,7 +103,7 @@ class Nitpicker::Root : public Root_component, bool const provides_default_bg = (label == "backdrop"); Session_component *session = new (md_alloc()) - Session_component(_env, label, _view_stack, _font, _user_state, + Session_component(_env, label, _view_stack, _font, _focus_updater, _pointer_origin, _builtin_background, _framebuffer, provides_default_bg, *md_alloc(), unused_quota, _focus_reporter, *this); @@ -127,6 +124,10 @@ class Nitpicker::Root : public Root_component, void _destroy_session(Session_component *session) { + /* invalidate pointers held by other sessions to the destroyed session */ + for (Session_component *s = _session_list.first(); s; s = s->next()) + s->forget(*session); + _session_list.remove(session); _global_keys.apply_config(_config.xml(), _session_list); @@ -482,14 +483,16 @@ void Nitpicker::Main::_handle_focus() typedef Session::Label Label; Label const label = _focus_rom->xml().attribute_value("label", Label()); - /* determine session that matches the label found in the focus ROM */ + /* + * Determine session that matches the label found in the focus ROM + */ View_owner *next_focus = nullptr; for (Session_component *s = _session_list.first(); s; s = s->next()) if (s->label() == label) next_focus = s; if (next_focus) - _user_state.focus(*next_focus); + _user_state.focus(next_focus->forwarded_focus()); else _user_state.reset_focus(); } diff --git a/repos/os/src/server/nitpicker/session_component.cc b/repos/os/src/server/nitpicker/session_component.cc index 8fd0c62c4..77bd8cc23 100644 --- a/repos/os/src/server/nitpicker/session_component.cc +++ b/repos/os/src/server/nitpicker/session_component.cc @@ -48,6 +48,36 @@ bool Session_component::_views_are_equal(View_handle v1, View_handle v2) } +View_owner &Session_component::forwarded_focus() +{ + Session_component *next_focus = this; + + /* helper used for detecting cycles */ + Session_component *next_focus_slow = next_focus; + + for (bool odd = false; ; odd = !odd) { + + /* we found the final focus once the forwarding stops */ + if (!next_focus->_forwarded_focus) + break; + + next_focus = next_focus->_forwarded_focus; + + /* advance 'next_focus_slow' every odd iteration only */ + if (odd) + next_focus_slow = next_focus_slow->_forwarded_focus; + + /* a cycle is detected if 'next_focus' laps 'next_focus_slow' */ + if (next_focus == next_focus_slow) { + error("cyclic focus forwarding by ", next_focus->label()); + break; + } + } + + return *next_focus; +} + + void Session_component::_execute_command(Command const &command) { switch (command.opcode) { @@ -407,11 +437,13 @@ void Session_component::focus(Capability session_cap) if (this->cap() == session_cap) return; - Session_component const &caller = *this; + _forwarded_focus = nullptr; _env.ep().rpc_ep().apply(session_cap, [&] (Session_component *session) { if (session) - _focus_controller.focus_view_owner(caller, *session); }); + _forwarded_focus = session; }); + + _focus_updater.update_focus(); } diff --git a/repos/os/src/server/nitpicker/session_component.h b/repos/os/src/server/nitpicker/session_component.h index eb79faaf3..a345cba4d 100644 --- a/repos/os/src/server/nitpicker/session_component.h +++ b/repos/os/src/server/nitpicker/session_component.h @@ -101,7 +101,7 @@ class Nitpicker::Session_component : public Rpc_object, Font const &_font; - Focus_controller &_focus_controller; + Focus_updater &_focus_updater; Signal_context_capability _mode_sigh { }; @@ -135,6 +135,8 @@ class Nitpicker::Session_component : public Rpc_object, Visibility_controller &_visibility_controller; + Session_component *_forwarded_focus = nullptr; + /** * Calculate session-local coordinate to physical screen position * @@ -157,8 +159,6 @@ class Nitpicker::Session_component : public Rpc_object, */ bool _views_are_equal(View_handle, View_handle); - bool _focus_change_permitted() const; - void _execute_command(Command const &); void _destroy_view(View_component &); @@ -169,7 +169,7 @@ class Nitpicker::Session_component : public Rpc_object, Session_label const &label, View_stack &view_stack, Font const &font, - Focus_controller &focus_controller, + Focus_updater &focus_updater, View_component &pointer_origin, View_component &builtin_background, Framebuffer::Session &framebuffer, @@ -184,7 +184,7 @@ class Nitpicker::Session_component : public Rpc_object, _session_alloc(&session_alloc, ram_quota), _framebuffer(framebuffer), _framebuffer_session_component(view_stack, *this, framebuffer, *this), - _view_stack(view_stack), _font(font), _focus_controller(focus_controller), + _view_stack(view_stack), _font(font), _focus_updater(focus_updater), _pointer_origin(pointer_origin), _builtin_background(builtin_background), _framebuffer_session_cap(_env.ep().manage(_framebuffer_session_component)), @@ -295,6 +295,8 @@ class Nitpicker::Session_component : public Rpc_object, xml.attribute("domain", _domain->name()); } + View_owner &forwarded_focus() override; + /**************************************** ** Interface used by the main program ** @@ -346,6 +348,12 @@ class Nitpicker::Session_component : public Rpc_object, _framebuffer_session_component.submit_sync(); } + void forget(Session_component &session) + { + if (_forwarded_focus == &session) + _forwarded_focus = nullptr; + } + /********************************* ** Nitpicker session interface ** diff --git a/repos/os/src/server/nitpicker/user_state.cc b/repos/os/src/server/nitpicker/user_state.cc index 2fa74c446..23e618e8e 100644 --- a/repos/os/src/server/nitpicker/user_state.cc +++ b/repos/os/src/server/nitpicker/user_state.cc @@ -174,7 +174,7 @@ void User_state::_handle_input_event(Input::Event ev) } if (_hovered->has_transient_focusable_domain()) { - global_receiver = _hovered; + global_receiver = &_hovered->forwarded_focus(); } else { /* * Distinguish the use of the builtin focus switching and the @@ -187,9 +187,9 @@ void User_state::_handle_input_event(Input::Event ev) * methods. */ if (_focus_via_click) - _focus_view_owner_via_click(*_hovered); + _focus_view_owner_via_click(_hovered->forwarded_focus()); else - global_receiver = _hovered; + global_receiver = &_hovered->forwarded_focus(); _last_clicked = _hovered; } @@ -422,37 +422,6 @@ void User_state::forget(View_owner const &owner) } -bool User_state::_focus_change_permitted(View_owner const &caller) const -{ - /* - * If no session is focused, we allow any client to assign it. This - * is useful for programs such as an initial login window that - * should receive input events without prior manual selection via - * the mouse. - * - * In principle, a client could steal the focus during time between - * a currently focused session gets closed and before the user - * manually picks a new session. However, in practice, the focus - * policy during application startup and exit is managed by a - * window manager that sits between nitpicker and the application. - */ - if (!_focused) - return true; - - /* - * Check if the currently focused session label belongs to a - * session subordinated to the caller, i.e., it originated from - * a child of the caller or from the same process. This is the - * case if the first part of the focused session label is - * identical to the caller's label. - */ - char const * const focused_label = _focused->label().string(); - char const * const caller_label = caller.label().string(); - - return strcmp(focused_label, caller_label, strlen(caller_label)) == 0; -} - - void User_state::_focus_view_owner_via_click(View_owner &owner) { _next_focused = &owner; diff --git a/repos/os/src/server/nitpicker/user_state.h b/repos/os/src/server/nitpicker/user_state.h index 4f038a400..2eb48b517 100644 --- a/repos/os/src/server/nitpicker/user_state.h +++ b/repos/os/src/server/nitpicker/user_state.h @@ -27,7 +27,7 @@ namespace Nitpicker { class User_state; } -class Nitpicker::User_state : public Focus_controller +class Nitpicker::User_state { private: @@ -125,8 +125,6 @@ class Nitpicker::User_state : public Focus_controller } _key_array { }; - bool _focus_change_permitted(View_owner const &caller) const; - void _focus_view_owner_via_click(View_owner &); void _handle_input_event(Input::Event); @@ -172,30 +170,6 @@ class Nitpicker::User_state : public Focus_controller { } - /******************************** - ** Focus_controller interface ** - ********************************/ - - void focus_view_owner(View_owner const &caller, - View_owner &next_focused) override - { - /* check permission by comparing session labels */ - if (!_focus_change_permitted(caller)) { - warning("unauthorized focus change requesed by ", caller.label()); - return; - } - - /* - * To avoid changing the focus in the middle of a drag operation, - * we cannot perform the focus change immediately. Instead, it - * comes into effect via the 'apply_pending_focus_change()' method - * called the next time when the user input is handled and no drag - * operation is in flight. - */ - _next_focused = &next_focused; - } - - /**************************************** ** Interface used by the main program ** ****************************************/ diff --git a/repos/os/src/server/nitpicker/view_owner.h b/repos/os/src/server/nitpicker/view_owner.h index fda97678a..424edef65 100644 --- a/repos/os/src/server/nitpicker/view_owner.h +++ b/repos/os/src/server/nitpicker/view_owner.h @@ -93,6 +93,11 @@ struct Nitpicker::View_owner : Interface * Produce report with the owner's information */ virtual void report(Xml_generator &) const { } + + /** + * Recipient of the focus whenever this view owner becomes focused + */ + virtual View_owner &forwarded_focus() { return *this; } }; #endif /* _VIEW_OWNER_H_ */