Unify key binding implementations #6140

bhennion · 2024-12-07T12:00:26Z

The long term goal (for a follow up PR) is to implement configurable shortcuts.

This PR simply uses the KeyBinding architecture used in TextEditor for key bindings various locations in the code.

It does not mean all of those key bindings should be open to configuration, but it gives us a common base to work with.

bhennion · 2024-12-09T18:22:48Z

In order to get the right implementation, I've been giving some thoughts to shortcuts customization. I need some opinions as to what key binding should be configurable (Note that not all keybindings are "shortcuts" like Ctrl+Z. Some are just "when the user presses Ctrl" or things like that).
To help with the discussion, I put a list of current key bindings defined in the code (it does not contain those defined in mainmenubar.xml) at the bottom of this post.

Now, I think we want everything defined in mainmenubar.xml to be configurable - or possibly even every action in Action.enum.h should be able to be triggered by a configurable shortcut.
But there are other places.
If we look for instance at the key bindings defined in XournalView, some non-standard shortcuts (from vim) have been hardcoded. We should definitely not have hardcoded those but it shows there is a demand for customization outside of the main menubar's entries.

IMO, the shortcut customization dialog should be aware of almost all key bindings accross the app: it should be able to detect conflicts.

The other thing to consider is: if a key binding is customizable, then it improves its discoverability (just because it has to be written down in the customization dialog). I don't think a lot of users know about the non-trivial key bindings of the geometry tools/spline handler/shape handlers/vertical tool.

My feeling is that:

Key bindings defined in mainmenubar.xml and in control/actions/ActionProperties.h should be configurable. The user should be able to add/modify shortcuts for most (if not all) of the actions in the ActionDatabase (with a choice of parameter if the GAction has a parameter)
Key bindings in BaseShapeHandler could be configurable: improves discoverability
Key bindings in SplineHandler should be configurable: allows to reassign "single key shortcuts" to other uses (see e.g. Single key shortcuts ? #6115) + improves discoverability
Key bindings in VerticalToolHandler could be configurable (improves discoverability)
Key bindings in EditSelection: I don't know. Maybe not configurable
Key bindings in TextEditor shoud not be configurable: the keyboard input is too important to let users meddle with it here (like introducing a single key shortcut for instance...).
Key bindings in GeometryToolInputHandler should be configurable: allows to reassign "single key shortcuts" to other uses (see e.g. Single key shortcuts ? #6115) + improves discoverability
Key bindings in SearchBar should not be configurable
Key bindings in XournalView: I would have said "don't allow for customization" but as I mentionned above, contributors have already customized them (by adding vim-like navigation). So maybe yes, or maybe partially, or maybe the navigation things should be triggered via the ActionDatabase and the color things should be configurable

So, @xournalpp/core do you have any opinion, insights or comments about those questions?

List of current keybindings:

Key bindings in BaseShapeHandler

    if (event.keyval == GDK_KEY_Shift_L || event.keyval == GDK_KEY_Shift_R) {
59:    } else if (event.keyval == GDK_KEY_Control_L || event.keyval == GDK_KEY_Control_R) {
61:    } else if (event.keyval == GDK_KEY_Alt_L || event.keyval == GDK_KEY_Alt_R) {

Key bindings in SplineHandler

61:        case GDK_KEY_BackSpace: {
72:        case GDK_KEY_Right: {
77:        case GDK_KEY_Left: {
82:        case GDK_KEY_Up: {
87:        case GDK_KEY_Down: {
92:        case GDK_KEY_r:
93:        case GDK_KEY_R: {  // r like "rotate"
94:            double angle = (event.keyval == GDK_KEY_R) ? -ROTATE_AMOUNT : ROTATE_AMOUNT;
103:        case GDK_KEY_s:
104:        case GDK_KEY_S: {  // s like "scale"
109:            if (event.keyval == GDK_KEY_S) {
131:    if (event.keyval == GDK_KEY_Escape) {

Key bindings in VerticalToolHandler

71:    if ((event.keyval == GDK_KEY_Control_L || event.keyval == GDK_KEY_Control_R) && this->spacingSide == Side::Below) {
80:    if ((event.keyval == GDK_KEY_Control_L || event.keyval == GDK_KEY_Control_R) && this->spacingSide == Side::Above) {

Key bindings in EditSelection

1255:            {{{NONE,  GDK_KEY_Left},    moveAndKeepVisible<-REGULAR_MOVE_AMOUNT,                    0>},
1256:             {{ALT,   GDK_KEY_Left},    moveAndKeepVisible<  -SMALL_MOVE_AMOUNT,                    0>},
1257:             {{SHIFT, GDK_KEY_Left},    moveAndKeepVisible<  -LARGE_MOVE_AMOUNT,                    0>},
1258:             {{NONE,  GDK_KEY_Right},   moveAndKeepVisible< REGULAR_MOVE_AMOUNT,                    0>},
1259:             {{ALT,   GDK_KEY_Right},   moveAndKeepVisible<   SMALL_MOVE_AMOUNT,                    0>},
1260:             {{SHIFT, GDK_KEY_Right},   moveAndKeepVisible<   LARGE_MOVE_AMOUNT,                    0>},
1261:             {{NONE,  GDK_KEY_Up},      moveAndKeepVisible<                   0, -REGULAR_MOVE_AMOUNT>},
1262:             {{ALT,   GDK_KEY_Up},      moveAndKeepVisible<                   0,   -SMALL_MOVE_AMOUNT>},
1263:             {{SHIFT, GDK_KEY_Up},      moveAndKeepVisible<                   0,   -LARGE_MOVE_AMOUNT>},
1264:             {{NONE,  GDK_KEY_Down},    moveAndKeepVisible<                   0,  REGULAR_MOVE_AMOUNT>},
1265:             {{ALT,   GDK_KEY_Down},    moveAndKeepVisible<                   0,    SMALL_MOVE_AMOUNT>},
1266:             {{SHIFT, GDK_KEY_Down},    moveAndKeepVisible<                   0,    LARGE_MOVE_AMOUNT>},
1267:             {{NONE,  GDK_KEY_Escape},  clear}});

Key bindings in TextEditor

21:    {{mod, GDK_KEY_##key}, wrap<&TextEditor::moveCursor, mvt, dir, false>},                \
22:            {{mod & SHIFT, GDK_KEY_##key}, wrap<&TextEditor::moveCursor, mvt, dir, true>}, \
23:            {{mod, GDK_KEY_KP_##key}, wrap<&TextEditor::moveCursor, mvt, dir, false>}, {   \
24:        {mod& SHIFT, GDK_KEY_KP_##key}, wrap<&TextEditor::moveCursor, mvt, dir, true>      \
48:         {{CTRL, GDK_KEY_a}, wrap<&TextEditor::selectAtCursor, TextEditor::SelectType::ALL>},
49:         {{CTRL, GDK_KEY_slash}, wrap<&TextEditor::selectAtCursor, TextEditor::SelectType::ALL>},
51:         {{NONE, GDK_KEY_Delete}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_CHARS, 1>},
52:         {{NONE, GDK_KEY_KP_Delete}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_CHARS, 1>},
54:         {{NONE, GDK_KEY_BackSpace}, wrap<&TextEditor::backspace>},
55:         {{SHIFT, GDK_KEY_BackSpace}, wrap<&TextEditor::backspace>},
57:         {{CTRL, GDK_KEY_Delete}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_WORD_ENDS, 1>},
58:         {{CTRL, GDK_KEY_KP_Delete}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_WORD_ENDS, 1>},
59:         {{CTRL, GDK_KEY_BackSpace}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_WORD_ENDS, -1>},
61:         {{CTRL & SHIFT, GDK_KEY_Delete}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_PARAGRAPH_ENDS, 1>},
62:         {{CTRL & SHIFT, GDK_KEY_KP_Delete}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_PARAGRAPH_ENDS, 1>},
63:         {{CTRL & SHIFT, GDK_KEY_BackSpace}, wrap<&TextEditor::deleteFromCursor, GTK_DELETE_PARAGRAPH_ENDS, -1>},
65:         {{CTRL, GDK_KEY_x}, wrap<&TextEditor::cutToClipboard>},
66:         {{SHIFT, GDK_KEY_Delete}, wrap<&TextEditor::cutToClipboard>},
68:         {{CTRL, GDK_KEY_c}, wrap<&TextEditor::copyToClipboard>},
69:         {{CTRL, GDK_KEY_Insert}, wrap<&TextEditor::copyToClipboard>},
71:         {{CTRL, GDK_KEY_v}, wrap<&TextEditor::pasteFromClipboard>},
72:         {{SHIFT, GDK_KEY_Insert}, wrap<&TextEditor::pasteFromClipboard>},
74:         {{NONE, GDK_KEY_Insert}, wrap<&TextEditor::toggleOverwrite>},
75:         {{NONE, GDK_KEY_KP_Insert}, wrap<&TextEditor::toggleOverwrite>},
77:         {{CTRL, GDK_KEY_b}, wrap<&TextEditor::toggleBoldFace>},
78:         {{CTRL, GDK_KEY_B}, wrap<&TextEditor::toggleBoldFace>},
80:         {{CTRL, GDK_KEY_plus}, wrap<&TextEditor::increaseFontSize>},
81:   
8000
      {{CTRL, GDK_KEY_KP_Add}, wrap<&TextEditor::increaseFontSize>},
83:         {{CTRL, GDK_KEY_minus}, wrap<&TextEditor::decreaseFontSize>},
84:         {{CTRL, GDK_KEY_KP_Subtract}, wrap<&TextEditor::decreaseFontSize>},
86:         {{NONE, GDK_KEY_Return}, wrap<&TextEditor::linebreak>},
87:         {{NONE, GDK_KEY_ISO_Enter}, wrap<&TextEditor::linebreak>},
88:         {{NONE, GDK_KEY_KP_Enter}, wrap<&TextEditor::linebreak>},
90:         {{NONE, GDK_KEY_Tab}, wrap<&TextEditor::tabulation>},
91:         {{NONE, GDK_KEY_KP_Tab}, wrap<&TextEditor::tabulation>},
92:         {{NONE, GDK_KEY_ISO_Left_Tab}, wrap<&TextEditor::tabulation>},
94:         {{SHIFT, GDK_KEY_Tab}, wrap<&TextEditor::tabulation>},
95:         {{SHIFT, GDK_KEY_KP_Tab}, wrap<&TextEditor::tabulation>},
96:         {{SHIFT, GDK_KEY_ISO_Left_Tab}, wrap<&TextEditor::tabulation>}});

Key bindings in GeometryToolInputHandler

140:        case GDK_KEY_Left:
143:        case GDK_KEY_Up:
146:        case GDK_KEY_Right:
149:        case GDK_KEY_Down:
152:        case GDK_KEY_r:
155:        case GDK_KEY_R:  // r like "rotate"
158:        case GDK_KEY_s:
161:        case GDK_KEY_S:
164:        case GDK_KEY_m:

Key bindings in SearchBar

37:                         if (event->keyval == GDK_KEY_Return) {
46:                         } else if (event->keyval == GDK_KEY_Escape) {

Key bindings in XournalView

31:        {{NONE, GDK_KEY_Page_Down},    wrap<&ScrollHandler::goToNextPage>},
32:        {{NONE, GDK_KEY_KP_Page_Down}, wrap<&ScrollHandler::goToNextPage>},
33:        {{NONE, GDK_KEY_Page_Up},      wrap<&ScrollHandler::goToPreviousPage>},
34:        {{NONE, GDK_KEY_KP_Page_Up},   wrap<&ScrollHandler::goToPreviousPage>},
35:        {{NONE, GDK_KEY_space},        wrap<&ScrollHandler::scrollByVisibleArea, ScrollHandler::DOWN>},
36:        {{SHIFT, GDK_KEY_space},       wrap<&ScrollHandler::scrollByVisibleArea, ScrollHandler::UP>},
38:        {{SHIFT, GDK_KEY_KP_Up},       wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::UP>},
39:        {{SHIFT, GDK_KEY_Up},          wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::UP>},
40:        {{SHIFT, GDK_KEY_k},           wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::UP>},  // Why??
41:        {{SHIFT, GDK_KEY_K},           wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::UP>},  // Why??
42:        {{SHIFT, GDK_KEY_KP_Down},     wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::DOWN>},
43:        {{SHIFT, GDK_KEY_Down},        wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::DOWN>},
44:        {{SHIFT, GDK_KEY_j},           wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::DOWN>},  // Why??
45:        {{SHIFT, GDK_KEY_J},           wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::DOWN>},  // Why??
46:        {{SHIFT, GDK_KEY_KP_Left},     wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::LEFT>},
47:        {{SHIFT, GDK_KEY_Left},        wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::LEFT>},
48:        {{SHIFT, GDK_KEY_h},           wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::LEFT>},  // Why??
49:        {{SHIFT, GDK_KEY_KP_Right},    wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::RIGHT>},
50:        {{SHIFT, GDK_KEY_Right},       wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::RIGHT>},
51:        {{SHIFT, GDK_KEY_l},           wrap<&ScrollHandler::scrollByOnePage, ScrollHandler::RIGHT>},  // Why??
53:        {{NONE, GDK_KEY_KP_Up},        wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::UP>},
54:        {{NONE, GDK_KEY_Up},           wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::UP>},
55:        {{NONE, GDK_KEY_k},            wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::UP>},  // Why??
56:        {{NONE, GDK_KEY_K},            wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::UP>},  // Why??
57:        {{NONE, GDK_KEY_KP_Down},      wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::DOWN>},
58:        {{NONE, GDK_KEY_Down},         wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::DOWN>},
59:        {{NONE, GDK_KEY_j},            wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::DOWN>},  // Why??
60:        {{NONE, GDK_KEY_J},            wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::DOWN>},  // Why??
61:        {{NONE, GDK_KEY_KP_Left},      wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::LEFT>},
62:        {{NONE, GDK_KEY_Left},         wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::LEFT>},
63:        {{NONE, GDK_KEY_h},            wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::LEFT>},  // Why??
64:        {{NONE, GDK_KEY_KP_Right},     wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::RIGHT>},
65:        {{NONE, GDK_KEY_Right},        wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::RIGHT>},
66:        {{NONE, GDK_KEY_l},            wrap<&ScrollHandler::scrollByOneStep, ScrollHandler::RIGHT>},  // Why??
68:        {{NONE, GDK_KEY_End},          wrap<&ScrollHandler::goToLastPage>},
69:        {{NONE, GDK_KEY_KP_End},       wrap<&ScrollHandler::goToLastPage>},
70:        {{NONE, GDK_KEY_Home},         wrap<&ScrollHandler::goToFirstPage>},
71:        {{NONE, GDK_KEY_KP_Home},      wrap<&ScrollHandler::goToFirstPage>}
86:        {{NONE, GDK_KEY_1}, setColorByNumber<0>},
87:        {{NONE, GDK_KEY_2}, setColorByNumber<1>},
88:        {{NONE, GDK_KEY_3}, setColorByNumber<2>},
89:        {{NONE, GDK_KEY_4}, setColorByNumber<3>},
90:        {{NONE, GDK_KEY_5}, setColorByNumber<4>},
91:        {{NONE, GDK_KEY_6}, setColorByNumber<5>},
92:        {{NONE, GDK_KEY_7}, setColorByNumber<6>},
93:        {{NONE, GDK_KEY_8}, setColorByNumber<7>},
94:        {{NONE, GDK_KEY_9}, setColorByNumber<8>},
95:        {{NONE, GDK_KEY_0}, setColorByNumber<9>},

rolandlo · 2024-12-10T04:37:32Z

I haven't made up my mind yet, about which shortcuts/keypresses should be configurable yet. But about discoverability let me add that there is another way to help with that: the GtkShortcutsWindow (which is present both in Gtk3 and Gtk4). It's just used for presenting shortcuts/keypresses, not for modifying them.
In the Libadwaita redesign mockup I have added a GtkShortcutsWindow with all shortcuts I could find. For example here is the section about the geometry tools:

bhennion · 2024-12-11T08:16:09Z

I haven't made up my mind yet, about which shortcuts/keypresses should be configurable yet. But about discoverability let me add that there is another way to help with that: the GtkShortcutsWindow (which is present both in Gtk3 and Gtk4).

That's a good point, but I am not quite sure how usable it is with configurable shortcuts (even just for displaying them). It seems that GtkShortcutsShortcut/GtkShortcutsSection/GtkShortcutsWindow have no public constructor. The only way to build them is via a GtkBuilder and a UI file. It makes it complicated to generate a GtkShortcutsWindow dynamically.

bhennion · 2024-12-11T08:57:43Z

About what should be configurable or not: we will probably want to have some immutable shortcuts that the user cannot override, while allowing the user to add another shortcut for the same action. I have in mind navigation key bindings for instance.

rolandlo · 2024-12-11T09:07:27Z

I haven't made up my mind yet, about which shortcuts/keypresses should be configurable yet. But about discoverability let me add that there is another way to help with that: the GtkShortcutsWindow (which is present both in Gtk3 and Gtk4).

That's a good point, but I am not quite sure how usable it is with configurable shortcuts (even just for displaying them). It seems that GtkShortcutsShortcut/GtkShortcutsSection/GtkShortcutsWindow have no public constructor. The only way to build them is via a GtkBuilder and a UI file. It makes it complicated to generate a GtkShortcutsWindow dynamically.

Apparently that changed in Gtk 4.14 though with new API
gtk_shortcuts_window_add_section
gtk_shortcuts_section_add_group
gtk_shortcuts_group_add_shortcut , see
https://docs.gtk.org/gtk4/class.ShortcutsSection.html?q=shortcuts

It's unclear to me however how to construct a GtkShortcutsShortcut though. In Python I can do Gtk.ShortcutsShortcut() to construct one, but I don't see how to set its properties.

rolandlo · 2024-12-11T10:14:40Z

I haven't made up my mind yet, about which shortcuts/keypresses should be configurable yet. But about discoverability let me add that there is another way to help with that: the GtkShortcutsWindow (which is present both in Gtk3 and Gtk4).

That's a good point, but I am not quite sure how usable it is with configurable shortcuts (even just for displaying them). It seems that GtkShortcutsShortcut/GtkShortcutsSection/GtkShortcutsWindow have no public constructor. The only way to build them is via a GtkBuilder and a UI file. It makes it complicated to generate a GtkShortcutsWindow dynamically.

Apparently that changed in Gtk 4.14 though with new API gtk_shortcuts_window_add_section gtk_shortcuts_section_add_group gtk_shortcuts_group_add_shortcut , see https://docs.gtk.org/gtk4/class.ShortcutsSection.html?q=shortcuts

It's unclear to me however how to construct a GtkShortcutsShortcut though. In Python I can do Gtk.ShortcutsShortcut() to construct one, but I don't see how to set its properties.

I see how this is supposed to be done now (in Python). This works in the Libadwaita redesign mockup:

        shortcut_window = win.get_help_overlay()
        section = Gtk.ShortcutsSection()
        section.props.title = "My new section"
        shortcut_window.add_section(section)
        group = Gtk.ShortcutsGroup()
        group.props.title = "My new group"
        section.add_group(group)
        shortcut = Gtk.ShortcutsShortcut()
        shortcut.props.accelerator = "<Ctrl><Shift>j"
        shortcut.props.title = "My new shortcut"
        group.add_shortcut(shortcut)

bhennion · 2024-12-11T16:07:23Z

I see how this is supposed to be done now (in Python). This works in the Libadwaita redesign mockup:

The C/C++ version of that is probably along the lines

GtkShortcutsShortcut* shortcut = GTK_SHORTCUTS_SHORTCUT(g_object_new(GTK_TYPE_SHORTCUTS_SHORTCUT, "accelerator", "<Ctrl><Shift>j", "title", "My name", nullptr));

In any case, this isn't for now as we need GTK 4.14.

This reverts commit b089023.

…025)

bhennion · 2025-01-30T19:16:37Z

\action create-installers debian ubuntu-22

github-actions · 2025-01-30T19:16:50Z

Started 2 build(s) (see details)

github-actions · 2025-01-30T19:21:42Z

CI successful!
Available artifacts:

bhennion force-pushed the pr/shortcuts branch from 40399cc to 0b4d1af Compare December 14, 2024 15:34

bhennion added 10 commits January 30, 2025 18:07

Unify key binding implementation in XournalView

c73e2e6

Add Navigation actions

d68cfe4

Add contructor to KeyEvent

2540984

Shortcuts for menu entries are defined in ShortcutConfiguration

c8063ab

same, better

f47d0c3

same, better better

7092133

Revert "Add Navigation actions"

3e11a30

This reverts commit b089023.

fixup

a4abdd6

Remove Ubuntu 20 target (Deprecated CI image - retired on April 1st 2…

e1272fb

…025)

fixup

e8c92ad

bhennion force-pushed the pr/shortcuts branch from f5eb964 to e8c92ad Compare January 30, 2025 18:28

bhennion added 3 commits January 30, 2025 19:39

Workaround if glib<2.80

fa44c7f

Workaround if glib<2.82

41a6f28

Bump glib version requirement

699ff91

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Unify key binding implementations #6140

Unify key binding implementations #6140

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Unify key binding implementations #6140

Are you sure you want to change the base?

Unify key binding implementations #6140

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!