After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 757943 - Numlock is disabled on login with wayland
Numlock is disabled on login with wayland
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
3.20.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 758211 (view as bug list)
Depends on:
Blocks: WaylandRelated
 
 
Reported: 2015-11-11 13:41 UTC by Kamil Páral
Modified: 2017-07-03 18:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsettings-desktop-schemas: [PATCH] schemas: add numlock states (2.15 KB, patch)
2016-09-08 08:15 UTC, Olivier Fourdan
none Details | Review
mutter: [PATCH] wayland: save/restore numlock state (5.40 KB, patch)
2016-09-08 08:16 UTC, Olivier Fourdan
none Details | Review
mutter: [PATCH v2] wayland: save/restore numlock state (14.28 KB, patch)
2016-09-09 09:04 UTC, Olivier Fourdan
none Details | Review
mutter: [PATCH v3] wayland: save/restore numlock state (14.26 KB, patch)
2016-09-09 16:06 UTC, Olivier Fourdan
none Details | Review
mutter: [PATCH v4] wayland: save/restore numlock state (14.90 KB, patch)
2016-09-09 16:22 UTC, Olivier Fourdan
committed Details | Review

Description Kamil Páral 2015-11-11 13:41:44 UTC
With X11 GNOME session, numlock is enabled on login (or maybe it remembers its last state). With Wayland GNOME session, numlock is always disabled on login. Even though it was enabled in my last wayland session, and it was still enabled when I logged out to gdm, once I hit Login button, it gets disabled.

Please remember the last state of numlock even with wayland session.
gnome-shell-3.18.1-1.fc23.x86_64
mutter-3.18.1-4.fc23.x86_64
gnome-settings-daemon-3.18.1-1.fc23.x86_64
Comment 1 Rui Matos 2015-11-17 16:13:07 UTC
*** Bug 758211 has been marked as a duplicate of this bug. ***
Comment 2 Bastien Nocera 2016-04-18 14:36:47 UTC
Got another report of that from a friend.

The code we have for that in X11 lives in gnome-settings-daemon, and won't work on Wayland.
Comment 3 Martin Páleník 2016-07-31 15:02:41 UTC
Is there a temporary workaround? It is quite a showstopper for my mom. She has issues entering her password (containing digits) on the login screen. She uses fully updated Fedora 24 WS.

Both numlockx and Gnome Tweak failed to fix the issue. I am probably forced to autologin and somehow encrypt the /home folder to protect her data. Or use the X11 session with the login screen somehow.
Comment 4 Pecita 2016-09-06 05:46:40 UTC
This seems a timing issue.
Of having added a NFS filesystem the startup has slowed and the bug disappeared.
Comment 5 Olivier Fourdan 2016-09-08 08:14:08 UTC
OK, what we can do is to take a similar approach to what g-s-d does for X, but in mutter so that it works on Wayland.

It means:

 - adding a couple keys "remember-numlock-state" and "numlock-state" to org.gnome.desktop.peripherals.keyboard so that meta-wayland-keyboard can use them

 - adding the xkb bits in meta-wayland-keyboard to save and (optionally, depending on "remember-numlock-state") restore the state

The following two patches (one for gsettings-desktop-schemas and the ohter one for mutter) are an attempt to do that.

I dunno if it's too late for 3.22 though...
Comment 6 Olivier Fourdan 2016-09-08 08:15:38 UTC
Created attachment 335071 [details] [review]
gsettings-desktop-schemas: [PATCH] schemas: add numlock states

Similarly to gnome-settings-daemon, add the keys and enum to save and
restore the state of NumLock to org.gnome.desktop.peripherals.keyboard
Comment 7 Olivier Fourdan 2016-09-08 08:16:30 UTC
Created attachment 335072 [details] [review]
mutter: [PATCH] wayland: save/restore numlock state

Save the state on NumLock so that is can be (optionally) restored on
next login.
Comment 8 Kamil Páral 2016-09-08 08:17:55 UTC
You're my hero, Olivier. Many thanks.
Comment 9 Olivier Fourdan 2016-09-08 08:49:57 UTC
Humm, the state is restored but the LED doesn't stay on, it goes on at startup and goes back off shortly after.

Yet the NumLock status is restored, as seen in dconf-editor and when using the numeric keypad on the keyboard, so it's just the LED that's behaves oddly.
Comment 10 Olivier Fourdan 2016-09-08 08:50:38 UTC
(I mean with attachment 335072 [details] [review] applied)
Comment 11 Rui Matos 2016-09-08 12:54:23 UTC
Review of attachment 335071 [details] [review]:

I think, if we are to fix this for 3.22 at this point in the release cycle, it's better to just make mutter use the g-s-d schema directly. With a runtime check for the schema's existence to handle cases where g-s-d isn't installed.

We can migrate those settings later, changing g-s-d to use the new schema at the same time so that we don't end up with the same setting in two different places.
Comment 12 Rui Matos 2016-09-08 12:57:25 UTC
Review of attachment 335072 [details] [review]:

All of this needs to be done lower in the stack, see clutter-device-manager-evdev.c, clutter-seat-evdec.c and meta-backend-native.c . The only wayland specific bit is a handler to update wayland xkb state similar to the one we already have to update the layout group index.
Comment 13 Bastien Nocera 2016-09-08 12:58:28 UTC
(In reply to Rui Matos from comment #11)
> Review of attachment 335071 [details] [review] [review]:
> 
> I think, if we are to fix this for 3.22 at this point in the release cycle,
> it's better to just make mutter use the g-s-d schema directly. With a
> runtime check for the schema's existence to handle cases where g-s-d isn't
> installed.
> 
> We can migrate those settings later, changing g-s-d to use the new schema at
> the same time so that we don't end up with the same setting in two different
> places.

Or mutter can handle the setting for both X and Wayland, as we usually do. This would avoid splitting the implementation in 2 different places.
Comment 14 Olivier Fourdan 2016-09-08 13:21:39 UTC
Also, for the record, it's gnome-shell itself that turns the LED off afterwards (which explains why my patch works with mutter standalone but not gnome-shell)

from keyboardManager.js _applyLayoutGroup:

  Meta.get_backend().set_keymap()
    +-> meta_backend_native_set_keymap()
      +-> clutter_evdev_set_keyboard_map()
        +-> clutter_evdev_update_xkb_state()
          +-> clutter_evdev_update_xkb_state()
            +-> clutter_seat_evdev_sync_leds()

And that last call turns all LEDs off...
Comment 15 Olivier Fourdan 2016-09-09 09:04:35 UTC
Created attachment 335162 [details] [review]
mutter: [PATCH v2] wayland: save/restore numlock state

(In reply to Rui Matos from comment #11)
> Review of attachment 335071 [details] [review] [review]:
> 
> I think, if we are to fix this for 3.22 at this point in the release cycle,
> it's better to just make mutter use the g-s-d schema directly. With a
> runtime check for the schema's existence to handle cases where g-s-d isn't
> installed.
> 
> We can migrate those settings later, changing g-s-d to use the new schema at
> the same time so that we don't end up with the same setting in two different
> places.

Agreed, done.

(In reply to Rui Matos from comment #12)
> Review of attachment 335072 [details] [review] [review]:
> 
> All of this needs to be done lower in the stack, see
> clutter-device-manager-evdev.c, clutter-seat-evdec.c and
> meta-backend-native.c . The only wayland specific bit is a handler to update
> wayland xkb state similar to the one we already have to update the layout
> group index.

I fail to understand how "all* of this needs to be done lower in the stack.

We can (and need) to set teh xkb_state of the clutter evdev backend in clutter itself, yet we also need to update the gsettings whenever the numlock state is changed by the user (so that it can be restored at next login) and the meta-backend do not deal with keyboard events, so this part, imho, needs to be done in the wayland keyboard code.

So this new iteration does:

 - Add clutter_evdev_set_keyboard_numlock() to ClutterDeviceManager
 - Add a new vfunc set_numlock() to MetaBackendClass
 - Add a meta_backend_native_set_numlock() and meta_backend_x11_set_numlock() impl (although the latter does nothing for now)
 - Add a new meta_backend_set_numlock() entry to the generic backend that calls in the Backend's class vfunc set_numlock()
 - Add a new signal "numlock-state-changed" to MetaBackendClass so that the wayland keyboard can be notified when numlock is changed in the backend
 - Add the necessary hooks in wayland-keyboard to save/restore the numlock state using g-s-d schema and keys (if available)

That seems to work as expected.
Comment 16 Rui Matos 2016-09-09 15:00:54 UTC
Review of attachment 335162 [details] [review]:

All the code you have in meta-wayland-keyboard.c (except for the on_numlock_state_changed handler of course) logically belongs in meta-backend.c since it's not specific to wayland.

But looking at it a bit more we can't just put it there because our ("high level") xkb state tracking is indeed done in meta-wayland-keyboard.c... ugh. We'd need to move that bit somewhere else first. We also have a "low level" xkb state tracking instance in clutter-device-manager-evdev.c but to use that one we'd need to add a gsignal to it just for this purpose, ugh.

Let's keep this in meta-wayland-keyboard.c for now but without the gsignal in the backend since that's just silly given this situation.

::: clutter/clutter/evdev/clutter-device-manager-evdev.c
@@ +2546,3 @@
+
+  xkb_state_update_mask (state, depressed_mods, latched_mods, locked_mods, 0, 0, group_mods);
+  clutter_evdev_update_xkb_state (manager_evdev);

This creates a new xkb state instance which we don't really need to do here. Calling clutter_seat_evdev_sync_leds() should be enough

::: src/wayland/meta-wayland-keyboard.c
@@ +434,3 @@
+      break;
+    }
+  g_settings_set_enum (keyboard->gsd_settings, "numlock-state", numlock_state);

Is there any point in saving if remember-numlock-state is false? I'd prefer if we didn't to avoid the disk write here if not really needed.

@@ +624,3 @@
                     G_CALLBACK (on_keymap_layout_group_changed), keyboard);
+  g_signal_connect (backend, "numlock-state-changed",
+                    G_CALLBACK (on_numlock_state_changed), keyboard);

The purpose of such a signal would be decoupling the wayland frontend from the backend's job of keeping track of numlock state changes.

Since we can't easily do that now and are keeping everything here, let's get rid of the signal and call this handler directly.
Comment 17 Olivier Fourdan 2016-09-09 16:06:19 UTC
Created attachment 335208 [details] [review]
mutter: [PATCH v3] wayland: save/restore numlock state

(In reply to Rui Matos from comment #16)
> Review of attachment 335162 [details] [review] [review]:
> 
> All the code you have in meta-wayland-keyboard.c (except for the
> on_numlock_state_changed handler of course) logically belongs in
> meta-backend.c since it's not specific to wayland.
> 
> But looking at it a bit more we can't just put it there because our ("high
> level") xkb state tracking is indeed done in meta-wayland-keyboard.c... ugh.
> We'd need to move that bit somewhere else first. We also have a "low level"
> xkb state tracking instance in clutter-device-manager-evdev.c but to use
> that one we'd need to add a gsignal to it just for this purpose, ugh.
> 
> Let's keep this in meta-wayland-keyboard.c for now but without the gsignal
> in the backend since that's just silly given this situation.

It's what I thought as well, but... :)

> This creates a new xkb state instance which we don't really need to do here.
> Calling clutter_seat_evdev_sync_leds() should be enough

OK, done, but it needs to iterate through all the seats though, so I changed that.

> Is there any point in saving if remember-numlock-state is false? I'd prefer
> if we didn't to avoid the disk write here if not really needed.

Not really, only use case would be the user changing the "remember-numlock" setting without ever pressing the NumLock key after, in that case, the actual state wouldn't be saved.

But it's a corner case, and I am not ready to add a handler just to monitor the "remember-numlock" setting (too overkill imho) so I'd say let's just don't save if we're not to remember the state...

> The purpose of such a signal would be decoupling the wayland frontend from
> the backend's job of keeping track of numlock state changes.
> 
> Since we can't easily do that now and are keeping everything here, let's get
> rid of the signal and call this handler directly.

Yeah, I removed the signal entirely for now.
Comment 18 Rui Matos 2016-09-09 16:18:25 UTC
Review of attachment 335208 [details] [review]:

looks good, thanks for tackling this
Comment 19 Olivier Fourdan 2016-09-09 16:22:13 UTC
Created attachment 335209 [details] [review]
mutter: [PATCH v4] wayland: save/restore numlock state

(In reply to Olivier Fourdan from comment #17)
> Not really, only use case would be the user changing the "remember-numlock"
> setting without ever pressing the NumLock key after, in that case, the
> actual state wouldn't be saved.
> 
> But it's a corner case, and I am not ready to add a handler just to monitor
> the "remember-numlock" setting (too overkill imho) so I'd say let's just
> don't save if we're not to remember the state...

Actually, I take that back, adding a handler is trivial (Friday...) - Updated patch to take that case into accounts as well.
Comment 20 Olivier Fourdan 2016-09-09 17:48:49 UTC
Comment on attachment 335209 [details] [review]
mutter: [PATCH v4] wayland: save/restore numlock state

attachment 335209 [details] [review] pushed to git master after rebase as commit 4c106a9 - wayland: save/restore numlock state
Comment 21 AnAkkk 2016-09-10 09:00:14 UTC
Does that only fix the NumLock after login or will it also enable it on the login screen/gdm ?
Comment 22 Olivier Fourdan 2016-09-12 06:35:50 UTC
(In reply to AnAkkk from comment #21)
> Does that only fix the NumLock after login or will it also enable it on the
> login screen/gdm ?

gdm uses gnome-shell and mutter so that should also work in gdm (it does here)
Comment 23 antistress 2017-05-15 00:29:23 UTC
Hi, 
Reading that bug it's unclear to me what version of what libraries are needed to get the fix (mutter, wayland...). Could you precise that point please ?
I'm running Debian Sid and still have that bug. Therefore I would like to check my libraries versions before reporting a bug in Debian bug tracker.
Thanks !
Comment 24 Laurent Bigonville 2017-07-03 18:51:02 UTC
For the debian user coming here, this is related to this override[0] in gnome-settings-daemon package.

I'll try fix this soon in stretch.


[0]https://sources.debian.net/src/gnome-settings-daemon/3.22.2-2/debian/gnome-settings-daemon.gsettings-override/