Skip to content
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.

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

Release mapped modifiers after the original is released. Closes #70. #71

Merged
merged 6 commits into from
Apr 25, 2020

Conversation

rbreaves
Copy link
Contributor

No description provided.

@rbreaves rbreaves changed the title Release mapped modifiers after the original released. Closes #70. Release mapped modifiers after the original is released. Closes #70. Apr 19, 2020
released_modifiers_keys.append(modifier_key)
# Do not release new modifier
# until original modifier is released
if modifier_key != str(modifier.get_key()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnboundLocalError: local variable 'modifier' referenced before assignment

Copy link
Contributor Author

@rbreaves rbreaves Apr 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.. I am not really seeing or getting that error, but I will admit.. it seems like I should probably be having to define "modifier" within a loop, which I am not doing. Perhaps it only runs because of the prior for loop above it?

for modifier in combo.modifiers:

Is there a switch/argument I can run to get more debug like output when running it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I am not sure how to generate the debug output you showed I went ahead and set it to iterate through the possible modifier keys. I am not 100% certain it covers all use cases though, nor previously to be honest. It does appear to work fine with my use cases though.

There is one minor issue with using the pass_through_key. If I hit f3 and then try to use another mapped key then that mapped key fails initially until I hit it a second time.

# the issue
K("f3"): pass_through_key, # cancel find_next

K("C-g"): K("f3"), # find_next
K("Super-C-g"): K("M-f3"), # Default - find_all_under

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug output I posted was just the exception that was spat out to the terminal that I started xkeysnail in. You can add print statements etc to help with debugging.

I just added in your most recent commit, and this patch seems broken. My bindings to allow C-v and M-v in Firefox for page down / page up no longer work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Admittedly, the branch I'm testing this on contains other patches I've been using for a while, so there could be other interactions. https://github.com/Lenbok/xkeysnail/tree/tmp-test-mod-release )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, with your latest commit the single key mappings are fixed.

It's still broken. E.g. I have added tmux-ish style bindings to firefox like this:

# Keybindings for Firefox/Chrome
define_keymap(re.compile("Firefox|Google-chrome"), {
    # [...]
    # ` YYY, like tmux equivalents
    K("GRAVE"): {
        # ` ` (backtick)
        K("GRAVE"): K("GRAVE"),
        # ` k (close tab)
        K("k"): K("C-w"),
        # ` K (undo close tab)
        K("Shift-k"): K("C-Shift-t"),
        # ` c (new tab)
        K("c"): K("C-t"),
        # ` C (new container tab)
        K("Shift-c"): K("C-DOT"),
        # ` n (next tab)
        K("n"): K("C-TAB"),
        # ` p (prev tab)
        K("p"): K("C-Shift-TAB"),
        # ` TAB (switch to most recent tab, requires "Most Recent Tab" add on)
        K("TAB"): K("C-Shift-key_1"),
        # ` 1 (switch to tab 1)
        K("key_1"): K("M-key_1"),
        K("key_2"): K("M-key_2"),
        K("key_3"): K("M-key_3"),
        K("key_4"): K("M-key_4"),
        K("key_5"): K("M-key_5"),
    }
}, "Firefox and Chrome")

When I press grave followed by c, to create a new tab, the underlying C-t is issued but the control modifier is stuck down. (i.e. subsequently pressing s opens the save dialog). Is the problem that none of the input keys actually involved the modifier being issued?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually been wondering about nested scenarios, but I also haven't really tested any non-modifier key scenarios either I don't think. I will have to look into this more later. It's getting late for me.

I've been testing with this set of hotkeys.
https://github.com/rbreaves/kinto/blob/dev/xkeysnail/kinto.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lenbok I updated my patch to include mode_maps or multiple stroke keybinds rather. I tested it against your keybindings and everything appeared to work except for Grave - p. I am not sure why, but even with xkeysnail not running I couldn't get C-Shift-Tab to work right off any ways, only after C-Tab starts the tab switching process.. so I dunno what was up with that. I guess adding C-Tab to the start of it might have fixed it for me, but I did not test that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be a better way to handle setting when to mind multiple stroke keybinds, but I am basically just checking if _mode_map is anything but None, it is set when a multi stroke keybind is in play and if it is then I know to set a global boolean to remind the on_key function that it should manually release any mods set on the next go around.

Normally output.py would handle the release of mods that had been set, but I removed that bit of code completely as it interferes with proper remaps that would otherwise align properly with the length of the original modifier key being held.

Stuck mod keybinds are a risk I suppose with this change, but I think it is worth figuring out given the ability to remap keys like Alt and Ctrl-Tab can benefit due to the nature of how those keys work. Another alternative might be to create a similar but different K like function, maybe HK( ), to denote that keys being mapped via this function will attempt to Hold modifier keys until the original modifiers are released.

Copy link
Contributor Author

@rbreaves rbreaves Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now running my patch on a chromebook and have 1 or 2 other users using it as part of my Kinto (mac keybinding) app. It is part of my Alpha branch over there, after a few more days of testing and if I work out my wordwise support for browsers I'll merge into Kinto's dev branch. At least that is my plan.

@Lenbok If you still have any issues please let me know.


Initially I was going to work on a separate branch and integrate caret input detection directly into xkeysnail, but it is such a niche feature I really can't see the benefit of having it in xkeysnail directly. It would probably just introduce bugs and I still can't get IBus to work fully under Arch w/ Gnome, so there is that too.

I do have another ticket open for config py file update detection, which would also be beneficial. For now I am handling that via inotify-tools in bash as I don't think it can be implemented the same way that hotplug (watch) command is and would require another library to allow for a non-blocking while loop, I think. I tried already but it didn't go anywhere and I am not wanting to add a new library just for that.

@mooz mooz merged commit e3dcedd into mooz:master Apr 25, 2020
Lenbok added a commit to Lenbok/xkeysnail that referenced this pull request May 21, 2020
Fixes mooz#74, mooz#80, mooz#81

This reverts commit e3dcedd, reversing
changes made to 123432b.
paulie-g added a commit to paulie-g/xkeysnail that referenced this pull request Oct 17, 2020
Revert "Merge pull request mooz#71 from rbreaves/master"
trueneu added a commit to trueneu/xkeysnail that referenced this pull request Oct 22, 2020
Revert "Merge pull request mooz#71 from rbreaves/master"
mooz added a commit that referenced this pull request Oct 25, 2020
Revert "Merge pull request #71 from rbreaves/master"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants