woelper / oculante

A fast and simple image viewer / editor for many operating systems
https://github.com/woelper/oculante
MIT License
979 stars 43 forks source link

Keybinding in 0.6.35 is great, but not always registered #105

Open eugenesvk opened 1 year ago

eugenesvk commented 1 year ago

First of, awesome first attempt at keybindings — I can make c a on-hold modifier for all channel related switching, e.g.

That's a rather rare in-app keybinding convenience!

There are just a few of issues:

  1. tap c activates on press rather than on release, so I can't use c as a single key if I'm also using it as a modifier. Would be nice if the keys that are bound to have both single-tap functions and hold functions the single tap happened on release
  2. there is some interference with the great key rebinding app Karabiner Elements, but not a permanent one, check out the attached video: holdc,tap a is the troubling keybind, you can see from the bottom right event viewer that Oculante "ignores" the first few presses, but then activates all channels on a subsequent one. Sometimes it activates fine on the first one, so not sure what exactly is going on

https://user-images.githubusercontent.com/12956286/210088999-6a88064c-3ab8-42c2-b5ff-4cda6cfd3669.mov

  1. similarly, sometimes it sees modifiers invoked via Karabiner as 'stuck' (e.g., I have as a 'hyper' key that is mapped to rightrrr) and while trying rebinding keys I've noticed that Oculante indicates that these keys are pressed long after they've been released. Maybe there could be an extra check after some delay to see if the key is still pressed?
woelper commented 1 year ago

Glad you like it, and good points. I will improve keybindings in the future hopefully with:

I'll look into 3. I had to hack a bit around some limitations, but I hope this will improve in the future.

woelper commented 1 year ago

Is there anything speaking against having all shortcuts on key release?

Aligorith commented 1 year ago

Just checked what we used to do for Blender - Seems most hotkeys are mapped on Press, with the occasional troublesome ones set to Release.

IIRC, one of the main arguments against is that people often expect shortcuts to act when they press the key, so waiting until the key is released to fire may be confusing / feel laggy (e.g. "why hasn't it worked yet?").

woelper commented 1 year ago

Thanks for checking @Aligorith (and thanks for your work on blender, I am a big grease pencil fan :) ).

Maybe I am not seeing the full picture, but I don't see how we could support any sort of keys being pressed together (like a and b) while also allowing them pressed alone with being "on press". If that should work, the only thing I see is some delay to check if it is indeed a single keypress or if the user is pressing more. But this would introduce latency/delay too.

Another option would be to parse all available commands and see if any uses the key(s) which are down, and if there are any, handle the event on release - but this sounds dangerously like over-engineering.

At present I am checking if modifiers are down while the "on press" happens, so you could at least reliably press Ctr-a and a, but this won't work for any key.

eugenesvk commented 1 year ago

Is there anything speaking against having all shortcuts on key release?

I think so, you'd have unnecessary delay. Plus, for this type of "dual keys" I think it's not just triggering on release, but only triggering:

eugenesvk commented 1 year ago

but this sounds dangerously like over-engineering.

I'd hope that this "over-engineering" happened at some great Rust keybinding library, but I'm also aware at how rare good keybind support is, so that might not exist either :( If only you didn't need to track app state (e.g., whether you're inside some text field and need c to insert a letter right away), at least you could overengineer on the user side, but alas, afaik there is not way to track that kind of thing to account for in an external kbd remapping app

woelper commented 1 year ago

The keybinding is pretty much custom made, I am only getting events and do the handling myself, hence the overengineering remark (mostly towards me). I'll think a bit more, also to enable key repeats.

eugenesvk commented 1 year ago

Yep, understood that, just meant that (in a better world) you shouldn't really be the one handling all of this complexity yourself from the raw events, but instead use a great keybinding lib that would allow you to simply expose all the functionality to the user, who could then decide whether some key would need to be different on tap vs hold etc.

woelper commented 1 year ago

As the referenced issues are closed, can this be closed, too?

eugenesvk commented 1 year ago

Unfortunately, not, this is still one of the biggest issues preventing proper use, specifically, the 3rd point where the keys are stuck and block all keybinds