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

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<Session> 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

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_ */

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 <typename> class Root;
 	struct Main;
 }
@@ -106,7 +103,7 @@ class Nitpicker::Root : public Root_component<Session_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<Session_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();
 }

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<Nitpicker::Session> 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();
 }
 
 

repos/os/src/server/nitpicker/session_component.h

@@ -101,7 +101,7 @@ class Nitpicker::Session_component : public  Rpc_object<Session>,
 
 		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<Session>,
 
 		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<Session>,
 		 */
 		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>,
 		                  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>,
 			_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<Session>,
 				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<Session>,
 			_framebuffer_session_component.submit_sync();
 		}
 
+		void forget(Session_component &session)
+		{
+			if (_forwarded_focus == &session)
+				_forwarded_focus = nullptr;
+		}
+
 
 		/*********************************
 		 ** Nitpicker session interface **

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;

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 **
 		 ****************************************/

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_ */