*** Bug 758211 has been marked as a duplicate of this bug. ***

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.

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.

This seems a timing issue.
Of having added a NFS filesystem the startup has slowed and the bug disappeared.

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

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

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.

You're my hero, Olivier. Many thanks.

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.

(I mean with attachment 335072 [details] [review] applied)

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.

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.

(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.

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

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.

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.

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.

Review of attachment 335208 [details] [review]:

looks good, thanks for tackling this

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

Does that only fix the NumLock after login or will it also enable it on the login screen/gdm ?

(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)

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 !

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/