Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression for lxqt-runner: no focus if not clicked by mouse #1653

Closed
stefonarch opened this issue Mar 23, 2024 · 21 comments · Fixed by #1656
Closed

Regression for lxqt-runner: no focus if not clicked by mouse #1653

stefonarch opened this issue Mar 23, 2024 · 21 comments · Fixed by #1656
Labels
bug Something isn't working investigation required Needs further investigation regression

Comments

@stefonarch
Copy link
Contributor

stefonarch commented Mar 23, 2024

In current master lxqt-runner which is launched only by shortcut doesn't receive focus anymore automatically.

Keyboardinteractivity was set originally to OnDemand but even setting it to Exclusive doesn't change behavior - launching it and typing the typed keys go to the open document or terminal.

Reverting 6990ff7 solves.

Runner is still a PR: lxqt/lxqt-runner#277

@tsujan
Copy link

tsujan commented Mar 23, 2024

Just wanted to add that lxqt/lxqt-runner#277 doesn't have any problem in labwc 0.7.1 — I'm using them — and just sets the layer at the top.

@johanmalm
Copy link
Collaborator

We've made quite a few changes to layer-shell keyboard-interactivity recently (#1599).

I do really appreciate you reporting this, because there isn't much 'standard implementation' to go on and the protocol is quite vague on on-demand handling.

I have taken "Request regular keyboard focus semantics" to mean that the surface gets focus when receiving mouse-button-press, but not whenever the surfaces changes its keyboard-interactivity to 'on-demand'. I'm quite happy to revert 6990ff7 because I think it's highly likely that the only time a layer-shell surface changes its interactivity to on-demand is on map (or shortly after).

