GNOME Bugzilla – Bug 133318
Unnecessarily annoying when trying to define key that is already in use
Last modified: 2008-07-26 09:16:02 UTC
When I use the Gnome keybindings gizmo, and I try and use a key that is already in use, it tells me I can't. I then have to trawl through the list to see where it is already in use, delete it, then try again. Rather than telling me I can't, I'd prefer a dialog box that says something like "This key is already in use. Do you want to redefine this key? Yes/No". If you press Yes, it deletes the original binding.
Added GNOMEVER2.4, usability and bugsquad keywords. Set version to Gnome 2.4 as it is a relatively minor usability issue.
the current version tells which action is binded to the keybinding, do you think that's enough ?
Maybe allowing to override the already bound key would be useful.
Created attachment 109985 [details] [review] Allow shortcuts to be reassigned in the error dialog
Please use the -p parameter to diff. (svn diff doesn't support it, but google has instructions on how to enable it.) static void +write_key_to_gconf (KeyEntry *key_entry, ... + g_object_unref (G_OBJECT (client)); Unnecessary cast. + dialog = gtk_message_dialog_new (toplevel, + GTK_DIALOG_DESTROY_WITH_PARENT | GTK_DIALOG_MODAL, + GTK_MESSAGE_WARNING, + GTK_BUTTONS_OK, + _("Error setting new accelerator in configuration database: %s\n"), + err->message); "Accelerator"? What's that? Drop the newline. @@ -903,54 +939,54 @@ dialog = gtk_message_dialog_new (GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (view))), GTK_DIALOG_DESTROY_WITH_PARENT | GTK_DIALOG_MODAL, + GTK_MESSAGE_QUESTION, + GTK_BUTTONS_NONE, + "Reassign already used shortcut?"); + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), + _("The shortcut \"%s\" is currently used for the \"%s\" action. You may choose to use it for the \"%s\" action instead."), Not sure about this. The info what the shortcut is currently used for is rather important so using it as secondary text feels wrong. Additionally, without the secondary text, the primary text is not easily understood. Reassign to the old? To the new? + gtk_dialog_add_buttons (GTK_DIALOG (dialog), + "gtk-cancel", GTK_RESPONSE_CANCEL, + "Reassign", 0, Why not use a stock response id instead of 0 here? No shortcut for reassign? Not translatable? + if (response == 0) + { + write_key_to_gconf (&tmp_key, 0, 0, 0, + GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (view)))); ... + write_key_to_gconf (key_entry, keyval, mask, keycode, + GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (view)))); This is going to break. If the first write fails, we'll end up with two actions being bwound to the same shortcut.
(In reply to comment #5) > Please use the -p parameter to diff. (svn diff doesn't support it, but google > has instructions on how to enable it.) > > static void > +write_key_to_gconf (KeyEntry *key_entry, > ... > + g_object_unref (G_OBJECT (client)); > > Unnecessary cast. Removed in the updated patch that I'll attach now. > + dialog = gtk_message_dialog_new (toplevel, > + GTK_DIALOG_DESTROY_WITH_PARENT | > GTK_DIALOG_MODAL, > + GTK_MESSAGE_WARNING, > + GTK_BUTTONS_OK, > + _("Error setting new accelerator in > configuration database: %s\n"), > + err->message); > > "Accelerator"? What's that? Drop the newline. That message does not come from me, it was in the code before. I just left it as it was. > @@ -903,54 +939,54 @@ > dialog = > gtk_message_dialog_new (GTK_WINDOW (gtk_widget_get_toplevel > (GTK_WIDGET (view))), > GTK_DIALOG_DESTROY_WITH_PARENT | > GTK_DIALOG_MODAL, > + GTK_MESSAGE_QUESTION, > + GTK_BUTTONS_NONE, > + "Reassign already used shortcut?"); > + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), > + _("The shortcut \"%s\" is currently used for the \"%s\" action. You > may choose to use it for the \"%s\" action instead."), > > Not sure about this. The info what the shortcut is currently used for is rather > important so using it as secondary text feels wrong. Additionally, without the > secondary text, the primary text is not easily understood. Reassign to the old? > To the new? I changed the text a bit, making the old action part of the primary text. The secondary text now only explains the implications of using it for the new action anyway. > + gtk_dialog_add_buttons (GTK_DIALOG (dialog), > + "gtk-cancel", GTK_RESPONSE_CANCEL, > + "Reassign", 0, > > Why not use a stock response id instead of 0 here? No shortcut for reassign? > Not translatable? GTK_RESPONSE_ACCEPT is used now. The forgotten gettext call and mnemonic were pure sloppiness - they are added now. > + if (response == 0) > + { > + write_key_to_gconf (&tmp_key, 0, 0, 0, > + GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET > (view)))); > ... > + write_key_to_gconf (key_entry, keyval, mask, keycode, > + GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET > (view)))); > > This is going to break. If the first write fails, we'll end up with two actions > being bwound to the same shortcut. Now the key is only bound to the new action when unbinding it beforehand was successful.
Created attachment 110360 [details] [review] Updated key reassign patch See comment #6.
Created attachment 111878 [details] [review] Key reassign patch #3 Small tweaks as discussed with Jens Granseuer. Note that I couldn't test the newest version of the patch as I don't have the 2.23 dev libraries installed yet (patch is against trunk), but the changes should be sufficiently small to not break anything.
Created attachment 111879 [details] [review] Key reassign patch #4 Now with internationalization! (I should really get used to add those gettext macros...)
Can some dev look at this?
I discussed the patch with Denis quite a while ago, and he knows it's broken and still needs some love.
Created attachment 115236 [details] [review] Key reassign patch #5 Now using GConfChangeSet. I noticed though that the key reassigning does not seem to work with all shortcuts (gconf keys and accelerator labels are set just fine, but the shortcut just doesn't work; reassigning the same accelerator to that shortcut again fixes the problem). I suppose this is less of a problem with the patch than a bug in gnome-settings-daemon, though.
Created attachment 115295 [details] [review] Key reassign patch #6 Some changes as discussed with Jens Granseuer.
Committed a slightly revised version to fix the shortcuts sometimes not working. Thanks, Denis!