Created attachment 1902 Screenshot The window manager shortcuts cannot be configured -- the tab is grayed out, see screenshot.
not surprising, it's not implemented, bug Jannis...
Created attachment 1920 Adds shortcut support to xfwm4 This patch adds shortcut support to xfwm4. It uses XfceShortcutsProvider to manage the shortcuts (clone defaults, shortcut add/removal notifications). This patch also modifies the way shortcuts are parsed into MyKey structs. Olivier, could you review this patch? If you have questions, just ask. If you want XfceShortcutsProvider to be better integrated into xfwm4 (e.g. by renaming it), I can do that. I suppose it makes sense to keep it this way though because this way to implement shortcuts requires very little modifications to xfwm4 itself.
Here's the stat output for the patch for you to have a better overview about where I made changes: src/Makefile.am | 4 +- src/keyboard.c | 119 +++----- src/keyboard.h | 3 +- src/screen.h | 6 +- src/settings.c | 285 ++++++++++++----- src/xfce-shortcuts-provider.c | 676 +++++++++++++++++++++++++++++++++++++++++ src/xfce-shortcuts-provider.h | 87 ++++++ 7 files changed, 1014 insertions(+), 166 deletions(-)
*** Bug 4525 has been marked as a duplicate of this bug. ***
As discussed on irc, the code for XfceShortcutsProvider should be better suited in a private lib instead of being duplicated in different parts of the project. Also, this patch replaces the Xlib based code with gtk_accelerator_parse(), including getModifierMap(). What bug or defect in the current code does this change fix? I am a bit reluctant to "fix" the code unless it is broken (especially in this case, this code is fairly sensitive and has been proven to work for years, so I need to be reassured here). Also, I am not entirely sure this is correct: Every time the keyboard mapping is modified (xmodmap, layout change, etc.), we need to re-grab the key shortcuts and buttons. This is done by monitoring XMappingEvent in handleMappingNotify() in events.c, and call reloadSettings() if the modifier mapping has changed (note that this should also update keyboard shortcuts too, and that is possibly missing from current code!). But the X events are handled in a gtk event filter, which means that the X events are treated before gtk+ receive them, so gdk/gtk+ is not yet aware of the mapping notify when handleMappingNotify() is called, so I wonder if gtk_accelerator_parse() would give the correct expected results in that case.
Also, can you explain the rationale behind this change: @@ -353,10 +318,6 @@ initModifiers (Display * dpy) { ScrollLockMask = (1 << (i / modmap->max_keypermod)); } - else if (!AltMask && ((syms[j] == XK_Alt_L) || (syms[j] == XK_Alt_R))) - { - AltMask = (1 << (i / modmap->max_keypermod)); - } else if (!SuperMask && ((syms[j] == XK_Super_L) || (syms[j] == XK_Super_R))) { SuperMask = (1 << (i / modmap->max_keypermod));
(In reply to comment #5) > As discussed on irc, the code for XfceShortcutsProvider should be better suited > in a private lib instead of being duplicated in different parts of the project. Agreed and done (moved the code from xfce4-settings and xfwm4 into libxfcegui4 as discussed). > Also, this patch replaces the Xlib based code with gtk_accelerator_parse(), > including getModifierMap(). What bug or defect in the current code does this > change fix? I am a bit reluctant to "fix" the code unless it is broken > (especially in this case, this code is fairly sensitive and has been proven to > work for years, so I need to be reassured here). The shortcut strings generated by XfceShortcutDialog are created using gtk_accelerator_name (keycode, modifiers). The rationale behind using gtk_accelerator_parse (shortcut_string) is to make sure the string is properly parsed. Since the shortcut string format has changed to the GTK+ accelerator format, there is no need implement this ourselves anymore. Especially if there is a pair of methods like this in GTK+ for exactly that purpose. > Also, I am not entirely sure this is correct: Every time the keyboard mapping > is modified (xmodmap, layout change, etc.), we need to re-grab the key > shortcuts and buttons. This is done by monitoring XMappingEvent in > handleMappingNotify() in events.c, and call reloadSettings() if the modifier > mapping has changed (note that this should also update keyboard shortcuts too, > and that is possibly missing from current code!). > > But the X events are handled in a gtk event filter, which means that the X > events are treated before gtk+ receive them, so gdk/gtk+ is not yet aware of > the mapping notify when handleMappingNotify() is called, so I wonder if > gtk_accelerator_parse() would give the correct expected results in that case. Yes, you're right. I had some issues due to this which I wasn't able to explain until now. Yesterday on the train back home I debugged this and the problem seems to be exactly what you describe here. We have two options here: 1. Parse the accelerators ourselves (this requires to implement the full GTK+ accelerator format) and use MappingNotify to regrab the keys. 2. Connect to the "keys-changed" signal of the default GdkKeymap and regrab the keys in the signal callback. This could be done in initSettings(). "keys-changed" is emitted after the MappingNotify event is handled by GDK/GTK+. It works fine in combination with gtk_accelerator_parse(). It's already being used in xfce4-settings-helper and I'm running xfwm4 with it without any problems. It also reduces the code in xfwm4 a bit. The code for handling the signal could look like this: static void cb_keys_changed (GdkKeymap *keymap, ScreenInfo *screen_info) { initModifiers (myScreenGetXDisplay (screen_info)); reloadSettings (screen_info->display_info, UPDATE_BUTTON_GRABS); } Personally, I lean towards 2. because it a) uses well-tested GDK/GTK+ methods b) works like a charm in xfce4-settings-helper and c) simplifies the xfwm4 code by a few lines. 1. has the advantage of having full control over the MappingNotify event. Currently this event is only used to regrab the keys. IMHO it's not necessary to do this on the X level since there is a GDK mechanism for exactly the same thing. > Also, can you explain the rationale behind this change: > > @@ -353,10 +318,6 @@ initModifiers (Display * dpy) > { > ScrollLockMask = (1 << (i / modmap->max_keypermod)); > } > - else if (!AltMask && ((syms[j] == XK_Alt_L) || (syms[j] == > XK_Alt_R))) > - { > - AltMask = (1 << (i / modmap->max_keypermod)); > - } > else if (!SuperMask && ((syms[j] == XK_Super_L) || > (syms[j] == XK_Super_R))) > { > SuperMask = (1 << (i / modmap->max_keypermod)); Yeah, but I only have a bad rationale for that: it somehow fixed a part of the "gtk_accelerator_parse() inside the MappingNotify handler" mistake you explained above. I have reverted this change in my local copy already.
Created attachment 1930 Revised patch This is a revised patch using the "keys-changed" signal of the default GdkKeymap. It uses libxfce4kbd-private for everything related to the shortcuts. Compared to the patch before I also fixed a few indentation inconsistencies.
I think this patch can go in, but there is still one big thing missing... defaults. All defaults are empty, and there is no default shortcut at all (not even Alt-Tab work by default). Ideally, we should get rid of keythemerc and the corresponding files (given that the default values are available, possibly with an xfconf xml file?)
(In reply to comment #9) > I think this patch can go in, but there is still one big thing missing... > defaults. > > All defaults are empty, and there is no default shortcut at all (not even > Alt-Tab work by default). > > Ideally, we should get rid of keythemerc and the corresponding files (given > that the default values are available, possibly with an xfconf xml file?) Yeah. There is support for defaults in XfceShortcutsProvider already. All it needs is this (in a global representation of the "xfce4-keyboard-shortcuts" channel): <channel name="xfce4-keyboard-shortcuts" version="1.0"> <property name="providers" type="array"> <value type="string" value="xfwm4"/> </property> <property name="xfwm4" type="empty"> <property name="default" type="empty"> <property name="<Alt>Tab" value="cycle_windows_key"/> ... </property> </property> </channel> I'm not sure how to install these defaults yet though. Either via a script that calls xfconf-query or via one xfce4-keyboard-shortcuts.xml file installed by xfce4-settings. I suppose using xfconf-query is the better of the two ways because it's independent of the backend xfconf is using. Let's see what Brian has to say about this.
Hmm, not sure what to do here. Xfconfd doesn't have provisions for merging multiple system files into one channel (well, unless you have more than one system dir). Even if it did, there'd be a clash with the /providers property, and I'm not sure how to solve that.
Created attachment 1931 Fix compiler warnings Revised patch, fixed compiler warnings and remove support for keythemes all together.
(In reply to comment #11) > Hmm, not sure what to do here. Xfconfd doesn't have provisions for merging > multiple system files into one channel (well, unless you have more than one > system dir). Even if it did, there'd be a clash with the /providers property, > and I'm not sure how to solve that. As mentioned on IRC, the /providers property isn't mandatory. Providers always add themselves at first startup. Since there will be only very few command shortcuts (like for xfrun4, xfce4-about, help and a few more) we might as well ship the defaults with xfwm4 instead of xfce4-settings. Since a channel isn't bound to one single program (in fact, it makes a lot of sense to share a channel among several programs) it seems reasonable that more than one package wants to install defaults into the same channel.
(In reply to comment #11) > Hmm, not sure what to do here. Xfconfd doesn't have provisions for merging > multiple system files into one channel (well, unless you have more than one > system dir). Even if it did, there'd be a clash with the /providers property, > and I'm not sure how to solve that. I am not sure that merging is the issue here, I am fine with having the defaults provided by xfce4-settings (not perfect, but workable). Still, IMHO, xfconf really needs a way to set a bunch of default values at once (like GConf has, If I am not mistaken).
Actually, seeing as both xfce4-settings and xfwm4 depend on libxfce4kbd-private, it probably makes most sense to ship a combined defaults file with libxfcegui4. Olivier: what do you mean by setting a bunch of default values at once? Doesn't gconf just use schemas for that? That's more or less what our default .xml files do, except for the limitation on one file per channel, and that they're optional in our case.
(In reply to comment #15) > Olivier: what do you mean by setting a bunch of default values at once? > Doesn't gconf just use schemas for that? That's more or less what our default > .xml files do, except for the limitation on one file per channel, and that > they're optional in our case. Yes, absolutely, this is what I thought, but I eventually got confused by Jannis' comment about the XML file depending on the backend. So if the right solution is to put the combined XML file in libxfcegui4, then that's just fine with me.
(In reply to comment #16) > (In reply to comment #15) > > Olivier: what do you mean by setting a bunch of default values at once? > > Doesn't gconf just use schemas for that? That's more or less what our default > > .xml files do, except for the limitation on one file per channel, and that > > they're optional in our case. > > Yes, absolutely, this is what I thought, but I eventually got confused by > Jannis' comment about the XML file depending on the backend. > > So if the right solution is to put the combined XML file in libxfcegui4, then > that's just fine with me. Yeah, shipping it with libxfce4kbd-private/libxfcegui4 sounds like the best idea.
I have comitted the patch to xfwm4 in revision 28532. I've also added a default xfce4-keyboard-shortcuts.xml to libxfce4kbd-private in revision 28533. It already contains examples for default shortcuts and only needs to be filled now.
Jannis, did you commit the modified patch I attached? I setill the compilation errors that I fixed yesterday...
Jannis, did you commit the modified patch I attached? I still the compilation errors that I fixed yesterday...
(In reply to comment #20) > Jannis, did you commit the modified patch I attached? I still the compilation > errors that I fixed yesterday... I think I did, yes. But I'll check again if it's needed.
No worries, gcc is now happy with this commit: http://svn.xfce.org/index.cgi/xfce/revision?rev=28542 Please let me know if you spot something wrong.