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

Have ? be an additional/default shortcut for help #694

Closed
huyz opened this issue Sep 30, 2023 · 11 comments · Fixed by #703
Closed

Have ? be an additional/default shortcut for help #694

huyz opened this issue Sep 30, 2023 · 11 comments · Fixed by #703
Labels
Milestone

Comments

@huyz
Copy link

huyz commented Sep 30, 2023

? is much more common for help than h in TUIs.
Take BSD top or btop.

It would be nice if it were the default (or an additional) shortcut out of the box (I'd like to avoid having to configure trip if possible)

Note that often h is reserved for whenever vi motion keys are desired in TUIs.

@c-git
Copy link
Collaborator

c-git commented Sep 30, 2023

I think that's a good idea as well, It's what I would try first. I probably wouldn't change it to be the default so as to affect existing users. Changing defaults can be disruptive.

@fujiapple852
Copy link
Owner

Hey there @huyz. That is a fair ask, to do this the options seem to be:

  1. Continue to require users to override the key bindings

Trippy already allows for the ToggleHelp command to be rebound to ? as you note, and this can be done from the config file or the command line:

trip --tui-key-bindings toggle-help=\? example.com
  1. Change the default binding for the ToggleHelp command to ?

Whilst not impossible to change this default (I regard everything in Trippy as unstable until it hits 1.0), I'm not convinced that everyone would agree that this is a better default. I'd want to see more feedback from a larger pool of users before agreeing to make this change.

  1. Add support for being able to bind multiple keys to a command

This would avoid needing to change the existing default and would allow for maximum flexibility. It does add complexity and i'm unclear what this would look like in the Tui (i.e. in the help dialog or main dialog header). It doesn't seem like a feature that will be needed very often and so hard to justify the extra complexity.

  1. Add a special AltToggleHelp key binding which performs the same function as the ToggleHelp command

This is a bit of a hack, but it would allow for a 2nd default key binding to toggle the help dialog without introducing any extra complexity. It also does not solve the problem of unbinding h, which would still have to be done via config or command line.

A related issue here is that help dialog box in the Tui has hardcoded text and does not currently factor in the key binding. The same is true for the header. These should both be fixed to reflect the configured key bindings.

Further, the information shown in the help dialog is all also available in the settings dialog (bindings tab) which does respect key bindings and so arguably the help dialog should not show any key bindings. In which case, what information would we want to show in the help dialog instead?

@c-git
Copy link
Collaborator

c-git commented Oct 2, 2023

I'd love to hear @huyz feedback on this. But I am partial to something closer to #4 as I don't see it as necessary to remove the h binding. Furthermore after having looked at the interface again. I do NOT think the h should be removed as it fits well as a default given the bold h that shows in the header. I think help is an important functionality (more on that in a bit) and more ways to help users get started is better. I would like to point out (even though I'm likely in the minority that I keep my monitor pretty dim and contrast low as not everything supports dark mode) the bold highlights are not always very noticeable or conspicuous. That said, where I did look for available keys for actions I could perform was is in a row at the bottom, like what you get in the nano editor so you don't need to remember the bindings or figure out where to look them up. The most important ones are always there. I don't know how hard that would be to add but if we do then it should only show if there is space available. I've not done any TUI programming before but I did noticed that the amount of available space determines what shows and things "selectively" disappear based on priority as the window gets smaller.

More on help

I think if we can, we should change the help from being hard coded to show what the current hot keys are. Doesn't have to happen right away but I think it would be more useful to show users what they are not what the defaults are. I also think we could put more info into the help. I think it would be good if we could do something like a man page that gives some getting started tips and that kind of thing. I would need to sit for a bit longer and think about it more to say exactly what I think should be added. But I do think more than one screen of information would be better to help users figure what they need to do to get started. One thing that would be good to add for example would be a short explanation of what each section of the settings is so users would know where to look for things instead of just going through all of them to see what is there.

Example of how to make "new" hotkey more discoverable.

I think adding (?) to the header would be good to make it easier for ppl to find the help. That is easier to see for ppl like me where there isn't much contrast between bold and not bold. A key in brackets is more obvious in a monochrome context. However if we do decide to go with the bottom row keys thing then I think the right most one should be help and then no change would be needed to the header.

Screen Shot

PS. I would like to know how prevalent the perception is that h or ? is a better binding for help. Unfortunately polls are hard to get responses to so I'm not sure how we should go about getting more feedback. Arguably once you've found either option it doesn't really matter anymore because now you know.

@fujiapple852
Copy link
Owner

That said, where I did look for available keys for actions I could perform was is in a row at the bottom, like what you get in the nano editor so you don't need to remember the bindings or figure out where to look them up. The most important ones are always there.

I like the idea of a footer showing help keys; in fact I think Trippy had one early on but I removed it to save screen real estate and also put the help keys at the top right as there was a natural gap. I'd quite like to move the "Status: Running, discovered 10 hops" part to the footer and reduce the header by 1 line overall.

I don't know how hard that would be to add but if we do then it should only show if there is space available. I've not done any TUI programming before but I did noticed that the amount of available space determines what shows and things "selectively" disappear based on priority as the window gets smaller.

Easy to add, harder to make the layout responsive in this way. I'd be keen to see what this looks and feels like if someone as keen to experiment (https://github.com/fujiapple852/trippy/blob/master/src/frontend/render/app.rs is where the magic happens).

@c-git
Copy link
Collaborator

c-git commented Oct 2, 2023

Thanks, I'd love to take a stab at it but if I'm honest it will be a while before I can get to it. Even for trippy there are many more things I want to get done that are higher priority for me.

@huyz
Copy link
Author

huyz commented Oct 3, 2023

I can't speak to how many people would prefer ? over h besides my intuition. I would guess user expectation is correlated with what common tools use, so an unscientific survey of some arbitrary but common TUI tools that I use may be illustrative:

Both ? and h

  • mtr: ?, h with h hint displayed
  • htop: ?, h, F1 with F1 hint
  • procps-ng top (on Ubuntu): ?, h with no hints
  • ctop: ?, h with no hints
  • btop: ?, F1 and (unless vim_keys = True) h, with no hints

?

  • macOS top: ? with no hints
  • ncdu: ? with ? hint, h used for vi motion
  • gdu-go: ? with ? hint, h used for vi motion
  • lazygit: ? with ? hint, h used for vi motion
  • lazydocker: ? with no hint, h used for vi motion
  • ranger: ? with no hint, h used for vi motion
  • nnn: ? with no hint, h used for vi motion
  • hackernews-TUI: ? with ? hint, h used for vi motion
  • lnav: ? with ? hint, h used for vi motion
  • gotop: ? with no hint

h

  • glances: h with no hints displayed
  • multitail: h or F1 with F1 hint

Neither

  • midnightcommander: Alt-1 with hint displayed

@c-git I'm not sure what #4 is supposed to show. You have a screenshot?

@fujiapple852
Copy link
Owner

That’s great analysis @huyz !

it makes me feel like the option of adding an alternative help command is less of a hack as it seems quite common.

How about we follow the mtr approach?

mtr: ?, h with h hint displayed

That makes this trivial and avoids the problem of how to display both hints.

@c-git
Copy link
Collaborator

c-git commented Oct 3, 2023

@huyz #4 is basically do both.

Yeah I agree with going with the mtr approach. Tbh I'm not sure what hints means. I'll try to make time to try one of those listed to see what it is.

@huyz
Copy link
Author

huyz commented Oct 3, 2023

Tbh I'm not sure what hints means

By hint, I just mean that the keyboard shortcut is displayed in a legend in the UI so that the user doesn't have to guess.

@fujiapple852
Copy link
Owner

@huyz I've implemented the approach discussed above in #703. If you're about to try it out that would be great. This will go into the 0.9 release.

This PR:

  • Adds the ToggleHelpAlt command and binds it to ? by default
  • Modifies the tui to respect both ToggleHelp and ToggleHelpAlt
  • Adds toggle-help-alt to the template config file
  • Updates the help text in the help dialog to mention h and ?

We'll leave larger changes to the help dialog and/or the header/footer for another time.

@fujiapple852 fujiapple852 self-assigned this Oct 3, 2023
@fujiapple852 fujiapple852 added this to the 0.9.0 milestone Oct 3, 2023
@huyz
Copy link
Author

huyz commented Oct 3, 2023

51152c7 looks good. Thanks, I appreciate it!

@fujiapple852 fujiapple852 removed their assignment Oct 6, 2023
fujiapple852 added a commit that referenced this issue Nov 25, 2023
fujiapple852 added a commit that referenced this issue Nov 25, 2023
fujiapple852 added a commit that referenced this issue Nov 25, 2023
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 1, 2023
[0.9.0] - 2023-11-30

Added

- Added support for tracing flows
  ([#776](fujiapple852/trippy#776))
- Added support for `icmp` extensions
  ([#33](fujiapple852/trippy#33))
- Added support for `MPLS` label stack class `icmp` extension
  objects ([#753](fujiapple852/trippy#753))
- Added support for [paris]
  (https://github.com/libparistraceroute/libparistraceroute) ECMP routing
  for `IPv6/udp` ([#749](fujiapple852/trippy#749))
- Added `--unprivileged` (`-u`) flag to allow tracing without elevated
  privileges (macOS
  only) ([#101](fujiapple852/trippy#101))
- Added `--tui-privacy-max-ttl` flag to hide host and IP details for low ttl
  hops ([#766](fujiapple852/trippy#766))
- Added `toggle-privacy` (default: `p`) key binding to show or hide private
  hops ([#823](fujiapple852/trippy#823))
- Added `toggle-flows` (default: `f`) key binding to show or hide tracing
  flows ([#777](fujiapple852/trippy#777))
- Added `--dns-resolve-all` (`-y`) flag to allow tracing to all IPs resolved
  from DNS lookup
  entry ([#743](fujiapple852/trippy#743))
- Added `dot` report mode (`-m dot`) to output hop graph in Graphviz `DOT`
  format ([#582](fujiapple852/trippy#582))
- Added `flows` report mode (`-m flows`) to output a list of all unique tracing
  flows ([#770](fujiapple852/trippy#770))
- Added `--icmp-extensions` (`-e`) flag for parsing `IPv4`/`IPv6` `icmp`
  extensions ([#751](fujiapple852/trippy#751))
- Added `--tui-icmp-extension-mode` flag to control how `icmp` extensions are
  rendered ([#752](fujiapple852/trippy#752))
- Added `--print-config-template` flag to output a template config
  file ([#792](fujiapple852/trippy#792))
- Added `--icmp` flag as a shortcut for `--protocol icmp`
  ([#649](fujiapple852/trippy#649))
- Added `toggle-help-alt` (default: `?`) key binding to show or hide
  help ([#694](fujiapple852/trippy#694))
- Added panic handing to Tui
  ([#784](fujiapple852/trippy#784))
- Added official Windows `scoop` package
  ([#462](fujiapple852/trippy#462))
- Added official Windows `winget` package
  ([#460](fujiapple852/trippy#460))
- Release `musl` Debian `deb` binary asset
  ([#568](fujiapple852/trippy#568))
- Release `armv7` Linux binary assets
  ([#712](fujiapple852/trippy#712))
- Release `aarch64-apple-darwin` (aka macOS Apple Silicon) binary
  assets ([#801](fujiapple852/trippy#801))
- Added additional Rust Tier 1 and Tier 2 binary assets
  ([#811](fujiapple852/trippy#811))

Changed

- [BREAKING CHANGE] `icmp` extension object data added to `json` and `stream`
  reports ([#806](fujiapple852/trippy#806))
- [BREAKING CHANGE] IPs field added to `csv` and all tabular
  reports ([#597](fujiapple852/trippy#597))
- [BREAKING CHANGE] Command line flags `--dns-lookup-as-info` and
  `--tui-preserve-screen` no longer require a boolean
  argument ([#708](fujiapple852/trippy#708))
- [BREAKING CHANGE] Default key binding for `ToggleFreeze` changed from `f`
  to `ctrl+f` ([#785](fujiapple852/trippy#785))
- Always render AS lines in hop details mode
  ([#825](fujiapple852/trippy#825))
- Expose DNS resolver module as part of `trippy` library
  ([#754](fujiapple852/trippy#754))
- Replaced unmaintained `tui-rs` crate with `ratatui` crate
  ([#569](fujiapple852/trippy#569))

Fixed

- Reverse DNS lookup not working in reports
  ([#509](fujiapple852/trippy#509))
- Crash on NetBSD during window resizing
  ([#276](fujiapple852/trippy#276))
- Protocol mismatch causes tracer panic
  ([#745](fujiapple852/trippy#745))
- Incorrect row height in Tui hop detail navigation view for hops with no
  responses ([#765](fujiapple852/trippy#765))
- Unnecessary socket creation in certain tracing modes
  ([#647](fujiapple852/trippy#647))
- Incorrect byte order in `IPv4` packet length calculation
  ([#686](fujiapple852/trippy#686))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants