vonshednob / pter

Manage your todo.txt in a commandline user interface (TUI)
https://vonshednob.cc/pter/
MIT License
102 stars 6 forks source link

Bug: chained keybindings do not seem to work on the Help screen #59

Closed andrei-a-papou closed 5 months ago

andrei-a-papou commented 5 months ago

image

I'm using Ctrl + x, q for quit to keep myself from accidentally quitting the app (a plain q is too easy to hit). I've just noticed that this binding has no effect on the Help screen, even though it seems to be recognized. Single-key bindings work fine, as does Ctrl + C.

vonshednob commented 5 months ago

Oh, interesting! I’ll have a look at that.

vonshednob commented 5 months ago

Yeah, that simply was not implemented at all. While at it, I also fixed the strange scrolling behaviour (I had to press the cursor key a bunch of times before it started scrolling).

Try it out with the latest commit!

andrei-a-papou commented 5 months ago

Chained keybindings on Help screen mostly work, but there are some hiccups:

  1. I use ^Xq to exit Help screen. The combination works, but after returning to the main screen, ^X remains painted at the bottom of the screen.
  2. On the Help screen, if I press ^X, there's no way to cancel (except by hitting q and exiting). On the main screen, I can press ^X and then a random unused key (or simply do a ^C) to break the chain.
  3. There's no feedback for non-existing keys on the Help screen. On the main screen, pter prints a No such keybinding error and cancels the chain. This works for chained and non-chained keybindings.
vonshednob commented 5 months ago

Hm, good one. The happy paths worked :sweat_smile: Try with this: ef741a1d03

andrei-a-papou commented 5 months ago

This is much nicer, thank you!

Strangely, though ^L doesn't seem to refresh things and pter shows the No such keybinding error.

As a side note, do we really need Quit and Refresh screen on the Help screen? Personally, I don't mind Refresh screen (if it works), but maybe we should remove the Quit hint? It functions the same as Cancel anyway, so it's a tiny bit confusing.

image

vonshednob commented 5 months ago

I think the problem is that the help bar does not actually show the commands available in the help screen, but keeps showing the general pter shortcuts :thinking:

The fact that it accepts both quit and cancel is … uhm, historical? Not much thinking ever went into the help bar on the help screen; for kinda obvious reasons, I think. But yeah, it should still be correct, helpful, and consistent.

vonshednob commented 5 months ago

You know what? quit should actually quit (talk about being consistent) and cancel should only close the help screen.

So the help bar stays as it is, but quit actally quits and refresh now works.

andrei-a-papou commented 5 months ago

You know what? quit should actually quit (talk about being consistent) and cancel should only close the help screen.

In terms of consistency, I agree 100%!

However, maybe I'm missing something, but why would you want to quit the app from the Help screen? Especially with the default q binding that's all too easy to hit -- and then it's good-bye my complex unsaved search, for example.

  1. I'm not sure making hints on Help screen configurable is worth it, so maybe just removing the "Quit" hint is easiest?

  2. Or at least moving it to the far right, so it's not the first hint/action? In which case (also for consistency) I would make the "Quit" action come last on the main screen too (I'm talking about the default hints here).

What do you think?

vonshednob commented 5 months ago

Maybe not just removing the quit hint, but the functionality altogether :thinking: but then I'll have to protect the user from locking themselves in (like it's done with the quit function).

Having quit as the first help option in the main screen is intentional: sure, the help bar could deploy some backpack-problem solution to ensure that the last help option is quit, but it's so much easier to show it as the first option. pter shouldn't be the vi/vim of TUI task managers ;)

andrei-a-papou commented 5 months ago

but then I'll have to protect the user from locking themselves in

I'm not sure I follow... You can always "Cancel"/^C, go back to main screen and quit from there, no?

Having quit as the first help option in the main screen is intentional

It's your call -- after all, the main screen hints are fully configurable :)

vonshednob commented 5 months ago

but then I'll have to protect the user from locking themselves in

I'm not sure I follow... You can always "Cancel"/^C, go back to main screen and quit from there, no?

Oh, allow me to elaborate: since a user might configure q = nop, pter will not start if there is no shortcut to quit.

Similarly pter should deny to start if there is no shortcut for cancel, because otherwise a user might get stuck in the help screen since quit won't be accessible in the help screen either.

Having quit as the first help option in the main screen is intentional

It's your call -- after all, the main screen hints are fully configurable :)

True, true. So many new features; it's admittedly hard to keep track :sweat_smile:

andrei-a-papou commented 5 months ago

Ah, I see, but doesn't pter already check for quit = nop and cancel = nop? I just checked and it already refuses to start if either is unset.

True, true. So many new features

It's time to release them, then ;)

vonshednob commented 5 months ago

Ah, I see, but doesn't pter already check for quit = nop and cancel = nop? I just checked and it already refuses to start if either is unset.

What? You're right, it already does… I had no idea. git blame says it's been a year though, so that's way beyond the event horizon.

True, true. So many new features

It's time to release them, then ;)

It really is. I'll still remove quit from the help screen functions though. And let's see if the scrolling can be fixed into submission.

andrei-a-papou commented 5 months ago

It really is. I'll still remove quit from the help screen functions though.

I just tried the latest commit, and the quit function on Help screen no longer works via keybinding -- so that's good. But the hint for it is still there, FYI

vonshednob commented 5 months ago

Argh. Yeah, fixed that.

andrei-a-papou commented 5 months ago

Works, thank you!

vonshednob commented 5 months ago

I'll plan to release 3.16 then later this week, hoping that nothing else gets in the way. I'm halfway considering giving this release the nickname 'andrei-a-papou'.

andrei-a-papou commented 5 months ago

Thank you, but no need: you've already mentioned me in git commits, this is plenty :)

vonshednob commented 5 months ago

Closing this, so I don't have to feel bad to release with pending issues in the milestone.

andrei-a-papou commented 5 months ago

Tested again, things seem to be working fine here!