Add support for a second keybind per action #870

rcalixte · 2024-01-11T19:27:41Z

Modifying code:

terminatorlib/config.py
terminatorlib/keybindings.py
terminatorlib/plugin.py
terminatorlib/preferences.glade
terminatorlib/prefseditor.py
terminatorlib/terminal_popup_menu.py
terminatorlib/window.py

Modifying tests:

tests/test_prefseditor_keybindings.py

Closes #371

NOTES

This can be expanded to three bindings per action if needed but I thought two would be sufficient.
Whitespace was cleaned up in the modified files in addition to addenda below.

ADDENDA

Added missing parameters to base definitions
Removed unnecessary parentheses from return and del statements
Changed comparisons from == True to is True (unsure if test conditions were meant to be truthy or not)
Formatted regular strings that could be f-strings
Various other code quality cleanups

cc @mattrose This will probably be easier to review with whitespace changes hidden. For the translation files, I just ran both scripts to pick up the new strings. I've tested this locally off of the master branch as well. For configs in the "old"/current format, there are config parsing warnings at startup (only visible if you launch from a terminal), but nothing visible otherwise to the user and nothing that impacts usability. Once any setting is modified, the new format for bindings will be written to the config and the warnings won't be seen again. So users who don't make any settings changes after upgrading won't see anything change in their experience. For the context menu, I opted to show the first binding found only, in the event more than one binding is configured for an action. A warning dialog is also shown if there is an attempt to set an existing binding to a new action or the same action twice. Feel free to let me know if any additional changes are needed in order to get this merged and released.

