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

feat(console): Add vi style keybinds for navigating tables #223

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

k0nserv
Copy link
Contributor

@k0nserv k0nserv commented Dec 17, 2021

Adds hjkl as alternatives to the arrow keys and G and gg to navigate
to the first and last rows in tables, respectively.

Changes to the UI:

image

@k0nserv k0nserv force-pushed the implement-vi-style-keybinds branch from cb3ddd4 to a206da6 Compare December 17, 2021 18:35
@k0nserv k0nserv changed the title Add vi style keybinds for navigating tables feat(console):Add vi style keybinds for navigating tables Dec 17, 2021
@k0nserv k0nserv changed the title feat(console):Add vi style keybinds for navigating tables feat(console): Add vi style keybinds for navigating tables Dec 17, 2021
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, i'm sure this will make a lot of people very happy! :)

we also display the keybindings in the UI, so maybe we should also add the vi-style keybindings to the table controls we display, here:

pub(in crate::view) fn controls(styles: &view::Styles) -> Text {
tui::text::Text::from(Spans::from(vec![
Span::raw("controls: "),
bold(styles.if_utf8("\u{2190}\u{2192}", "left, right")),
text::Span::raw(" = select column (sort), "),
bold(styles.if_utf8("\u{2191}\u{2193}", "up, down")),
text::Span::raw(" = scroll, "),
bold(styles.if_utf8("\u{21B5}", "enter")),
text::Span::raw(" = view details, "),
bold("i"),
text::Span::raw(" = invert sort (highest/lowest), "),
bold("q"),
text::Span::raw(" = quit"),
]))
}

also, I wonder if it's worth making a CLI option for toggling between vi only, arrow keys only, and enabling both? but, we could also do that in a follow-up PR.

@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 17, 2021

Thanks for the suggestions, I considered both those options as I wrote this.

For adding the bindings to the UI I wasn't sure how to do it so I wanted to raise it here instead.

One possibility is \u{2190}(h)\u{2192}(l) for left/right and likewise for up and down. I'm happy to add this, my concern was that it might be a bit messy. I guess for G and gg those could just be added directly.

e.g. like this
image

An option to consider if this display of keybinding does get too unwieldy is ? to bring up a floating window that displays them all.

About the CLI toggling: My thinking was there's no harm in having both by default, folks will use the ones they know/prefer? I'm not sure there's any benefit to allowing them to be disabled, but I can certainly add that in a follow up PR if you wish

@hawkw
Copy link
Member

hawkw commented Dec 17, 2021

One possibility is \u{2190}(h)\u{2192}(l) for left/right and likewise for up and down.

How about \u{2190}/\u{2192} or h/l and similar, instead? (ideally, with the or and the slashes not bolded). But, honestly, I'm fine with whatever. I think just displaying gg and G separately the way you did in your screenshot looks correct.

An option to consider if this display of keybinding does get too unwieldy is ? to bring up a floating window that displays them all.

Yeah, that might be a good idea, especially if we add more controls for things like filtering (#131) in the future. A simpler solution might just be to make it two lines rather than one line, but that eats up more vertical real estate...

About the CLI toggling: My thinking was there's no harm in having both by default, folks will use the ones they know/prefer? I'm not sure there's any benefit to allowing them to be disabled, but I can certainly add that in a follow up PR if you wish

Honestly, the only real benefit I had in mind is that it makes the controls list shorter to disable one set of keybindings :) I really don't think people are likely to disable or enable keybindings with a command-line option. I think that if we eventually add a config file, it might be more likely to want to configure keybindings there, but in that case, I'd imagine users would want to specify custom keybindings, rather than just turning them on or off. All of that is just something to think about for the future, though.

Once we've added the vi keybindings to the table controls, I'll be happy to merge this!

@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 18, 2021

image

Here's what it looks like now

hawkw added a commit that referenced this pull request Dec 18, 2021
Currently, the controls line for table views is never wrapped, even if
it's too long to fit on a single line in the current terminal width.
This isn't great...especially since PR #223 is adding more controls, and
will make the line even longer.

This branch allows wrapping the controls paragraph if it doesn't fit in
the current area. The code for this was surprisingly complex: while
`tui` supports wrapping text, layout area sizes are determined _before_
text wrapping. So, we can't just say "use as many lines as are necessary
to fit this text in the current terminal width", we have to implement
that logic ourselves, and pass the required number of lines to the
`Layout::constraints` method.
@hawkw
Copy link
Member

hawkw commented Dec 18, 2021

image

Here's what it looks like now

Yup, that looks good to me. I noticed that the controls line is now quite long, and probably won't fit in a narrower terminal...and that we truncate the line instead of wrapping it. So, I went ahead and implemented wrapping in PR #231, so that we can add more text to the controls and still have it display nicely on narrower terminals. Once that's merged, let's go ahead and merge this PR!

hawkw added a commit that referenced this pull request Dec 18, 2021
Currently, the controls line for table views is never wrapped, even if
it's too long to fit on a single line in the current terminal width.
This isn't great...especially since PR #223 is adding more controls, and
will make the line even longer.

This branch allows wrapping the controls paragraph if it doesn't fit in
the current area. The code for this was surprisingly complex: while
`tui` supports wrapping text, layout area sizes are determined _before_
text wrapping. So, we can't just say "use as many lines as are necessary
to fit this text in the current terminal width", we have to implement
that logic ourselves, and pass the required number of lines to the
`Layout::constraints` method.

## Screenshots

On the main branch, we don't wrap. Here's a wide enough terminal:
![image](https://user-images.githubusercontent.com/2796466/146655512-59ee64b1-eeb6-418d-b0d0-e84e5b80fb87.png)
...and here's one that's too narrow:
![image](https://user-images.githubusercontent.com/2796466/146655518-760387ab-b1c4-45f2-8f1a-eaf59a16067a.png)

After this change, when the terminal is wide enough, it looks the same:
![image](https://user-images.githubusercontent.com/2796466/146655537-2b1f7f16-0b1f-4a43-a6bb-d40b25569eef.png)
...but if the terminal is too narrow, it wraps:
![image](https://user-images.githubusercontent.com/2796466/146655545-130241ab-e483-40da-ba51-4a5dcf513cc5.png)

It might be nice to change this code to only wrap on commas, so each
`<keys> = <description>` isn't broken...but that seems like a lot more
work than just wrapping on whitespace, and the current thing is better
than it was before!
@hawkw
Copy link
Member

hawkw commented Dec 21, 2021

@k0nserv any updates here? It would be nice to get this updated with the latest main branch, and then I'll be happy to merge it!

@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 21, 2021

I saw that you had merged main into this branch and I thought you'd merge when #231 was merged.

What do you need me to do before we can merge?

@hawkw
Copy link
Member

hawkw commented Dec 21, 2021

What do you need me to do before we can merge?

It looks like the change for adding the keybindings to the help text hasn't actually been added to this branch yet? Once that's done, I'll merge this.

@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 21, 2021

Huh, I thought I pushed that, but I guess not then. Shall resolve

Adds `hjkl` as alternatives to the arrow keys and `G` and `gg` to navigate
to the first and last rows in tables, respectively.
@k0nserv k0nserv force-pushed the implement-vi-style-keybinds branch from b2c616d to 201f381 Compare December 21, 2021 21:02
@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 21, 2021

Rebased on main and pushed now

@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 21, 2021

I believe you need to approve the some of the workflows for them to run

image

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, looks good to me! thanks again for working on this!

@hawkw hawkw merged commit 1845c99 into tokio-rs:main Dec 21, 2021
@k0nserv
Copy link
Contributor Author

k0nserv commented Dec 21, 2021

Thanks for merging 👍

@k0nserv k0nserv deleted the implement-vi-style-keybinds branch December 21, 2021 22:22
hawkw added a commit that referenced this pull request Jan 18, 2022
<a name=""></a>
##  (2022-01-18)

#### Features

*  feature-flag `tracing-journald` dependency (#250) ([24f25db](24f25db))
*  add vi style keybinds for tables (#223) ([1845c99](1845c99))

#### Bug Fixes

*  fix task lookup in async ops view (#257) ([9a50b63](9a50b63))
*  don't make details requests with rewritten IDs (#251) ([4ec26a8](4ec26a8))
*  fix build error with journald enabled ([a931b7e](a931b7e))
*  increase default event buffer capacity a bit (#235) ([0cf0aee](0cf0aee))
*  wrap controls line when the terminal is too narrow (#231) ([ef41507](ef41507))
*  don't enable crossterm mouse capture (#222) ([e020d66](e020d66), closes [#167](167))
hawkw added a commit that referenced this pull request Jan 18, 2022
<a name=""></a>
##  (2022-01-18)

#### Features

*  feature-flag `tracing-journald` dependency (#250) ([24f25db](24f25db))
*  add vi style keybinds for tables (#223) ([1845c99](1845c99))

#### Bug Fixes

*  fix task lookup in async ops view (#257) ([9a50b63](9a50b63))
*  don't make details requests with rewritten IDs (#251) ([4ec26a8](4ec26a8))
*  fix build error with journald enabled ([a931b7e](a931b7e))
*  increase default event buffer capacity a bit (#235) ([0cf0aee](0cf0aee))
*  wrap controls line when the terminal is too narrow (#231) ([ef41507](ef41507))
*  don't enable crossterm mouse capture (#222) ([e020d66](e020d66), closes [#167](167))
hawkw added a commit that referenced this pull request Jan 18, 2022
<a name=""></a>
##  (2022-01-18)

#### Features

*  feature-flag `tracing-journald` dependency (#250) ([24f25db](24f25db))
*  add vi style keybinds for tables (#223) ([1845c99](1845c99))

#### Bug Fixes

*  fix task lookup in async ops view (#257) ([9a50b63](9a50b63))
*  don't make details requests with rewritten IDs (#251) ([4ec26a8](4ec26a8))
*  fix build error with journald enabled ([a931b7e](a931b7e))
*  increase default event buffer capacity a bit (#235) ([0cf0aee](0cf0aee))
*  wrap controls line when the terminal is too narrow (#231) ([ef41507](ef41507))
*  don't enable crossterm mouse capture (#222) ([e020d66](e020d66), closes [#167](167))
hawkw added a commit that referenced this pull request Jan 18, 2022
<a name=""></a>
##  (2022-01-18)

#### Features

*  feature-flag `tracing-journald` dependency (#250) ([24f25db](24f25db))
*  add vi style keybinds for tables (#223) ([1845c99](1845c99))

#### Bug Fixes

*  fix task lookup in async ops view (#257) ([9a50b63](9a50b63))
*  don't make details requests with rewritten IDs (#251) ([4ec26a8](4ec26a8))
*  fix build error with journald enabled ([a931b7e](a931b7e))
*  increase default event buffer capacity a bit (#235) ([0cf0aee](0cf0aee))
*  wrap controls line when the terminal is too narrow (#231) ([ef41507](ef41507))
*  don't enable crossterm mouse capture (#222) ([e020d66](e020d66), closes [#167](167))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants