GNOME Bugzilla – Bug 610245
Apply keyboard settings to newly plugged in devices
Last modified: 2010-10-13 14:40:18 UTC
Just like in the mouse plugin.
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.
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.
(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?
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...
something like this should do: int nevents, nother_events; XEventClass *events, *other_events; XGetSelectedExtensionEvents(dpy, win, &nevents, &events, ¬her_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 :)
IIRC Sergey once said he wanted this functionality in libgnomkbd or something instead. Sergey?
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.
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.
already.
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!
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
(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.
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.
(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.
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.
(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.
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.
The attached patch should work when libxklavier is built with XInput support.
I am happy with the patch. Bastien, would you commit it? Or I can do it...
Attachment 155653 [details] pushed as f168d90 - Apply all keyboard settings to new keyboards
Pushed to gnome-2-28 as well.
@Bastien: thank you very much for fixing this, and for backporting it!
Thanks indeed!
*** Bug 585222 has been marked as a duplicate of this bug. ***