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

refac(console): generalize controls widget #427

Merged
merged 8 commits into from
Jun 1, 2023
Merged

refac(console): generalize controls widget #427

merged 8 commits into from
Jun 1, 2023

Conversation

hds
Copy link
Collaborator

@hds hds commented May 25, 2023

Each view in tokio-console has a widget up the top that lists the
available controls for that view. There was a common implementation of
this for table based views (tasks, resources, and async_ops) and
separate implementations for the task and resource views. The resource
view included two controls widgets, one for the Resource details at the
top of the view, and another above the table of Async Ops at the bottom
of the view.

This change centralises the logic for the creation of this controls
widget. This change is mostly a precursor to also displaying the
controls in the help view (at which point we can revisit whether the
entire list needs to be shown at the top of the screen).

Controls (an action and the key or keys used to invoke it) are defined
in structs so that their definition can be separated from the display
logic (which includes whether or not UTF-8 is supported).

This allows the problem of the text in the controls widget wrapping in
the middle of a control definition to be fixed. Previously a subset of
the controls would have wrapped like this:

controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details
= ↵, invert sort (highest/lowest) = i,

Notice how "view details = ↵," was split across multiple lines. The same
list of controls will now wrap at a full control definition.

controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j,
view details = ↵, invert sort (highest/lowest) = i,

Additionally, the list of controls on the Resource view has been
consolidated up the top of the screen.

Universal controls, those that are available in all views, are also
defined centrally. As well as the quit action, using the space bar to
pause has been added to that list. This was previously somewhat of an
undocumented feature.

Each view in tokio-console has a widget up the top that lists the
available controls for that view. There was a common implementation of
this for table based views (tasks, resources, and async_ops) and
separate implementations for the task and resource views. The resource
view included two controls widgets, one for the Resource details at the
top of the view, and another above the table of Async Ops at the bottom
of the view.

This change centralises the logic for the creation of this controls
widget. This change is mostly a precursor to also displaying the
controls in the help view (at which point we can revisit whether the
entire list needs to be shown at the top of the screen).

Controls (an action and the key or keys used to invoke it) are defined
in structs so that their definition can be separated from the display
logic (which includes whether or not UTF-8 is supported).

This allows the problem of the text in the controls widget wrapping in
the middle of a control definition to be fixed. Previously a subset of
the controls would have wrapped like this:

```text
controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details
= ↵, invert sort (highest/lowest) = i,
```

Notice how "view details = ↵," was split across multiple lines. The same
list of controls will now wrap at a full control definition.

```text
controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j,
view details = ↵, invert sort (highest/lowest) = i,
```

Additionally, the list of controls on the Resource view has been
consolidated up the top of the screen.

Universal controls, those that are available in all views, are also
defined centrally. As well as the quit action, using the space bar to
pause has been added to that list. This was previously somewhat of an
undocumented feature.
@hds hds requested a review from a team as a code owner May 25, 2023 14:15
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.

thanks, this looks good to me! i had some very minor suggestions, but they're not blockers for merging this!

tokio-console/src/view/controls.rs Show resolved Hide resolved
tokio-console/src/view/controls.rs Outdated Show resolved Hide resolved
Comment on lines 132 to 141
pub(crate) fn universal_controls() -> &'static Vec<ControlDisplay> {
static UNIVERSAL_CONTROLS: OnceCell<Vec<ControlDisplay>> = OnceCell::new();

UNIVERSAL_CONTROLS.get_or_init(|| {
vec![
ControlDisplay::new_simple("toggle pause", "space"),
ControlDisplay::new_simple("quit", "q"),
]
})
}
Copy link
Member

Choose a reason for hiding this comment

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

if we changed the ControlDisplay type to own an &'static [KeyDisplay] rather than a Vec, like I described above, UNIVERSAL_CONTROLS could just be a constant, rather than needing to be lazily initialized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Although in resource.rs I had to keep the OnceCell because I'm concatenating 2 arrays which it seems can't be dont in const.

If you know a way of doing this, that would be really cool.

tokio-console/src/view/controls.rs Outdated Show resolved Hide resolved
tokio-console/src/view/controls.rs Show resolved Hide resolved
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.

thanks, this looks good to me! i had some very minor suggestions, but they're not blockers for merging this!

hds and others added 3 commits May 25, 2023 22:26
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Make a bunch of things around ControlDisplay and KeyDisplay by using
array slices instead of vectors.
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.

looks good to me!

tokio-console/src/view/controls.rs Outdated Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) June 1, 2023 22:45
@hawkw hawkw merged commit ea00694 into tokio-rs:main Jun 1, 2023
hawkw pushed a commit that referenced this pull request Sep 29, 2023
Each view in tokio-console has a widget up the top that lists the
available controls for that view. There was a common implementation of
this for table based views (tasks, resources, and async_ops) and
separate implementations for the task and resource views. The resource
view included two controls widgets, one for the Resource details at the
top of the view, and another above the table of Async Ops at the bottom
of the view.

This change centralises the logic for the creation of this controls
widget. This change is mostly a precursor to also displaying the
controls in the help view (at which point we can revisit whether the
entire list needs to be shown at the top of the screen).

Controls (an action and the key or keys used to invoke it) are defined
in structs so that their definition can be separated from the display
logic (which includes whether or not UTF-8 is supported).

This allows the problem of the text in the controls widget wrapping in
the middle of a control definition to be fixed. Previously a subset of
the controls would have wrapped like this:

```text
controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details
= ↵, invert sort (highest/lowest) = i,
```

Notice how "view details = ↵," was split across multiple lines. The same
list of controls will now wrap at a full control definition.

```text
controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j,
view details = ↵, invert sort (highest/lowest) = i,
```

Additionally, the list of controls on the Resource view has been
consolidated up the top of the screen.

Universal controls, those that are available in all views, are also
defined centrally. As well as the quit action, using the space bar to
pause has been added to that list. This was previously somewhat of an
undocumented feature.
hds pushed a commit that referenced this pull request Feb 13, 2024
Each screen in Tokio Console has a controls section that lists all the
possible keys that can be used to control that screen. However, on the
Resource Detail screen, there were two controls sections - one at the
top of the screen and another above the Async Ops table.

Since the controls for the Async Ops view were already included in the
Resource Detail view controls panel in #427, the controls for the Async
Ops view only needed to be removed. This simplified the layout somewhat,
as the Async Ops view now only contains a single-subview, the table
itself.

Closes #258
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