New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send broken keychains in passthrough modes to browser #3683

Open

jakanaka-evan wants to merge 5 commits into qutebrowser:main from jakanaka-evan:forward-keys

jakanaka-evan commented Mar 7, 2018 •

edited by The-Compiler

Fixes #1044.

This change is

qutebrowser-bot added status: needs review component: keyinput priority: 3 - wishlist labels

Member

The-Compiler commented Mar 7, 2018

All the code movement makes it really difficult to review this (and introduces a high chance of conflicts) I'm afraid. Can you please open a separate PR for getting rid of keyparser.py (which should be straightforward to merge), and then rebase this (or open a new PR) based on that?

Some early feedback:

In e3e19b5 (in press), you're basically reimplementing Qt's event handling (for QLineEdit objects). I'd much prefer sending those events to the application (so they are handled correctly in any case) and finding some clean way to ignore them instead.
I don't understand 8cff673 and unfortunately the commit message just reiterates what the code already says, but doesn't say why this is done 😉

The-Compiler removed the status: needs review label

jakanaka-evan force-pushed the forward-keys branch from 8cff673 to bf38272 Compare

March 8, 2018 23:36

qutebrowser-bot added the status: needs review label

jakanaka-evan mentioned this pull request

keyinput: Merge keyparser into modeparsers #3689

Merged

Author

jakanaka-evan commented Mar 8, 2018

This PR is now based on #3689.

Member

The-Compiler commented Mar 9, 2018

Hmm, looks like the move is still there... Can you please rebase on the latest master?

The-Compiler removed the status: needs review label

jakanaka-evan force-pushed the forward-keys branch from bf38272 to 5afb066 Compare

March 9, 2018 14:22

qutebrowser-bot added the status: needs review label

The-Compiler requested changes

View reviewed changes

qutebrowser/keyinput/modeparsers.py Outdated

@@ @@ -154,6 +155,8 @@ class PassthroughKeyParser(CommandKeyParser): @@
                   Attributes:
                       _mode: The mode this keyparser is for.
+                      _orig_sequence: Cuerrent sequence with no key_mappings applied

Member

The-Compiler Mar 13, 2018

Cuerrent -> Current

qutebrowser/keyinput/modeparsers.py Outdated

@@ @@ -154,6 +155,8 @@ class PassthroughKeyParser(CommandKeyParser): @@
                   Attributes:
                       _mode: The mode this keyparser is for.
+                      _orig_sequence: Cuerrent sequence with no key_mappings applied
+                      _ignore_next_key: Whether to pass the next key through

Member

The-Compiler Mar 13, 2018

Please add a period here and on the line above.

qutebrowser/keyinput/modeparsers.py Outdated

                   def __repr__(self):
                       return utils.get_repr(self, mode=self._mode)
+                  def handle(self, e, *, dry_run=False):
+                      """Override to pass the chain through on NoMatch

Member

The-Compiler Mar 13, 2018

Please add a period.

qutebrowser/keyinput/modeparsers.py Outdated

+                      if window is None:
+                          return match
+                      self._ignore_next_key = True

Member

The-Compiler Mar 13, 2018

This seems rather brittle, as you're not guaranteed that the event you posted will be the next event which is received. I haven't tested it, but it likely also breaks if there's more than one key in the original sequence.

I'm okay with it when there's no other solution, but I'd like to check whether there's a better way. Can you try adding an attribute to the press_event / release_event object and see whether it's still there (via getattr(e, 'ignore', False) or so) when you get it back?

qutebrowser/keyinput/modeparsers.py Outdated

+                          A self.Match member.
+                      """
+                      if keyutils.is_modifier_key(e.key()) or self._ignore_next_key:
+                          self._ignore_next_key = self._ignore_next_key and dry_run

Member

The-Compiler Mar 13, 2018

I don't get this - why do you only want to set self._ignore_next_key if dry_run is set?

qutebrowser/keyinput/modeparsers.py

                   def __repr__(self):
                       return utils.get_repr(self, mode=self._mode)
+                  def handle(self, e, *, dry_run=False):

Member

The-Compiler Mar 13, 2018

This seems to duplicate a lot of the logic in BaseKeyParser.match, so I'm wondering whether there's a better way. Maybe KeySequence should have an original attribute where the original is stored when using with_mappings instead? Not sure how much easier this would make the code without seeing it though - what do you think?

Author

jakanaka-evan Mar 13, 2018

Here's a patch that adds a without_mappings method to KeySequence, to see how the code looks would look like. It's kind of clunky and incomplete though (even has pylint errors), because KeySequence is used pretty much as an immutable object.
If you want, I can rewrite the KeySequence class to be more mutable, but that would probably belong in it's own PR.
https://gist.github.com/jakanaka-evan/4973c4a4e46e8e3bdae57b17dba0e26c

qutebrowser/keyinput/modeparsers.py Outdated

+                  def clear_keystring(self):
+                      """Override to also clear the original sequence."""
+                      if self._orig_sequence:

Member

The-Compiler Mar 13, 2018

Why bother with the check? You can just clear it unconditionally.

qutebrowser/keyinput/modeparsers.py

+                      """Override to also clear the original sequence."""
+                      if self._orig_sequence:
+                          self._orig_sequence = keyutils.KeySequence()
+                      super().clear_keystring()

Member

The-Compiler Mar 13, 2018

Usually additional things are done after the super() call, not before it.

The-Compiler removed the status: needs review label

jakanaka-evan force-pushed the forward-keys branch from 5afb066 to ada54bb Compare

March 13, 2018 18:17

qutebrowser-bot added the status: needs review label

jakanaka-evan force-pushed the forward-keys branch from ada54bb to 29e06d3 Compare

March 13, 2018 19:34

qutebrowser-bot added the status: stale label

Member

The-Compiler commented Jul 4, 2018

I planned to take a look at this during my holidays before starting to learn for exams, but I didn't manage to do so (mainly because of a lot of issues with Qt 5.11...). I'll get back to it in September though!

qutebrowser-bot removed the status: stale label

qutebrowser-bot added the status: stale label

jgkamat requested changes

View reviewed changes

Member

jgkamat left a comment

I don't have an opinion between the alternative patch and the current one, but something that does need to be fixed is not respecting input.partial_timeout in passthrough mode. Since lifting it into commandkeyparser is probably not desirable, maybe there could be a helper function to set up the timeouts? A little bit of duplication is probably fine though :)

jakanakaevangeli added 4 commits

September 21, 2018 09:05


          Pass broken keychains to browser in passthrough modes

7a3ebe0


          Don't use key_mappings when passing keys

77481b4


          Use ignore_event attribute on events

a0a9ffc


          Respect input.partial_timeout in all modes

f2c3d27

jakanaka-evan force-pushed the forward-keys branch from 29e06d3 to f2c3d27 Compare

September 21, 2018 10:51

Author

jakanaka-evan commented Sep 21, 2018 •

edited

The last two commits address the partial_timeout issue for all modes (it was
lifted into BaseKeyParser). I can separate them into a new PR if you want.


          Stop and disconnect input.partial_timeout

7124f42

jakanaka-evan force-pushed the forward-keys branch from c37de35 to 7124f42 Compare

September 21, 2018 15:52

The-Compiler removed the status: needs review label

The-Compiler mentioned this pull request

Map keychains in passthrough-keymodes #1044

Open

The-Compiler removed the status: stale label

The-Compiler added this to Backlog in Pull request backlog

brightonanc added a commit to brightonanc/qutebrowser that referenced this pull request


          Modernizing the results from PR qutebrowser#3683

f8200c0

brightonanc mentioned this pull request

Full support for multi-key sequences #7162

Open

brightonanc added a commit to brightonanc/qutebrowser that referenced this pull request


          Modernizing the results from PR qutebrowser#3683

c4c132e

brightonanc added a commit to brightonanc/qutebrowser that referenced this pull request


          Modernizing the results from PR qutebrowser#3683

cf3aac4

brightonanc added a commit to brightonanc/qutebrowser that referenced this pull request


          Modernizing the results from PR qutebrowser#3683

52bb474

brightonanc added a commit to brightonanc/qutebrowser that referenced this pull request


          Modernizing the results from PR qutebrowser#3683

c4e5933

brightonanc added a commit to brightonanc/qutebrowser that referenced this pull request


          Modernizing the results from PR qutebrowser#3683

f522c3c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment