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 610245 - Apply keyboard settings to newly plugged in devices
Apply keyboard settings to newly plugged in devices
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: plugins
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
: 585222 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-02-17 11:12 UTC by Bastien Nocera
Modified: 2010-10-13 14:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Apply keyboard settings to newly plugged in devices (3.76 KB, patch)
2010-02-17 11:12 UTC, Bastien Nocera
none Details | Review
Apply all keyboard settings to new keyboards (5.21 KB, patch)
2010-03-09 14:43 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2010-02-17 11:12:23 UTC
Just like in the mouse plugin.
Comment 1 Bastien Nocera 2010-02-17 11:12:25 UTC
Created attachment 154015 [details] [review]
Apply keyboard settings to newly plugged in devices

Fixes repeat rate and the likes not being applied to newly
plugged in devices.
Comment 2 Peter Hutterer 2010-02-17 11:29:02 UTC
Review of attachment 154015 [details] [review]:

+                GConfClient *client;
+		client = gconf_client_get_default ();
+                apply_settings (client, 0, NULL, data);

tab/space mixup

+        XSelectExtensionEvent (display,
+                               RootWindow (display, DefaultScreen (display)),
+                               &class_presence, 1);

This has the potential to screw up. the class array is the list of events the client wants to know about, so you're essentially overwriting any other event masks. This is unlikely to be an issue given that the other plugins only register for presence events as well but it's something that we should at least be aware of. I think g-s-d shares one client connection between plugins. is that right (don't have access to the code atm)? if not, it's a non-issue anyway. Other than that - looks sane.
Comment 3 Bastien Nocera 2010-02-17 15:07:12 UTC
(In reply to comment #2)
> Review of attachment 154015 [details] [review]:
> 
> +                GConfClient *client;
> +        client = gconf_client_get_default ();
> +                apply_settings (client, 0, NULL, data);
> 
> tab/space mixup

Fixed locally

> +        XSelectExtensionEvent (display,
> +                               RootWindow (display, DefaultScreen (display)),
> +                               &class_presence, 1);
> 
> This has the potential to screw up. the class array is the list of events the
> client wants to know about, so you're essentially overwriting any other event
> masks. This is unlikely to be an issue given that the other plugins only
> register for presence events as well but it's something that we should at least
> be aware of. I think g-s-d shares one client connection between plugins. is
> that right (don't have access to the code atm)? if not, it's a non-issue
> anyway. Other than that - looks sane.

Only 2 of the plugins use this function, and both check for device presence (mouse and keyboard plugins). Maybe we could clean that up and use a common function?
Comment 4 Bastien Nocera 2010-02-17 15:11:12 UTC
I forgot to mention. If we're going to keep the current code, what would a fixed call look like? The XGetSelectedExtensionEvents() API is a bit weird...
Comment 5 Peter Hutterer 2010-02-18 07:03:21 UTC
something like this should do:

int nevents, nother_events;
XEventClass *events, *other_events;

XGetSelectedExtensionEvents(dpy, win, &nevents, &events, 
                            &nother_events, &other_events);
XFree(other_events); /* don't care about those */

for (i = 0; i < nevents; i++)
   if (events[i] == presence_class)
        return; /* already selected, don't bother */

nevents++;
events = realloc(events, nevents * sizeof(XEventClass));
events[nevents - 1] = presence_class;
XSelectExtensionEvents(dpy, win, events, nevents);
XFree(events);

you can ignore the the other_events mask, it doesn't matter for us. strictly speaking the realloc isn't correct since xlib allocated memory for us in a supposedly magic manner, so a new alloc and memcpy might be better. in practice, xlib uses malloc and free, unsurprisingly.

also, the man page for XGetSelectedExtensionEvent is wrong, thanks for pointing that out :)
Comment 6 Jens Granseuer 2010-02-20 09:48:04 UTC
IIRC Sergey once said he wanted this functionality in libgnomkbd or something instead. Sergey?
Comment 7 Sergey V. Udaltsov 2010-02-20 12:22:08 UTC
Yes, that functionality is there. Libxklavier, if built with xinput support, should detect plugged in devices and generate X-new-device event.

That event is processed by g-s-d (see gsd-keyboard-xkb.c).

Please check if it works for you.
Comment 8 Oliver Joos 2010-03-02 01:44:24 UTC
I was very glad when I noticed your discussion. Now I am curious: has the patch of Bastien already been tested successfully? Or why is this bug report 'RESOLVED' already?

Further I want to thank everybody involved in fixing this! It really hurts users like me with laptop + docking station. See https://bugs.launchpad.net/bugs/427168 and its 4 duplicates for other thankful victims.
Comment 9 Sergey V. Udaltsov 2010-03-02 08:22:28 UTC
already.
Comment 10 Oliver Joos 2010-03-08 12:23:33 UTC
Meanwhile I checked the patch in attachment 154015 [details] [review] myself and confirm: it works as expected. IMHO it is ready to be committed to 2.29.x.

To verify it I used a LiveCD of Ubuntu 10.04 alpha 3 and I did:
$ sudo apt-get build-dep gnome-settings-daemon
$ apt-get source gnome-settings-daemon
$ cd gnome-settings-daemon-2.29.91.1/
$ patch plugins/keyboard/gsd-keyboard-manager.c attachment_154015.patch 
$ ./configure --prefix=/usr
$ make
$ sudo make install
logout and re-login to Gnome => bug fixed!
Comment 11 Sebastien Bacher 2010-03-08 14:06:04 UTC
the libxklavier code seems to not be working, lucid has libxklavier 5.0 built with libxi and xinput showed as activated by the configure summary but it seems it's not working
Comment 12 Bastien Nocera 2010-03-08 14:08:21 UTC
(In reply to comment #7)
> Yes, that functionality is there. Libxklavier, if built with xinput support,
> should detect plugged in devices and generate X-new-device event.
> 
> That event is processed by g-s-d (see gsd-keyboard-xkb.c).
> 
> Please check if it works for you.

Except that that doesn't set the settings from gsd-keyboard-manager.c, only the XKB settings.
Comment 13 Sergey V. Udaltsov 2010-03-08 22:43:39 UTC
Sebastien, it seems to be working fine in 9.10. Do you know anything about changes in XInput in 10.04?

Could you please run tests/test_monitor (after "make check" that builds it):

XKL_DEBUG=200 test_monitor -l3

Does it detect plugged out/in kbd? Sorry, I do not have 10.04 so far.

I do not like the attached plugin - I would prefer to fix libxklavier.
Comment 14 Bastien Nocera 2010-03-08 23:25:43 UTC
(In reply to comment #13)
> Sebastien, it seems to be working fine in 9.10. Do you know anything about
> changes in XInput in 10.04?
> 
> Could you please run tests/test_monitor (after "make check" that builds it):
> 
> XKL_DEBUG=200 test_monitor -l3
> 
> Does it detect plugged out/in kbd? Sorry, I do not have 10.04 so far.
> 
> I do not like the attached plugin - I would prefer to fix libxklavier.

The problem isn't that keymaps aren't getting set, the problem is that the settings that are applied in the -manager.c part of the plugin aren't applied because there's nothing listening for new keyboards arriving there.
Comment 15 Sergey V. Udaltsov 2010-03-08 23:46:36 UTC
Look, there is a code in gsd-keyboard-xkb.c

                if (xkl_engine_get_features (xkl_engine) &
                    XKLF_DEVICE_DISCOVERY)
                        g_signal_connect (xkl_engine, "X-new-device",
                                          G_CALLBACK
                                          (gsd_keyboard_new_device), NULL);

That's what should make it work.
Comment 16 Bastien Nocera 2010-03-09 14:43:04 UTC
(In reply to comment #15)
> Look, there is a code in gsd-keyboard-xkb.c
> 
>                 if (xkl_engine_get_features (xkl_engine) &
>                     XKLF_DEVICE_DISCOVERY)
>                         g_signal_connect (xkl_engine, "X-new-device",
>                                           G_CALLBACK
>                                           (gsd_keyboard_new_device), NULL);
> 
> That's what should make it work.

No, it doesn't. It doesn't apply the repeat settings, the bell settings or the num-lock status.
Comment 17 Bastien Nocera 2010-03-09 14:43:20 UTC
Created attachment 155653 [details] [review]
Apply all keyboard settings to new keyboards

When libxklavier sends in a new device event, apply all the
settings to the new keyboard.
Comment 18 Bastien Nocera 2010-03-09 14:43:50 UTC
The attached patch should work when libxklavier is built with XInput support.
Comment 19 Sergey V. Udaltsov 2010-03-09 21:13:32 UTC
I am happy with the patch. Bastien, would you commit it? Or I can do it...
Comment 20 Bastien Nocera 2010-03-10 10:43:55 UTC
Attachment 155653 [details] pushed as f168d90 - Apply all keyboard settings to new keyboards
Comment 21 Bastien Nocera 2010-03-10 11:09:36 UTC
Pushed to gnome-2-28 as well.
Comment 22 Oliver Joos 2010-03-10 14:20:37 UTC
@Bastien: thank you very much for fixing this, and for backporting it!
Comment 23 Sergey V. Udaltsov 2010-03-10 18:23:49 UTC
Thanks indeed!
Comment 24 Bastien Nocera 2010-10-13 14:40:18 UTC
*** Bug 585222 has been marked as a duplicate of this bug. ***