Support keyboard navigation over multiple monitors in multitasking view#2536
Support keyboard navigation over multiple monitors in multitasking view#2536leolost2605 wants to merge 9 commits intomainfrom
Conversation
919ea31 to
139ae2f
Compare
d0aa36a to
6507c10
Compare
6507c10 to
96b1148
Compare
96b1148 to
3888e2a
Compare
3888e2a to
fb04de5
Compare
|
@lenemter could you give this a review? :) |
| @@ -10,7 +10,7 @@ | |||
| * It will propagate gesture events to all direct descendants that are also {@link ActorTarget}s. | |||
| * If a new child (or target via {@link add_target}) is added, its progress will be synced. | |||
| */ | |||
| public class Gala.ActorTarget : Clutter.Actor, GestureTarget { | |||
There was a problem hiding this comment.
I don't like adding Focusable here, this intertwines focusable and gestures systems which is counter intuitive
There was a problem hiding this comment.
The idea was to kind of grow Focusable into a Gala.Widget someday if we have to add more stuff that is common to most ui parts. I just didn't quite want to commit to calling it that yet but we can do that. I'm not quite sure I understand the counter intutitive part but I'm open to other ideas
There was a problem hiding this comment.
We could even merge the actor target into the widget then 🤷
There was a problem hiding this comment.
Or at least something like a MultitaskingViewWidget. Because we need a way to propagate these things through the actor hierarchy and the best way for this is a base class IMHO but like I said I'm open to other suggestions
There was a problem hiding this comment.
I mean that widgets should explicitly implement Focusable.
TBH I spent like 20 minutes to understand how FocusController works with MultitaskingView because I didn't catch that ActorTarget (which is supposed to handle gestures) unexpectedly handles focus state now.
There was a problem hiding this comment.
ActorTarget (which is supposed to handle gestures) unexpectedly handles focus state now
Well it doesn't handle focus state but it derives from a class that does.
I get what you're saying but we kind of really need a base class otherwise everything will just be a giant hack (at least what I can think of, like I said I'm still very much open to other ideas). And I'm not sure anyone does it differently? Both GNOME Shell and GTK have focus management in the widget base class and do it similar to this PR.
Once again what do you think about introducing a widget? And then renaming ActorTarget to WidgetTarget? This would solve overlooking actortarget extending focusable.
This way widgets also explicitly use focusable because they are using a baseclass that's called Widget(Target) and widgets bring focus management.
87170b0 to
a57c179
Compare
|
@lenemter I've added two commits on top that rename focusable to widget and then actortarget to widget target. Let me know what you think! |
|
@leolost2605 I think it would be better to leave ActorTarget as is, rename Focusable to Widget, and make Widget subclass ActorTarget. This way Widget uses gesture infrastructure, not the other way around. What do you think? |
a57c179 to
280ecc4
Compare

Instead of manually handling keyboard focus where needed, do it like GTK does it with some adaptations.
We have the FocusController which is added to the "root" focusable of a focus tree (e.g. mtv or window overview).
Then we have the focusable class which is subclassed by the actors in the actor tree where focus is needed. The focusable handles deciding which implementor gets the focus based on the direction.
We now also hide the focus indicator after 5 seconds.
Fixes #2651
Fixes #2648
We can easily add navigation via Tab now too but I would leave that to a follow up