rndusr / stig

TUI and CLI for the BitTorrent client Transmission
GNU General Public License v3.0
554 stars 24 forks source link

Ability to unbind a key from all contexts at once #195

Closed BioBox closed 3 years ago

BioBox commented 3 years ago

The unbind command may be insufficient, especially for those who use a non-qwerty keyboard layout, because there's no way to completely unbind a key. If you want 'n' to be the key for <down> and tried bind n <down>, then one would be quite annoyed to find that the key is still bound in the helptext and tab contexts.

One idea would be to bind a key to a certain action in all contexts, but many actions only make sense in a particular one and default is supposed to handle that anyway.

Instead I think it's much better to give unbind the pseudo-context all which completely unbinds the key in all contexts.

FYI: This passes both the commands test and the tui test.

rndusr commented 3 years ago

Thank you for contributing! Sounds like a good idea. But I have a few minor complaints.

if not key_removed:
    return 1
else:
    return 0

Looks like you could just return key_removed here? Don't return an integer if you're using it as a bool.

if all([_unbind(key, context) for context in self.contexts]):

You don't need to create a list here. all() can take a generator directly:

if all(_unbind(key, context) for context in self.contexts):

Also, the rest of the function uses self._actions instead of self.contexts, which does the same thing, but is inconsistent and confusing when reading the code.

I'll take a closer look over the weekend.

BioBox commented 3 years ago

Thanks for responding.

  1. Yeah, please change it to return key_removed and pretend I didn't write that. :sweat_smile: I'm very used to returning 0 for success.
  2. Actaully, that's exactly what I first did, but there's a gotcha. Check this out. You'll find that wrapping it in a list fixes things.
  3. Yes self._actions should have been used here, but self.contexts is so more readable that I used it without realizing.

It looks like you can take it from here, but I can amend it myself too if you want.

rndusr commented 3 years ago

What exactly does passing a list to all() fix? Your link just explains what I'm saying: You can pass any iterable to all, not just a sequence.

Please fix the other stuff. It's your PR, not mine.

I'd also appreciate if you could look into writing tests for it. But I don't remember how messy they are. Feel free to leave that to me.

BioBox commented 3 years ago

Do you get notified when I push to this branch? Anyways I did everything you asked. The test code was very readable and easy to add on to. It's not much but I can't think of anything more to add.

Oh almost forgot. The reason why it's a list is because here Python stops iteration when it finds a falsy element. When that happens the first context will be unbound and all other potential contexts are unharmed. Wrapping it in a list fixes this: the iteration may still be cut short, but the list must be built before that.

Just to be 100% sure I removed the bracket and ran my test code. It failed.

rndusr commented 3 years ago

I made some adjustments, mainly I turned ALL_CONTEXTS back into a real constant.

Instead of introducing a new magic context, that is passed from the CLI right into the guts of stig, I'm using the existing --all flag:

:unbind --all h j k l