Bug 28257 – [Gtk] Doesn't support gtk-key-themes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Nic Ferrier]

using a few different webkitgtk browsers (midori, python webkitgtk) I've tried using the Emacs gtk-key-theme (set in ~/.gtkrc-2.0).

It doesn't work, tho it does work in gtk/gecko browsers.

I can confirm this bug.  Emacs bindings won't work with text fields in midori or uzbl, but work everywhere else.

[Randal Barlow]

Created attachment 45304 [details]
This patch binds the emacs text control shortcuts usually provided by the emacs gtk-key-theme, overriding the default shortcuts. A temporary solution for those of us who require these shortcuts now

The right way for this to be done is by checking the "gtk-key-theme-name" property on the screen's GtkSettings and choosing the associated KeyDownEnty for that gtk-key-theme in WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp

For now this patch will change the default text control keys to be something close to what is found in the Emacs gtk-key-theme. I guess it is for those of us who's fingers/minds are too mangled to change now.

[Randal Barlow]

Created attachment 45343 [details]
This patch that gets the default screen's GtkSettings and checks the "gtk-key-theme" property and applies one of two(Emacs, and the standard) themes

I suspect the correct solution will end up being something like this, but I am not familiar enough with any other key themes to make a entry for theme

[Randal Barlow]

Comment on attachment 45343 [details]
This patch that gets the default screen's GtkSettings and checks the "gtk-key-theme" property and applies one of two(Emacs, and the standard) themes