Question:
Should the workflows be updated for python 3.10 and 3.11? They are currently 3.8 and 3.9. I tested 3.10 and 3.11 on my local fork without issue so it should be safe to make that change. (Adding 3.12 would require working around the removal of packages like setuptools and I reason that isn't on the roadmap yet which is why I excluded it.)

(Side note: I'd only recently learned about Terminator and it's literally been the biggest boon for my productivity of late, immediately becoming my default on every machine. Thanks to you and the team for resurrecting and maintaining it!)

mattrose · 2024-01-13T14:29:51Z

Wow, there's a lot here, I'll take a look as soon as I can.

mattrose · 2024-02-17T14:36:28Z

So, there's so much here that it's impossible for me to do a review. 70,000 changes is way too big for a code review. The idea is good, but it looks like you went through it with a code formatter or linter that made a bunch of irrelevant changes. That's fine as it is, but it makes it impossible for me to easily review the changes that are necessary to the feature, and the output of the linter. Also, the doc changes should probably be done in a separate PR as well, so that we can more easily tease those out.

If you could make another PR with just the changes that are necessary to implement the feature, I could definitely review that and merge it in, with some work, but as it is, I can't really review or merge this PR.

rcalixte · 2024-02-17T15:07:30Z

If you could make another PR with just the changes that are necessary to implement the feature, I could definitely review that and merge it in, with some work, but as it is, I can't really review or merge this PR.

I mentioned that it would be easier to review with GitHub's whitespace option for that reason. The line change count is also largely due to the modified translation files due to the changed strings. I can pull those and the PR should be easier to review but I do think proper translation support should be implemented at some point in time.

Ref: gnome-terminator#870

rcalixte · 2024-02-17T19:17:18Z

Question:
Should the workflows be updated for python 3.10 and 3.11? They are currently 3.8 and 3.9. I tested 3.10 and 3.11 on my local fork without issue so it should be safe to make that change. (Adding 3.12 would require working around the removal of packages like setuptools and I reason that isn't on the roadmap yet which is why I excluded it.)

Should I open the PRs for this too? We could add 3.10 and 3.11 while dropping 3.8. (I don't know of any distros that ship 3.8 and an updated terminator package.)

mattrose · 2024-02-18T03:38:42Z

terminatorlib/keybindings.py

 keyboard shortcuts.

 """

 import re
-from gi.repository import Gtk, Gdk
+from gi.repository import Gdk


Why remove GTK here? Is it necessary?

It's an unused import here. There was one call that was commented out.

Should we remove the comment too?

I'd rather keep code cleanup changes separate from feature or bugfix changes. It means when I go back theough the history I can tell exactly what changes did what. This is the point I was trying to get across with my earlier comment. There's too much going on in this commit. The stuff may be good to include in another PR, but in a PR that is just supposed to implement a feature, the code should just reflect what is necessary to implement that feature. I definitely welcome bugfixes and code cleanup PRs, just not mixed in with feature implementation PRs

mattrose

I'd rather keep code cleanup changes separate from feature or bugfix changes. It means when I go back theough the history I can tell exactly what changes did what. This is the point I was trying to get across with my earlier comment. There's too much going on in this commit. The stuff may be good to include in another PR, but in a PR that is just supposed to implement a feature, the code should just reflect what is necessary to implement that feature. I definitely welcome bugfixes and code cleanup PRs, just not mixed in with feature implementation PRs

mattrose · 2024-02-18T03:40:56Z

terminatorlib/keybindings.py

@@ -91,7 +95,7 @@ def reload(self):
                    self._lookup[mask][keyval] = action
                    self._masks |= mask

-    def _parsebinding(self, binding):


Why change this method name?

The underscore kept it as an internal method. This affected the tests from being able to access the method.

mattrose · 2024-02-18T03:42:02Z

terminatorlib/keybindings.py

@@ -103,27 +107,25 @@ def _parsebinding(self, binding):
            raise KeymapError('No key found')
        keyval = Gdk.keyval_from_name(key)
        if keyval == 0:
-            raise KeymapError("Key '%s' is unrecognised" % key)
+            raise KeymapError(f"Key '{key}' is unrecognised")


Why are you changing this string formatting?

F-strings have been preferred since Python 3.6 went GA.

mattrose · 2024-02-18T03:42:16Z

terminatorlib/keybindings.py

@@ -103,27 +107,25 @@ def _parsebinding(self, binding):
            raise KeymapError('No key found')
        keyval = Gdk.keyval_from_name(key)
        if keyval == 0:
-            raise KeymapError("Key '%s' is unrecognised" % key)


Why are you changing this string formatting?

F-strings have been preferred since Python 3.6 went GA.

mattrose · 2024-02-18T03:42:32Z

terminatorlib/keybindings.py

        return (keyval, mask)

    def _lookup_modifier(self, modifier):
        """Map modifier names to gtk values"""
        try:
            return self.modifiers[modifier.lower()]
-        except KeyError:
-            raise KeymapError("Unhandled modifier '<%s>'" % modifier)
+        except KeyError as exc:


Why are you changing this string formatting?

mattrose · 2024-02-18T03:52:03Z

terminatorlib/keybindings.py

 keyboard shortcuts.

 """

 import re
-from gi.repository import Gtk, Gdk
+from gi.repository import Gdk


I'd rather keep code cleanup changes separate from feature or bugfix changes. It means when I go back theough the history I can tell exactly what changes did what. This is the point I was trying to get across with my earlier comment. There's too much going on in this commit. The stuff may be good to include in another PR, but in a PR that is just supposed to implement a feature, the code should just reflect what is necessary to implement that feature. I definitely welcome bugfixes and code cleanup PRs, just not mixed in with feature implementation PRs

Ref: gnome-terminator#870

Modifying code: - terminatorlib/config.py - terminatorlib/keybindings.py - terminatorlib/plugin.py - terminatorlib/preferences.glade - terminatorlib/prefseditor.py - terminatorlib/terminal_popup_menu.py - terminatorlib/window.py Modifying test: - tests/test_prefseditor_keybindings.py Closes gnome-terminator#371

Ref: gnome-terminator#870

rcalixte force-pushed the multiple_keybinds branch 13 times, most recently from e392cfb to 961b5a0 Compare January 13, 2024 00:08

rcalixte force-pushed the multiple_keybinds branch from 961b5a0 to a36a3ff Compare January 27, 2024 18:40

rcalixte force-pushed the multiple_keybinds branch 9 times, most recently from bf09d79 to 4f5fba1 Compare February 11, 2024 21:01

rcalixte force-pushed the multiple_keybinds branch 3 times, most recently from 06228fa to f191937 Compare February 16, 2024 18:13

rcalixte force-pushed the multiple_keybinds branch from f191937 to a7c7073 Compare February 17, 2024 19:11

rcalixte added a commit to rcalixte/terminator that referenced this pull request Feb 17, 2024

Updating documentation in doc/* to reflect dual keybinding options

e3edf44

Ref: gnome-terminator#870

rcalixte mentioned this pull request Feb 17, 2024

Updating documentation in doc/* to reflect dual keybinding options #888

Open

mattrose reviewed Feb 18, 2024

View reviewed changes

rcalixte force-pushed the multiple_keybinds branch from a7c7073 to dcf6443 Compare February 20, 2024 18:01

rcalixte added a commit to rcalixte/terminator that referenced this pull request Feb 20, 2024

Updating documentation in doc/* to reflect dual keybinding options

a0a1b84

Ref: gnome-terminator#870

rcalixte force-pushed the multiple_keybinds branch from dcf6443 to 520fd1f Compare March 16, 2024 02:56

rcalixte added a commit to rcalixte/terminator that referenced this pull request Mar 16, 2024

Updating documentation in doc/* to reflect dual keybinding options

8940b9f

Ref: gnome-terminator#870

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for a second keybind per action #870

Add support for a second keybind per action #870

rcalixte commented Jan 11, 2024 •

edited

mattrose commented Jan 13, 2024

mattrose commented Feb 17, 2024

rcalixte commented Feb 17, 2024

rcalixte commented Feb 17, 2024

mattrose Feb 18, 2024 •

edited

rcalixte Feb 18, 2024 •

edited

rcalixte Feb 18, 2024

mattrose Feb 18, 2024

mattrose left a comment

mattrose Feb 18, 2024

rcalixte Feb 18, 2024

mattrose Feb 18, 2024

rcalixte Feb 18, 2024

mattrose Feb 18, 2024

rcalixte Feb 18, 2024

mattrose Feb 18, 2024

mattrose Feb 18, 2024

Add support for a second keybind per action #870

Are you sure you want to change the base?

Add support for a second keybind per action #870

Conversation

rcalixte commented Jan 11, 2024 • edited

mattrose commented Jan 13, 2024

mattrose commented Feb 17, 2024

rcalixte commented Feb 17, 2024

rcalixte commented Feb 17, 2024

mattrose Feb 18, 2024 • edited

Choose a reason for hiding this comment

rcalixte Feb 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcalixte commented Jan 11, 2024 •

edited

mattrose Feb 18, 2024 •

edited

rcalixte Feb 18, 2024 •

edited