-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
Introduce new help view to display information on any view #243
Conversation
console/src/view/mod.rs
Outdated
@@ -164,6 +171,17 @@ impl View { | |||
} | |||
} | |||
} | |||
_ => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that this is unreachable. I guess I will have to create a new ViewState
state to indicate that the help state is present. I think as long as I store what the previous state was, I can go back to it when esc
(or any other key) is pressed.
@bIgBV this is awesome, thanks for working on this! As for what should be displayed in the help text:
|
|
console/src/view/help.rs
Outdated
.margin(0); | ||
|
||
let content_area = content_layout | ||
.constraints([layout::Constraint::Length(height)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to create an area within the popup to render the text into, which would be responsive, but doing it like this doesn't seem to be working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what isn't working, exactly? is the problem that the text isn't wrapping?
console/src/view/help.rs
Outdated
.constraints([layout::Constraint::Length(height)]) | ||
.split(popup_area)[0]; | ||
|
||
let display_text = Paragraph::new(content).block(styles.border_block().title("Help")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want the text to wrap, you need to call .wrap()
on the Paragraph
:
let display_text = Paragraph::new(content).block(styles.border_block().title("Help")); | |
let display_text = Paragraph::new(content) | |
.block(styles.border_block().title("Help")) | |
.wrap(Wrap { trim: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was exactly what I was looking for. Thanks!
@hawkw got the text in the help popup to wrap properly (thanks again for the suggestion). I also updated the text in the detail views to be (slightly) less generic. I think this is in a good place to be merged and I could separately open a new PR once we finalize the content of the help text for different views. |
@bIgBV sorry it took me so long to look at this, I'd really like to actually give it a review now. It looks like this branch is now out of date with |
We want to display the help popop in the current view, no matter whichever it might be. This is because we might have distinct help infomation per view. Therefore, the help view being displayed is unrelated to any state in which the console as a whole is. We handle this by separating the rendering of the help popup from the state of the application.
For now I just copied the controls sections in the table for this.
@hawkw no worries. I just rebased this branch on top of main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me such a long time to review this, but it looks like a good start. I had some small suggestions.
Thanks for working on this! If you no longer have the time to continue working on it, I could make the remaining suggested changes myself and go ahead and merge this branch. Let me know if you'd like that. Thanks again!
let content = self | ||
.help_text | ||
.take() | ||
.expect("help_text should be initialized") | ||
.into(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kind of weird to me that help_text
is an Option
that we have to take
here? Is there a reason the render
method has to take an &mut self
? It could just take self
--- this isn't a trait method or anything.
_area: layout::Rect, | ||
_state: &mut State, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this has these arguments, when it doesn't use them? since this isn't a trait method, I think we should just not take these arguments just because they are passed to other views' render
methods.
_area: layout::Rect, | |
_state: &mut State, |
@@ -168,32 +177,56 @@ impl View { | |||
update_kind | |||
} | |||
|
|||
fn handl_help_popup(&mut self, event: crossterm::event::Event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: should probably be
fn handl_help_popup(&mut self, event: crossterm::event::Event) { | |
fn handle_help_popup(&mut self, event: crossterm::event::Event) { |
} | ||
if self.show_help_toggle && !input::is_help_toggle(&event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, take it or leave it: if we used if ... else if
, we could avoid the second call to input::is_help_toggle
:
} | |
if self.show_help_toggle && !input::is_help_toggle(&event) { | |
} else if self.show_help_toggle { |
@@ -168,32 +177,56 @@ impl View { | |||
update_kind | |||
} | |||
|
|||
fn handl_help_popup(&mut self, event: crossterm::event::Event) { | |||
if input::is_help_toggle(&event) { | |||
// TODO: Pause state if we are about to show the help toggle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably do this before merging?
help_content = HelpView::new(TableListState::<TasksTable>::render_help_content( | ||
&self.styles, | ||
)); | ||
} | ||
ViewState::ResourcesList => { | ||
self.resources_list | ||
.render(&self.styles, frame, area, state, ()); | ||
help_content = HelpView::new( | ||
TableListState::<ResourcesTable>::render_help_content(&self.styles), | ||
); | ||
} | ||
ViewState::TaskInstance(ref mut view) => { | ||
let now = state | ||
.last_updated_at() | ||
.expect("task view implies we've received an update"); | ||
view.render(&self.styles, frame, area, now); | ||
help_content = HelpView::new(TaskView::render_help_content(&self.styles)); | ||
} | ||
ViewState::ResourceInstance(ref mut view) => { | ||
view.render(&self.styles, frame, area, state); | ||
help_content = HelpView::new(ResourceView::render_help_content(&self.styles)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that this will construct the help content for each view every time we render a screen, even if we're not showing the help window. That seems a little bit inefficient, since we have to allocate a big vec
and a bunch of strings.
I think it might be more efficient to add a second match self.state
inside of the if self.show_help_toggle
box, and only build the help text if we are actually going to display a help box. That might make the method a bit longer, but I think it's worth doing to avoid having to format the help text when we're not going to display it...
@@ -267,6 +267,10 @@ impl TaskView { | |||
frame.render_widget(fields_widget, fields_area); | |||
frame.render_widget(percentiles_widget, percentiles_area); | |||
} | |||
|
|||
pub(in crate::view) fn render_help_content(_styles: &view::Styles) -> Spans<'static> { | |||
Spans::from(vec![Span::raw("A view to diplay help data for a task")]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also include controls?
@bIgBV quick ping — still interested in working on this? |
@hawkw I completely missed my notifications on this. While I'm still interested in working on this, my job has kept me pretty busy, and will keep doing so for the next couple of weeks. So if you can pull in my changes and add the ones you suggested on top of that, please go for it! |
@bIgBV Hey, would you feel comfortable giving me write access to your fork repo ("Adding contributors" in settings). Otherwise, I'd need to start a fork from scratch and apply your changes etc. which might get a bit gnarly, and we'd lose this convo history. |
@erwanor I've sent you an invitation. |
Adds a help modal which is available on every view. The help help modal can be accessed by pressing `?` and overlays the current view. To leave the help modal, the user can press `?` or `Esc`. This PR is based on #243 originally authored by @bIgBV. The previous PR has been dormant for around a year. Currently the help modal only displays a vertical list of controls. This is the same information that is available in the controls widget on each view. Here is an example of the tasks view with the help view modal active: ```text connection: http://localhost:6669/ (CONNECTED) views: t = tasks, r = resources controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details = ↵, invert sort (highest/lowest) = i, scroll to top = gg, scroll to bottom╭Help──────────────────────────────────────────╮t = q ╭Warnings───────│controls: │───────────────╮ │⚠ 1 tasks have │ select column (sort) = ←→ or h, l │ │ ╰───────────────│ scroll = ↑↓ or k, j │───────────────╯ ╭Tasks (12) ▶ Ru│ view details = ↵ │───────────────╮ │Warn ID State│ invert sort (highest/lowest) = i │t Location│ │ 19 ▶ │ scroll to top = gg │::task console-│ │ 22 ⏸ │ scroll to bottom = G │::task console-│ │⚠ 1 23 ⏸ │ toggle pause = space │::task console-│ │ 24 ⏸ │ toggle help = ? │::task console-│ │ 25 ⏸ │ quit = q │::task console-│ │ 74 ⏹ │ │::task console-│ │ 75 ⏸ │ │::task console-│ │ 77 ⏸ │ │::task console-│ │ 78 ⏸ ╰──────────────────────────────────────────────╯::task console-│ │ 79 ⏹ wait 11s 4ms 56µs 11s 2 tokio::task console-│ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` However, the idea is that the help modal can provide contextual information depending on the view and the state of the application being observed. This will allow us to provide more details about any lints which are currently triggering and also to reduce the height of the current controls widget to just one line (perhaps optionally) as the full list of controls can be accessed from the help view. Co-authored-by: bIgBV <[email protected]>
Adds a help modal which is available on every view. The help help modal can be accessed by pressing `?` and overlays the current view. To leave the help modal, the user can press `?` or `Esc`. This PR is based on #243 originally authored by @bIgBV. The previous PR has been dormant for around a year. Currently the help modal only displays a vertical list of controls. This is the same information that is available in the controls widget on each view. Here is an example of the tasks view with the help view modal active: ```text connection: http://localhost:6669/ (CONNECTED) views: t = tasks, r = resources controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details = ↵, invert sort (highest/lowest) = i, scroll to top = gg, scroll to bottom╭Help──────────────────────────────────────────╮t = q ╭Warnings───────│controls: │───────────────╮ │⚠ 1 tasks have │ select column (sort) = ←→ or h, l │ │ ╰───────────────│ scroll = ↑↓ or k, j │───────────────╯ ╭Tasks (12) ▶ Ru│ view details = ↵ │───────────────╮ │Warn ID State│ invert sort (highest/lowest) = i │t Location│ │ 19 ▶ │ scroll to top = gg │::task console-│ │ 22 ⏸ │ scroll to bottom = G │::task console-│ │⚠ 1 23 ⏸ │ toggle pause = space │::task console-│ │ 24 ⏸ │ toggle help = ? │::task console-│ │ 25 ⏸ │ quit = q │::task console-│ │ 74 ⏹ │ │::task console-│ │ 75 ⏸ │ │::task console-│ │ 77 ⏸ │ │::task console-│ │ 78 ⏸ ╰──────────────────────────────────────────────╯::task console-│ │ 79 ⏹ wait 11s 4ms 56µs 11s 2 tokio::task console-│ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` However, the idea is that the help modal can provide contextual information depending on the view and the state of the application being observed. This will allow us to provide more details about any lints which are currently triggering and also to reduce the height of the current controls widget to just one line (perhaps optionally) as the full list of controls can be accessed from the help view. Co-authored-by: bIgBV <[email protected]>
Adds a help modal which is available on every view. The help help modal can be accessed by pressing `?` and overlays the current view. To leave the help modal, the user can press `?` or `Esc`. This PR is based on #243 originally authored by @bIgBV. The previous PR has been dormant for around a year. Currently the help modal only displays a vertical list of controls. This is the same information that is available in the controls widget on each view. Here is an example of the tasks view with the help view modal active: ```text connection: http://localhost:6669/ (CONNECTED) views: t = tasks, r = resources controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details = ↵, invert sort (highest/lowest) = i, scroll to top = gg, scroll to bottom╭Help──────────────────────────────────────────╮t = q ╭Warnings───────│controls: │───────────────╮ │⚠ 1 tasks have │ select column (sort) = ←→ or h, l │ │ ╰───────────────│ scroll = ↑↓ or k, j │───────────────╯ ╭Tasks (12) ▶ Ru│ view details = ↵ │───────────────╮ │Warn ID State│ invert sort (highest/lowest) = i │t Location│ │ 19 ▶ │ scroll to top = gg │::task console-│ │ 22 ⏸ │ scroll to bottom = G │::task console-│ │⚠ 1 23 ⏸ │ toggle pause = space │::task console-│ │ 24 ⏸ │ toggle help = ? │::task console-│ │ 25 ⏸ │ quit = q │::task console-│ │ 74 ⏹ │ │::task console-│ │ 75 ⏸ │ │::task console-│ │ 77 ⏸ │ │::task console-│ │ 78 ⏸ ╰──────────────────────────────────────────────╯::task console-│ │ 79 ⏹ wait 11s 4ms 56µs 11s 2 tokio::task console-│ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` However, the idea is that the help modal can provide contextual information depending on the view and the state of the application being observed. This will allow us to provide more details about any lints which are currently triggering and also to reduce the height of the current controls widget to just one line (perhaps optionally) as the full list of controls can be accessed from the help view. Co-authored-by: bIgBV <[email protected]>
Adds a help modal which is available on every view. The help help modal can be accessed by pressing `?` and overlays the current view. To leave the help modal, the user can press `?` or `Esc`. This PR is based on #243 originally authored by @bIgBV. The previous PR has been dormant for around a year. Currently the help modal only displays a vertical list of controls. This is the same information that is available in the controls widget on each view. Here is an example of the tasks view with the help view modal active: ```text connection: http://localhost:6669/ (CONNECTED) views: t = tasks, r = resources controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details = ↵, invert sort (highest/lowest) = i, scroll to top = gg, scroll to bottom╭Help──────────────────────────────────────────╮t = q ╭Warnings───────│controls: │───────────────╮ │⚠ 1 tasks have │ select column (sort) = ←→ or h, l │ │ ╰───────────────│ scroll = ↑↓ or k, j │───────────────╯ ╭Tasks (12) ▶ Ru│ view details = ↵ │───────────────╮ │Warn ID State│ invert sort (highest/lowest) = i │t Location│ │ 19 ▶ │ scroll to top = gg │::task console-│ │ 22 ⏸ │ scroll to bottom = G │::task console-│ │⚠ 1 23 ⏸ │ toggle pause = space │::task console-│ │ 24 ⏸ │ toggle help = ? │::task console-│ │ 25 ⏸ │ quit = q │::task console-│ │ 74 ⏹ │ │::task console-│ │ 75 ⏸ │ │::task console-│ │ 77 ⏸ │ │::task console-│ │ 78 ⏸ ╰──────────────────────────────────────────────╯::task console-│ │ 79 ⏹ wait 11s 4ms 56µs 11s 2 tokio::task console-│ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` However, the idea is that the help modal can provide contextual information depending on the view and the state of the application being observed. This will allow us to provide more details about any lints which are currently triggering and also to reduce the height of the current controls widget to just one line (perhaps optionally) as the full list of controls can be accessed from the help view. Co-authored-by: bIgBV <[email protected]>
Adds a help modal which is available on every view. The help help modal can be accessed by pressing `?` and overlays the current view. To leave the help modal, the user can press `?` or `Esc`. This PR is based on #243 originally authored by @bIgBV. The previous PR has been dormant for around a year. Currently the help modal only displays a vertical list of controls. This is the same information that is available in the controls widget on each view. Here is an example of the tasks view with the help view modal active: ```text connection: http://localhost:6669/ (CONNECTED) views: t = tasks, r = resources controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details = ↵, invert sort (highest/lowest) = i, scroll to top = gg, scroll to bottom╭Help──────────────────────────────────────────╮t = q ╭Warnings───────│controls: │───────────────╮ │⚠ 1 tasks have │ select column (sort) = ←→ or h, l │ │ ╰───────────────│ scroll = ↑↓ or k, j │───────────────╯ ╭Tasks (12) ▶ Ru│ view details = ↵ │───────────────╮ │Warn ID State│ invert sort (highest/lowest) = i │t Location│ │ 19 ▶ │ scroll to top = gg │::task console-│ │ 22 ⏸ │ scroll to bottom = G │::task console-│ │⚠ 1 23 ⏸ │ toggle pause = space │::task console-│ │ 24 ⏸ │ toggle help = ? │::task console-│ │ 25 ⏸ │ quit = q │::task console-│ │ 74 ⏹ │ │::task console-│ │ 75 ⏸ │ │::task console-│ │ 77 ⏸ │ │::task console-│ │ 78 ⏸ ╰──────────────────────────────────────────────╯::task console-│ │ 79 ⏹ wait 11s 4ms 56µs 11s 2 tokio::task console-│ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` However, the idea is that the help modal can provide contextual information depending on the view and the state of the application being observed. This will allow us to provide more details about any lints which are currently triggering and also to reduce the height of the current controls widget to just one line (perhaps optionally) as the full list of controls can be accessed from the help view. Co-authored-by: bIgBV <[email protected]>
Adds a help modal which is available on every view. The help help modal can be accessed by pressing `?` and overlays the current view. To leave the help modal, the user can press `?` or `Esc`. This PR is based on #243 originally authored by @bIgBV. The previous PR has been dormant for around a year. Currently the help modal only displays a vertical list of controls. This is the same information that is available in the controls widget on each view. Here is an example of the tasks view with the help view modal active: ```text connection: http://localhost:6669/ (CONNECTED) views: t = tasks, r = resources controls: select column (sort) = ←→ or h, l, scroll = ↑↓ or k, j, view details = ↵, invert sort (highest/lowest) = i, scroll to top = gg, scroll to bottom╭Help──────────────────────────────────────────╮t = q ╭Warnings───────│controls: │───────────────╮ │⚠ 1 tasks have │ select column (sort) = ←→ or h, l │ │ ╰───────────────│ scroll = ↑↓ or k, j │───────────────╯ ╭Tasks (12) ▶ Ru│ view details = ↵ │───────────────╮ │Warn ID State│ invert sort (highest/lowest) = i │t Location│ │ 19 ▶ │ scroll to top = gg │::task console-│ │ 22 ⏸ │ scroll to bottom = G │::task console-│ │⚠ 1 23 ⏸ │ toggle pause = space │::task console-│ │ 24 ⏸ │ toggle help = ? │::task console-│ │ 25 ⏸ │ quit = q │::task console-│ │ 74 ⏹ │ │::task console-│ │ 75 ⏸ │ │::task console-│ │ 77 ⏸ │ │::task console-│ │ 78 ⏸ ╰──────────────────────────────────────────────╯::task console-│ │ 79 ⏹ wait 11s 4ms 56µs 11s 2 tokio::task console-│ ╰──────────────────────────────────────────────────────────────────────────────╯ ``` However, the idea is that the help modal can provide contextual information depending on the view and the state of the application being observed. This will allow us to provide more details about any lints which are currently triggering and also to reduce the height of the current controls widget to just one line (perhaps optionally) as the full list of controls can be accessed from the help view. Co-authored-by: bIgBV <[email protected]>
As no further progress had been made on this PR, a new one was opened (#432) inspired by it and has now been merged. |
As a part of addressing #225 , I'm introducing a new floating pane which can show information relevant to the current in focus pane. This can include shortcuts, key bindings and glossary information when using symbols and emojis..