>diff -rupN original/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp new/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
>--- original/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp	2009-11-12 16:40:58.000000000 -0500
>+++ new/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp	2009-12-21 15:00:49.818888011 -0500
>@@ -31,6 +31,7 @@
> #include "FocusController.h"
> #include "Frame.h"
> #include <glib.h>
>+#include <gtk/gtk.h>
> #include "KeyboardCodes.h"
> #include "KeyboardEvent.h"
> #include "NotImplemented.h"
>@@ -371,7 +372,10 @@ struct KeyPressEntry {
>     const char* name;
> };
> 
>-static const KeyDownEntry keyDownEntries[] = {
>+/*
>+Standard gtk-key-theme
>+*/
>+static const KeyDownEntry stdkeyDownEntries[] = {
>     { VK_LEFT,   0,                  "MoveLeft"                                    },
>     { VK_LEFT,   ShiftKey,           "MoveLeftAndModifySelection"                  },
>     { VK_LEFT,   CtrlKey,            "MoveWordLeft"                                },
>@@ -417,6 +421,64 @@ static const KeyDownEntry keyDownEntries
>     { VK_RETURN, AltKey | ShiftKey,  "InsertNewline"                               },
> };
> 
>+/*
>+Emacs gtk-key-theme
>+*/
>+static const KeyDownEntry emacskeyDownEntries[] = {
>+    { VK_LEFT,   0,                  "MoveLeft"                                    },
>+    { VK_LEFT,   ShiftKey,           "MoveLeftAndModifySelection"                  },
>+    { VK_LEFT,   CtrlKey,            "MoveWordLeft"                                },
>+    { VK_LEFT,   CtrlKey | ShiftKey, "MoveWordLeftAndModifySelection"              },
>+    { VK_RIGHT,  0,                  "MoveRight"                                   },
>+    { VK_RIGHT,  ShiftKey,           "MoveRightAndModifySelection"                 },
>+    { VK_RIGHT,  CtrlKey,            "MoveWordRight"                               },
>+    { VK_RIGHT,  CtrlKey | ShiftKey, "MoveWordRightAndModifySelection"             },
>+    { VK_UP,     0,                  "MoveUp"                                      },
>+    { VK_UP,     ShiftKey,           "MoveUpAndModifySelection"                    },
>+    { VK_PRIOR,  ShiftKey,           "MovePageUpAndModifySelection"                },
>+    { VK_DOWN,   0,                  "MoveDown"                                    },
>+    { VK_DOWN,   ShiftKey,           "MoveDownAndModifySelection"                  },
>+    { VK_NEXT,   ShiftKey,           "MovePageDownAndModifySelection"              },
>+    { VK_PRIOR,  0,                  "MovePageUp"                                  },
>+    { VK_NEXT,   0,                  "MovePageDown"                                },
>+    { VK_HOME,   0,                  "MoveToBeginningOfLine"                       },
>+    { VK_HOME,   ShiftKey,           "MoveToBeginningOfLineAndModifySelection"     },
>+    { VK_HOME,   CtrlKey,            "MoveToBeginningOfDocument"                   },
>+    { VK_HOME,   CtrlKey | ShiftKey, "MoveToBeginningOfDocumentAndModifySelection" },
>+
>+    { VK_END,    0,                  "MoveToEndOfLine"                             },
>+    { VK_END,    ShiftKey,           "MoveToEndOfLineAndModifySelection"           },
>+    { VK_END,    CtrlKey,            "MoveToEndOfDocument"                         },
>+    { VK_END,    CtrlKey | ShiftKey, "MoveToEndOfDocumentAndModifySelection"       },
>+
>+    { VK_BACK,   0,                  "DeleteBackward"                              },
>+    { VK_BACK,   ShiftKey,           "DeleteBackward"                              },
>+    { VK_DELETE, 0,                  "DeleteForward"                               },
>+    { VK_BACK,   CtrlKey,            "DeleteWordBackward"                          },
>+    { VK_DELETE, CtrlKey,            "DeleteWordForward"                           },
>+
>+    { 'B',       CtrlKey,            "ToggleBold"                                  },
>+    { 'I',       CtrlKey,            "ToggleItalic"                                },
>+
>+    { VK_ESCAPE, 0,                  "Cancel"                                      },
>+    { VK_OEM_PERIOD, CtrlKey,        "Cancel"                                      },
>+    { VK_TAB,    0,                  "InsertTab"                                   },
>+    { VK_TAB,    ShiftKey,           "InsertBacktab"                               },
>+    { VK_RETURN, 0,                  "InsertNewline"                               },
>+    { VK_RETURN, CtrlKey,            "InsertNewline"                               },
>+    { VK_RETURN, AltKey,             "InsertNewline"                               },
>+    { VK_RETURN, AltKey | ShiftKey,  "InsertNewline"                               },
>+    // Emacs key theme text control
>+    { VK_K,      CtrlKey,            "DeleteToEndOfLine"                           },
>+    { VK_F,      CtrlKey,            "MoveForward"                                 },
>+    { VK_B,      CtrlKey,            "MoveBackward"                                },
>+    { VK_W,      CtrlKey,            "DeleteWordBackward"                          },
>+    { VK_D,      CtrlKey,            "DeleteForward"                               },
>+    { VK_A,      CtrlKey,            "MoveToBeginningOfLine"                       },
>+    { VK_E,      CtrlKey,            "MoveToEndOfLine"                             },
>+};
>+
>+
> static const KeyPressEntry keyPressEntries[] = {
>     { '\t',   0,                  "InsertTab"                                   },
>     { '\t',   ShiftKey,           "InsertBacktab"                               },
>@@ -436,9 +498,22 @@ static const char* interpretEditorComman
>     if (!keyDownCommandsMap) {
>         keyDownCommandsMap = new HashMap<int, const char*>;
>         keyPressCommandsMap = new HashMap<int, const char*>;
>-
>-        for (unsigned i = 0; i < G_N_ELEMENTS(keyDownEntries); i++)
>-            keyDownCommandsMap->set(keyDownEntries[i].modifiers << 16 | keyDownEntries[i].virtualKey, keyDownEntries[i].name);
>+        
>+        // get the gtk key theme
>+        GtkSettings* settings = gtk_settings_get_default();
>+        gchar* key_theme_name;
>+        g_object_get(settings, "gtk-key-theme-name", &key_theme_name, NULL);
>+
>+        if (strcmp(key_theme_name, "Emacs") == 0)
>+        {
>+            for (unsigned i = 0; i < G_N_ELEMENTS(emacskeyDownEntries); i++)
>+                keyDownCommandsMap->set(emacskeyDownEntries[i].modifiers << 16 | emacskeyDownEntries[i].virtualKey, emacskeyDownEntries[i].name);
>+        }
>+        else
>+        {
>+            for (unsigned i = 0; i < G_N_ELEMENTS(stdkeyDownEntries); i++)
>+                keyDownCommandsMap->set(stdkeyDownEntries[i].modifiers << 16 | stdkeyDownEntries[i].virtualKey, stdkeyDownEntries[i].name);
>+        }
> 
>         for (unsigned i = 0; i < G_N_ELEMENTS(keyPressEntries); i++)
>             keyPressCommandsMap->set(keyPressEntries[i].modifiers << 16 | keyPressEntries[i].charCode, keyPressEntries[i].name);

[Randal Barlow]

Created attachment 45354 [details]
This patch like the last one gets the default screen's GtkSettings and checks the "gtk-key-theme" property and applies one of two(Emacs, and the standard) themes. Corrected a few bindings.

The last patch had Control-W incorrectly bound to DeleteWordForward not Backward

Sorry about my last comment, I incorrectly edited the bug thinking I could edit the attachment

[arno.]

Hi,
I noticed a few missing bindings:

Ctrl-Shift-B MoveBackwardAndModifySelection
Alt-Shift-B MoveWordBackwardAndModifySelection
Alt-B   MoveWordBackward
Ctrl-Shift-F MoveForwardAndModifySelection
Alt-Shift-F MoveWordForwardAndModifySelection
Alt-F MoveWordForward
Ctrl-Shift-A  MoveWordBackwardAndModifySelection
Ctrl-Shift-E  MoveToEndOfLineAndModifySelection
Alt-D       DeleteWordForward
Ctrl-K  DeleteToEndOfParagraph

and also Ctrl-U: deletes full line. I didn't find a command for that.

Those bindings are also used in GtkTextView but not GtkEntry (ie: textareas in Gecko)
Ctrl-P: move to previous line
Ctrl-Shift-P: move to previous line and select
Ctrl-N: move to next line
Ctrl-Shift-N: move to next line and select


But I see one problem with that method: if users have customized default gtk emacs bindings, or if people use another binding scheme, it will not work. Is is possible to get the GtkBindingSet for a GtkEntry or GtkTextView element, and traverse all key bindings to ultimately, get a binding keyval and modifier, and also the name of the signal it will invoke with its arguments. Therefore, it's theorically possible to use bindings defined for real for GtkEntry instead of hardcoding default one. Do you think that's a good idea to do that ?

[Christian Dywan]

Hardcoding any key themes is definitely wrong. The very idea of key bindings in GTK+ is that people can  customize them, and they do. For example Vi-like bindings, which are not shipped with GTK+.
The correct way is to either implement bindings in the widget instead of doing our own thing, or read the bindings and fill WebCore's internal key definitions with that.

[Martin Robinson]

Created attachment 55072 [details]
Patch which defers to a native text widget

I've added a patch which follows the same approach as Chromium and Gecko. Essentially a key events are send to a native widget and editing commands are intercepted and handled appropriately. The patch should read all GTK+ theme settings properly.

[WebKit Review Bot]

Attachment 55072 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:98:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:104:  One space before end of line comments  [whitespace/comments] [5]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:124:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:129:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:163:  One space before end of line comments  [whitespace/comments] [5]
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 14 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 55073 [details]
Previous patch + style fixes

[WebKit Review Bot]

Attachment 55073 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.h:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 55076 [details]
Patch with style fix

Gah. Uploaded the incorrect version of the patch.

[Martin Robinson]

Created attachment 55716 [details]
Patch with fixes for copy and paste

[Xan Lopez]

Comment on attachment 55716 [details]
Patch with fixes for copy and paste

Looks good to me, it would be good to have some tests for it eventually though.

[Martin Robinson]

Committed r59158: <http://trac.webkit.org/changeset/59158>

[Martin Robinson]

Comment on attachment 55716 [details]
Patch with fixes for copy and paste

Patch has been comitted. Clearing flags.