The only consideration I can think of before reverting is that this means that any layer-shell client with on-demand interactivity will 'grab' the keyboard when started (assuming that's when it sets the interactivity to on-demand, which is most likely the case). Appreciate help in thinking through if there are cases when this is not desirable... to avoid chasing our tail on this one.

As a side-note, I'm in the process of creating a labwc-layer-shell(5) man page to describe the implementation.

In terms of the described issue, please could confirm:

  1. Is the observation made on first launch, or is the client already running in the background and is being invoked by a keyboard-shortcut (via a unix-socket presumably?)
  2. What is the context of choosing on-demand rather than exclusive for it. Apologies - I've never used lxqt-runner and can't get it to work on my system 😊 (didn't try for that long though).
  3. I'm puzzled by the comment regarding Exclusive in OP. With exclusive keyboard interactivity it should still grab the keyboard when launched. Are you definite on that observation?
  4. Does the client get mapped/unmapped more than once (for example to wait in an unmapped state to be opened by a keyboard shortcut). I did remove the focus-setting from the map-handler in this commit: eaa995e Sway does set the keyboard interactivity on map, so maybe we're missing a trick here. https://github.com/swaywm/sway/blob/5a7477cb8f568ce4aeb852215ad40899f18f3d91/sway/desktop/layer_shell.c#L250-L263
  5. Just to confirm, when you click on the runner, does it get keyboard focus?

@tsujan
Copy link

tsujan commented Mar 23, 2024

Is the observation made on first launch

lxqt-runner starts hidden. It's made visible by a shortcut and gets hidden when it launches an app, or when its Qt window gets inactive (e.g., by clicking into another window), or when Escape is pressed. The problem with git labwc happened every time it was shown: no focus, unless the user focused it by clicking into its window. With labwc 0.7.1, it's focused when shown, as expected.

What is the context of choosing on-demand rather than exclusive for it.

According to @stefonarch's tests, "exclusive" didn't make a difference (sorry, I can't test now, using the latest stable labwc). The code is a simple layer-shell-qt code like this;

                layershell->setLayer(LayerShellQt::Window::Layer::LayerTop);
                layershell->setKeyboardInteractivity(LayerShellQt::Window::KeyboardInteractivityOnDemand);
                LayerShellQt::Window::Anchors anchors = {LayerShellQt::Window::AnchorTop};
                layershell->setAnchors(anchors);
                ...

EDIT: The original code uses "on-demand", as above.

@tsujan
Copy link

tsujan commented Mar 23, 2024

Just to confirm, when you click on the runner, does it get keyboard focus?

Yes (although I edited my above comment to emphasize that it does). The problem with git labwc is that it doesn't get focus automatically when shown.

@Consolatis
Copy link
Member

Consolatis commented Mar 23, 2024

Lets not revert 6990ff7.

But maybe the logic isn't right, the comment states that Must be (a) change from EXCLUSIVE to ON_DEMAND, maybe that is not actually true? E.g. can we end up in a situation where the assumption fails and it was actually ON_DEMAND before and still is (for example, did we forget to reset seat->focused_layer on unmap of that layer surface)? (Also the following part of the comment seems wrong, it doesn't "give us focus" but rather it drops focus which would be the right thing to do in an exclusive -> on_demand situation I think).

A WAYLAND_DEBUG=1 log would help to figure out what is actually sent in which order.

Basically the question is if a newly mapped client with ON_DEMAND set should get focus or not.
Usual (non-layershell) windows do so it could be argued that this should also happen in this case, at least if layer >= top.

Just mapping and then requesting ON_DEMAND after the fact (e.g. when it was the default NONE before) would be correct to not give keyboard focus, you'd need to change to EXCLUSIVE for that.

According to @stefonarch's tests, "exclusive" didn't make a difference (sorry, I can't test now, using the latest stable labwc). The code is a simple layer-shell-qt code like this

Is this still the case?

@Consolatis Consolatis added bug Something isn't working investigation required Needs further investigation regression labels Mar 23, 2024
@stefonarch
Copy link
Contributor Author

stefonarch commented Mar 24, 2024

Is this still the case?

If Exclusive changes behavior? It was so yesterday, I don't think it's changed but could test again.

EDIT: made a test run still with Exclusive:

  • sway: same as labwc - no keyboard on show runner ( I think that's new)
  • Hyprland: all fine
  • kwin: all fine
  • wayfire won't start and blocks PC atm...

I don't use other launchers like wofi or rofi, how do they handle show/hide?

@Consolatis
Copy link
Member

Interesting. Exclusive + top layer should always give you keyboard focus unless there is some other layershell client with exclusive. So maybe this is about a map / unmap / map cycle of a layershell surface. A WAYLAND_DEBUG=1 log would really help here.

@stefonarch
Copy link
Contributor Author

I did the following, got a quite big file:

WAYLAND_DEBUG=1 /bin/labwc -s lxqt-runner > runner.debug.log 2>&1

  1. forwarded shortcuts
  2. launched runner with Alt+space, it was hidden as usual
  3. typed ddd , it had no focus
  4. moved mouse to the runner window to get focus
  5. typed again ddd
  6. exit

runner.debug.log.tar.gz

@Consolatis
Copy link
Member

Consolatis commented Mar 24, 2024

WAYLAND_DEBUG=1 should only be necessary for lxqt-runner itself, not needed for the whole of labwc. E.g. you can just use WAYLAND_DEBUG=1 lxqt-runner > runner.debug.log 2>&1 under your usual labwc session.
I'll check the logs, thanks.

Edit:
That log only uses zwlr_layer_surface_v1@33.set_keyboard_interactivity(2) which means ON_DEMAND.

@stefonarch
Copy link
Contributor Author

Ooops, sure. Here a more compact log.
runner.debug.log.tar.gz

@stefonarch
Copy link
Contributor Author

stefonarch commented Mar 24, 2024

That log only uses zwlr_layer_surface_v1@33.set_keyboard_interactivity(2) which means ON_DEMAND.
Yes, I used the default runner, will do with the `Exclusive" package.

EDIT: looks like with "Exclusive" it's working now, not sure why it didn't yesterday.
runner.debug.log

@Consolatis
Copy link
Member

EDIT: looks like with "Exclusive" it's working now, not sure why it didn't yesterday.

Alright, the log looks clean otherwise. (wl_surface is created, wrapped into a a layershell surface, configures keyboard interactivity and then maps). So it really is just the question of how labwc should treat newly mapped layer surfaces with ON_DEMAND set.

Basically the question is if a newly mapped client with ON_DEMAND set should get focus or not.
Usual (non-layershell) windows do so it could be argued that this should also happen in this case, at least if layer >= top.

@Consolatis
Copy link
Member

Does this fix it with ON_DEMAND?

diff --git a/src/layers.c b/src/layers.c
index a18bc4fa..5d863dc1 100644
--- a/src/layers.c
+++ b/src/layers.c
@@ -348,6 +348,8 @@ handle_map(struct wl_listener *listener, void *data)
 	 * Processing of keyboard-interactivity changes is done in the
 	 * commit-handler.
 	 */
+	layer_try_set_focus(&layer->server->seat,
+		layer->scene_layer_surface->layer_surface);
 }
 
 static void

@stefonarch
Copy link
Contributor Author

Does this fix it with ON_DEMAND?

It does, but I need better triple testing ;)

@Consolatis
Copy link
Member

Consolatis commented Mar 24, 2024

Does this fix it with ON_DEMAND?

It does, but I need better triple testing ;)

Sweet, thanks for confirming. Thoughts @johanmalm ?
Should we restrict that to layer >= top? Otherwise, we'd give for example pcmanfm focus when spawed.

Note that this might also apply for use-cases like notification daemons (usually in top or overlay layers) when they use ON_DEMAND. Differentiating between these cases seems impossible. So for notification daemons the best course of action would likely be to use NONE and if they actually want keyboard interactivity to change it to on_demand after the map or on mouse enter (so it requires the user to click).

@droc12345
Copy link
Contributor

Otherwise, we'd give for example pcmanfm focus when spawed.

If something like pcmanfm was giving focus would that make it show up in the window switcher
running in all workspace mode? While playing with this you might check to make sure you're not
letting it start appearing as a regular window. Although if it appeared as a regular window for
a short period of time, normal focus rules should apply.

@tsujan
Copy link

tsujan commented Mar 24, 2024

If something like pcmanfm was giving focus would that make it show up in the window switcher
running in all workspace mode?

The desktop of git pcmanfm-qt doesn't show up in switcher. It's completely ready for Wayland.

EDIT: I mean pcmanfm-qt's desktop; its windows are normal.

@Consolatis
Copy link
Member

Consolatis commented Mar 24, 2024

layershell clients generally don't show up in the window switcher (or in panels using the foreign toplevel protocol).

@droc12345
Copy link
Contributor

I know a layer-shell shouldn't show up, but I have no idea how well implemented qt's version is.
It was more random thoughts as I was reading the discussion.

johanmalm added a commit to johanmalm/labwc that referenced this issue Mar 24, 2024
...for the following reasons:

1. We interpret 'normal input-focus semantics' for clients with on-demand
   keyboard interactivity to means that a surface receives input focus on
   cursor-button-press AND on map (the latter previously missing), just
   like a normal window would. In this regard, we do not differentiate
   between layers.

2. Most layer-surfaces set the keyboard interactivity at a similar time to
   their first (and normally only) map, so the absence of an explicit
   attempt to focus on map does not make a difference. However, for a
   long-running layer-shell client (such as lxqt-runner) which sets the
   interactivity on launch and then maps/unmaps many times throughout its
   lifetime, a specific focus-attempt is required on map to avoid the
   client itself having to keep resetting its interactivity to grab the
   keyboard on map.

3. Compositors like sway and river process focus (for clients with
   keyboard-interactivity)  in their map-handlers, so this makes for a
   common approach.

Fixes: labwc#1653
@johanmalm
Copy link
Collaborator

Sweet, thanks for confirming. Thoughts @johanmalm?

My mind was heading towards the map-handler too last night. Thanks for patch + testing. See #1656

Should we restrict that to layer >= top? Otherwise, we'd give for example pcmanfm focus when spawed.

I'm pretty easy on this, have taken your patch as is based on the fact that if we're applying 'normal input-focus semantics' to mean mouse-clock or map, then it's more consistent to not differentiated between layers.

If a desktop client like pcmanfm-qt --desktop or xfdesktop (with on-demand interactivity) was launched when there were already normal windows open, I think it's reasonable that such a client receives input-focus (so in other words, I don't think we're causing a problem or doing something unreasonable). I struggle to see why a background-layer client would want to keep mapping/unmapping, but hey - sometimes these use-cases are hard to predict!

Note that this might also apply for use-cases like notification daemons (usually in top or overlay layers) when they use ON_DEMAND. Differentiating between these cases seems impossible. So for notification daemons the best course of action would likely be to use NONE and if they actually want keyboard interactivity to change it to on_demand after the map or on mouse enter (so it requires the user to click).

Agree. Hope we don't break something else with this.

I checked sway+river map-handlers and they both try to focus if the client has keyboard-interactivity (which includes on-demand), so adopting the same approach seems sensible.

Consolatis pushed a commit that referenced this issue Mar 24, 2024
...for the following reasons:

1. We interpret 'normal input-focus semantics' for clients with on-demand
   keyboard interactivity to means that a surface receives input focus on
   cursor-button-press AND on map (the latter previously missing), just
   like a normal window would. In this regard, we do not differentiate
   between layers.

2. Most layer-surfaces set the keyboard interactivity at a similar time to
   their first (and normally only) map, so the absence of an explicit
   attempt to focus on map does not make a difference. However, for a
   long-running layer-shell client (such as lxqt-runner) which sets the
   interactivity on launch and then maps/unmaps many times throughout its
   lifetime, a specific focus-attempt is required on map to avoid the
   client itself having to keep resetting its interactivity to grab the
   keyboard on map.

3. Compositors like sway and river process focus (for clients with
   keyboard-interactivity)  in their map-handlers, so this makes for a
   common approach.

Fixes: #1653
@Consolatis
Copy link
Member

Consolatis commented Mar 24, 2024

Should be fixed in latest master, thanks for the report, debugging and testing :)

grisha128 pushed a commit to grisha128/labwc that referenced this issue Apr 19, 2024
...for the following reasons:

1. We interpret 'normal input-focus semantics' for clients with on-demand
   keyboard interactivity to means that a surface receives input focus on
   cursor-button-press AND on map (the latter previously missing), just
   like a normal window would. In this regard, we do not differentiate
   between layers.

2. Most layer-surfaces set the keyboard interactivity at a similar time to
   their first (and normally only) map, so the absence of an explicit
   attempt to focus on map does not make a difference. However, for a
   long-running layer-shell client (such as lxqt-runner) which sets the
   interactivity on launch and then maps/unmaps many times throughout its
   lifetime, a specific focus-attempt is required on map to avoid the
   client itself having to keep resetting its interactivity to grab the
   keyboard on map.

3. Compositors like sway and river process focus (for clients with
   keyboard-interactivity)  in their map-handlers, so this makes for a
   common approach.

Fixes: labwc#1653
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigation required Needs further investigation regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants