rbreaves / kinto

Mac-style shortcut keys for Linux & Windows.
http://kinto.sh
GNU General Public License v2.0
4.27k stars 214 forks source link

Idea: syntax sugar for "remap and pass thru" #697

Open joshgoebel opened 2 years ago

joshgoebel commented 2 years ago

Is your feature request related to a problem? Please describe.

It's hard to read the config and understand it when combos that are really pairs are spread out across multiple lines. I'd much rather look at/review 20 Sublime Text macros (to see if anything is missing) than review 40 when 20 of them are just "pass thru".

I wonder what kind of syntax sugar we might add at the remapper level to make this easier to write:

    K("insert"): pass_through_key,              # cancel toggle_overwrite
    K("C-M-o"): K("insert"),                    # toggle_overwrite
    K("Alt-c"): pass_through_key,                 # cancel toggle_case_sensitive
    K("C-M-c"): K("Alt-c"),                       # toggle_case_sensitive
    K("C-h"): pass_through_key,                 # cancel replace
    K("C-M-f"): K("C-h"),                       # replace
    K("C-Shift-h"): pass_through_key,           # cancel replace_next
    K("C-M-e"): K("C-Shift-h"),     

Right now it's a bit hard to see that these belong in pairs as well, making it more likely that we'll make an editing mistake int he future and only update one half.

Describe the solution you'd like

Putting "one combo per line" (with it's meta data included - such as patching up pass thru's, etc) would be preferable, perhaps simplify all the way to something like:

COMBO("C-M-c", "Alt-c"), passthru_original = true)

OR perhaps if this applies to every single combo we don't need to repeat it on every single line and could push that up into a wrapper... So you'd have:

define_keymap(re.compile("Sublime_text|^Subl$", re.IGNORECASE),{
   # no pass thrus
    K("Super-Space"): K("C-Space"),             # Basic code completion
    K("C-Super-up"): K("Alt-o"),                  # Switch file
    K("Super-RC-f"): K("f11"),                  # toggle_full_screen
    K("C-M-v"): [K("C-k"), K("C-v")]} | 
   # pass thru all
    pass_thru_all_originals({
    K("C-M-c"): K("Alt-c"),                       # toggle_case_sensitive
    K("C-M-f"): K("C-h"),                       # replace
    K("C-M-e"): K("C-Shift-h"),                 # replace_next
    K("C-g"): K("f3")}
)

Describe alternatives you've considered

None. I know you have your YAML/JSON ideas but I'm also interested if any of this stuff is easier to do at the python level as well.

rbreaves commented 2 years ago

Wonder if we could also do something like this.

COMBOS("
Super-Space : C-Space,
C-Super-up : Alt-o,
Super-RC-f : f11,
C-M-v : [C-k,C-v]
")

I don't know, may need single quotes inside it.

rbreaves commented 2 years ago

Optional third : to contain a description. Might be helpful if a UI is made for it later.

rbreaves commented 2 years ago

I am not sure if I follow your pass through example. I don't think anything is really being repeated, some combos just don't exist at all on macOS and need to be cleared out otherwise they'll still resolve while the proper set works as well. If you forego cancelling the original hotkeys they'd still function afaik. Only time I might not cancel a combo is if it conflicts with another and I get to just remap it to what it ought to be.

joshgoebel commented 2 years ago

I'm saying this pattern is hard to read and makes it look like there is a lot more complexity that there really is.

   K("Alt-c"): pass_through_key,
    K("C-M-c"): K("Alt-c"),   

The K("Alt-C") is repeated, when it doesn't need to be. This is a common pattern so it really should have a nice abstraction so one look at it and it's clear. The programmer in me screams to DRY this up. The above says: map one combo, then map a different one.

This is clearer:

map Ctrl-Alt-c  =>  Alt-c  (pass thru prior)

But like I said if we're passing it thru for ALL 20 then we don't need to repeat it 20 times, you could just list it once.


I'm also still not 100% sure what this doess... is this trying to hook up or break the old combos?

rbreaves commented 2 years ago

I'm also still not 100% sure what this doess... is this trying to hook up or break the old combos?

pass through simply breaks the old combo from working if you were to try and physically press Alt-C on your keyboard. It is technically still available but only to the virtual keyboard which xkeysnail creates (it's own virtual keyboard uinput based device).

When you press Ctrl-Alt-C it activates Alt-C at that moment, but yea the user can't simply press Alt-C because Alt-C is not available in that manner to macOS users.

Oh.. I think I am following you now.. You are saying by it being in that method you will automatically disable the physical combo on the right?

rbreaves commented 2 years ago

Trying to think if there is better terminology for this tbh that would be more clear for anyone and really I am not certain if I cancelled all that could be, so it may not even be fully consistent.

joshgoebel commented 2 years ago

You are saying by it being in that method you will automatically disable the physical combo on the right?

I'm saying you could automate that part with a nice abstraction to avoid those 20 lines, yes...

But I'm still not sure I follow....

When you press Ctrl-Alt-C it activates Alt-C at that moment,

Yep this is completely undertood, remapping 101.

but yea the user can't simply press Alt-C because Alt-C is not available in that manner to macOS users.

This I don't get though because all "pass thru" does is actually PASS THRU the key, it pushes it on the output - it doesn't "Drop it" or "ignore it"... it pushes it...

joshgoebel commented 2 years ago
  K("f9"): pass_through_key,      

This literally sends F9 to the output...

        elif command is pass_through_key:
            send_key_action(key, action)
            return True
joshgoebel commented 2 years ago

This literally sends F9 to the output...

And that's what would happen if you just pushed F9 (and it wasn't remapped)... so I don't understand why all those lines are necessary at all? Could you give me a specific example with Sublime Text of what would break if I removed one of the passthrus?

I've gotta be missing some nuance.

rbreaves commented 2 years ago

This literally sends F9 to the output...

It will output to the terminals console log yes - but I think the context the original author meant it in is that it passes through or out of ever registering with the actual virtual keyboard that xkeysnail created to interact w/ the OS. It is a black hole essentially where that particular combo goes in but never comes out on the other side as anything...

If you pressed F9 while it is mapped to a pass_through_key it would then do nothing. If you remove all the pass_through_keys you'd then have a bunch of hotkeys with duplicate functionality walking around aka the macOS remaps PLUS the Windows/Linux shortcut keys where possible.

Having the pass through stuff in there ensures that the user is only presented with macOS remaps and not additional hotkeys traditionally associated with the app running on Linux or Windows.

rbreaves commented 2 years ago

To be honest the naming of it is horrible - I think it has confused many people, including myself in the past. Literally naming it "the_black_hole" would have been more descriptive imho.

Or just do K("Alt-c"): K(""), but that might also create a nasty loop for all I know as you're not really wanting to clear the combo completely - you just don't want that physical combo to do anything.. you still need that combo to exist so you can remap another combo to it.

joshgoebel commented 2 years ago

Or just do K("Alt-c"): K(""),

That's not what it does at all - it types the key on the virtual output.

I'm trying F3 right now in Sublime Text 4 and it does a "find next"... iE it's really:

K("f3"): F("f3"),  

https://github.com/rbreaves/xkeysnail/blob/kinto/xkeysnail/transform.py#L525

Send key action punches the key on the virtual output.

rbreaves commented 2 years ago

That's not what it does at all - it types the key on the virtual output.

Well I do not recall having made any changes to its behavior on my fork, but I am sitting here and testing this as we speak.. pass through swallows up the keys on my system and that is what it has always done for me... I can't explain why it'd work different for you.

joshgoebel commented 2 years ago

So F3 in Sublime text silently drops on the floor for you? It doesn't find the next item?

rbreaves commented 2 years ago

So F3 in Sublime text silently drops on the floor for you? It doesn't find the next item?

Yes. Exactly. I am testing this as well speak F3 does nothing, it falls completely away, only Ctrl+G will find the next match.

rbreaves commented 2 years ago

Seriously I think something is bugged on your system or maybe you need to jump on Ubuntu Budgie 22.04, because the behavior you are describing is rather bizarre for how the pass_through is supposed to work and afaik it is how it works on everyone's system. So unless there's something different or unique about Arch or you're running a version of xkeysnail that is doing something different then it ought to be dropping those hotkeys so you don't have 2 versions of any given hotkey running around.

I am sure I didn't even touch all of the potential hotkey duplicates, but I got rid of the ones I was aware of.

joshgoebel commented 2 years ago

Does it log as a combo on your system? Here is mine:

(DD) WM_CLS 'Sublime_text' | DEV 'Apple, Inc Apple Keyboard' | KMAPS = [global default, screen paging (not term), Mac OS Cmd+dot, General GUI, Wordwise - not vscode, Sublime Text]
(DD)   COMBO: F3 => {}   [Sublime Text]

I must be missing something. I'm running my fork, but I haven't touched this part of the code and I'm also literally reading the code from your branch... it literally passes the key straight thru to the virtual output device... 🐛

rbreaves commented 2 years ago

And the only way that your refactor would make sense is if the method was to know to automatically remove the hotkeys on the right part of that remapping equation.

joshgoebel commented 2 years ago

I think there is something weird about your patch that I'm missing. Even from the xkeysnail docs it says I'm correct (as I read it):

pass_through_key: Pass through key to the application. Useful to override the global mappings behavior on certain applications.

It passes the key to the application. It's usage is to override global mappings... ie if you mapped F3 globally but needed it to behave differently inside ST you'd have to pass it thru to prevent the other keymap from messing with it.

rbreaves commented 2 years ago

Does it log as a combo on your system? Here is mine:

Yea, it'll show up in the console yes, but it isn't registering that hotkey combo with the application itself - meaning that hotkey in reality is not making it into your session. Xkeysnail grabs it and never lets it back into the actual system to interact with the app, may not be clear in the terminal that that is what is happening but it is happening in that manner for me.

rbreaves commented 2 years ago

I think there is something weird about your patch that I'm missing. Even from the xkeysnail docs it says I'm correct (as I read it):

I'd be very surprised.. I think their documentation describing it is just plain wrong.

joshgoebel commented 2 years ago

pass thru matches the name "pass thru"... they could have named it "drop" or "ignore"... you changed the behavior somehow. I'd almost bet a small amount of money that if test 0.4.0 right now that it'll have the behavior I'm seeing, not that you are seeing.

rbreaves commented 2 years ago

I can see that you are a real stickler for documentation and things making logical sense vs intuiting what's happening 😂.

@RedBearAK had the same issue, so no worries! https://github.com/rbreaves/kinto/issues/508

joshgoebel commented 2 years ago

Well I'm literally testing it - same as you... but I can't win if you're going to say the canonical documentation is just wrong and that "pass thru" should really mean "ignore". :wink:

One of the first things I did in my fork was go an add a REAL ignore...

        elif command is ignore_key:
            debug("ignore_key", key)
            return True
rbreaves commented 2 years ago

It's more likely that I branched off a broken version of xkeysnail and I just took advantage of a broken feature that someone repaired or got working again after I had forked mine lol. Either way I am pretty sure I did not change the behavior of that method/function.

joshgoebel commented 2 years ago

OH I KNOW WHY. Your version is buggy. I fxied that bug also:

       elif isinstance(command, dict):
            _mode_maps = [command]
            return False

The dict match comes first and (for some crazy reason) pass_thru is an empty dict... so that code never runs... is sets a mode map instead (which is broken) and effectively it drops it on the floor.

That's NOT correct behavior, I guarantee you. It's a bug. :)

rbreaves commented 2 years ago

Oh lmao.. well I was telling you lol - I stopped questioning it a loooong time ago.

joshgoebel commented 2 years ago

I wasn't calling you a liar, just the code was contradicting you... and code never lies - unless you read it wrong. 🤦🏼‍♂️

One second I'll make a small patch for what you SHOULD do.

rbreaves commented 2 years ago

And the more important part was that it was written by Masafumi Oyamada lol! Admittingly though it could have just as easily been me breaking it as I had touched this send_key_action in a few areas which could have also indirectly changed its behavior once I started to look at it, but yea I will go with what you found!

rbreaves commented 2 years ago

Well it is late for me, so I will have to look at in the morning, but yea I appreciate the help!

rbreaves commented 2 years ago

Tbh - I am so used to things not lining up that I look at documentation like a rough guide sketched out in sand lol. I pretty well don't trust most pieces of documentation unless someone really slaps me in the face with it and somehow convinces me that the person that wrote it was half way competent and lucid when writing it lol.

joshgoebel commented 2 years ago

That's why I read the code too. :) It literally said "pass it thru, type it on the output"... just the ordering of the if else sequence was wrong. :-) Whoever wrote it didn't realize that pass thru is a dict... or didn't think it thru... empty dicts are terrible magic values.

Though even a specific dict would not have saved it in this case... A tiny enum class would probably be optimal.

The commit I fixed it (and added ignore):

https://github.com/joshgoebel/keyszer/commit/f1900d6a3dbb4e18de023f89d034b30d9470bb8f

joshgoebel commented 2 years ago

So I think (in my nicer syntax I'd probably write it like this):

ignore_original_combos(
  keymap("Sublime Text",{
      K("C-M-c"): K("Alt-c"),                       # toggle_case_sensitive
      K("C-M-f"): K("C-h"),                       # replace
      K("C-M-e"): K("C-Shift-h"),                 # replace_next
      K("C-g"): K("f3")}
)

IE, you just wrap the whole keymap (I think that'd typically be the most common need)... and then it gets expanded to have all the ignore_key auto-added... or it could be an argument:

keymap("Sublime Text",{
      K("C-M-c"): K("Alt-c"),                       # toggle_case_sensitive
      K("C-M-f"): K("C-h"),                       # replace
      K("C-M-e"): K("C-Shift-h"),                 # replace_next
      K("C-g"): K("f3")}, 
      ignore_original_combos = true
)

Actually that latter is pretty nice, I might change conditionals to be passed as an argument like that...

rbreaves commented 2 years ago

Yea I like that too! Prevents nesting and is very clear imo.

And thinking of a Linus Torvalds quote last night that had me laughing "It's not a bug if other people use it, it's a feature!" when talking about why not to fix API/ABI's that people use lol.

But I may be the heaviest user of the original bugged feature and yea using it as a way to ignore combos that I don't won't working lol.

rbreaves commented 2 years ago

I'd be tempted to say it ought to be part of the K method.. as it might be nicer to ignore some and not others and not have to define additional code blocks to do so.

keymap("Sublime Text",{
      K("C-M-c"): K("Alt-c","-i"),                       # toggle_case_sensitive
      K("C-M-f"): K("C-h","-i"),                       # replace
      K("C-M-e"): K("C-Shift-h","-i"),                 # replace_next
      K("C-g"): K("f3")}, 
)

I am not sure though, that may not be as readable as I'd like either.

Maybe this instead

keymap("Sublime Text",{
      K("C-M-c"): Ki("Alt-c"),                       # toggle_case_sensitive
      K("C-M-f"): Ki("C-h"),                       # replace
      K("C-M-e"): Ki("C-Shift-h"),                 # replace_next
      K("C-g"): K("f3")}, 
)

Have Ki expand to use K while also appling the ignore?

joshgoebel commented 2 years ago

as it might be nicer to ignore some and not others and not have to define additional code blocks to do so.

I feel like that is likely an edge case and should be treated as such... if you needed two groups (some ignored, some not) I'd just find a way syntactically to specify it per grouping - not per combo... "Don't repeat yourself".

It seems 99% of the time what we want to do is zap the original combo.

Have Ki expand to use K while also appling the ignore?

Not sure I love it, but once you are on my branch it'd be trivial to add that yourself... keymaps in my branch just return actual simple objects you can iterate over, modify, etc... so you could have a few wrappers... and loop over the keymaps after definition to find whatever object type Ki turns into and expand those all into multiple mappings.