feat(console): Add icon that sorting state #301

nrskt · 2022-03-04T09:58:30Z

Add ▵ or ▿ icon that represents the sorting state.

Relates to #300

hawkw

This is definitely a good idea, thanks for working on it!

The main thing I think we need to change before this is ready to merge is supporting ASCII-only mode, which I left a comment about. Besides that, this seems more or less correct --- I think it might be worth changing how we add extra length to column titles, so that we don't change column width as often, but that's up to you.

hawkw · 2022-03-04T17:28:13Z

tokio-console/src/view/tasks.rs

+                    "▵"
+                } else {
+                    "▿"


The console supports running in an ASCII-only mode, so we need to be able to switch between these Unicode characters and ASCII-only text. For other Unicode characters we use as icons, we use the if_utf8 method on the Styles type to switch between Unicode and ASCII variants:

console/tokio-console/src/view/styles.rs

Lines 57 to 63 in 900a5c2

pub fn if_utf8<'a>(&self, utf8: &'a str, ascii: &'a str) -> &'a str {

if self.utf8 {

utf8

} else {

ascii

}

}

Here, I think we should do something like this:

Suggested change

"▵"

} else {

"▿"

styles.if_utf8("▵", "+")

} else {

styles.if_utf8("▿", "-")

using + for ascending and - for descending in ASCII-only mode. alternatively, we could use something like (asc.)/(desc.) for ascending/descending, if we don't think a single character is descriptive enough...

note that if we use multiple characters in the ASCII-only mode, we'll also have to add that much additional length to the column width, rather than just adding a single character in every case.

Thank you for the clear explanation. I will fix it.

I fixed c29d6c9.

hawkw · 2022-03-04T17:33:13Z

tokio-console/src/view/tasks.rs

+        match table_list_state.sort_by {
+            SortBy::Warns => warn_width.update_len(warn_width.len() + 1),
+            SortBy::Tid => id_width.update_len(id_width.len() + 1),
+            SortBy::State => state_len += 1,
+            SortBy::Name => name_width.update_len(name_width.len() + 1),
+            SortBy::Polls => polls_width.update_len(polls_width.len() + 1),
+            SortBy::Target => target_width.update_len(target_width.len() + 1),
+            SortBy::Location => location_width.update_len(location_width.len() + 1),
+            SortBy::Total | SortBy::Busy | SortBy::Idle => (),
+        };


one note about this approach is that it means that changing the sorted column has the potential to make that column one character wider than it was previously, if none of the text in the column was longer than the header. i feel like this has the potential to look a little bit jarring if selecting a new column to sort by makes the columns shift around. it might honestly be better to just always add an additional character of width to every column?

but, i'm definitely willing to be convinced otherwise.

i feel like this has the potential to look a little bit jarring if selecting a new column to sort by makes the columns shift around. it might honestly be better to just always add an additional character of width to every column?

I agree. I feel it would be more clear to add an additional character of width to every column. However, I am confused because there are a few places in the current implementation where the width is adjusted. I would like to fix it at another opportunity and understand the whole implementation a little bit.

We can always address that later!

I realize an idea.

Add const WIDTHS param to TableList trait definition.

Set the widths on the implementation.

pub(crate) trait TableList<const N: usize> { type Row; type Sort: SortBy + TryFrom<usize>; type Context; const HEADER: &'static [&'static str; N]; const WIDTHS: &'static [usize; N]; fn render<B: tui::backend::Backend>( state: &mut TableListState<Self, N>, styles: &view::Styles, frame: &mut tui::terminal::Frame<B>, area: layout::Rect, state: &mut state::State, cx: Self::Context, ) where Self: Sized; }

impl TableList<11> for TasksTable { type Row = Task; type Sort = SortBy; type Context = (); const HEADER: &'static [&'static str; 11] = &[ "Warn", "ID", "State", "Name", "Total", "Busy", "Idle", "Polls", "Target", "Location", "Fields", ]; const WIDTHS: &'static [usize; 11] = &[ Self::HEADER[0].len() + 1, Self::HEADER[1].len() + 1, Self::HEADER[2].len() + 1, Self::HEADER[3].len() + 1, Self::HEADER[4].len() + 1, Self::HEADER[5].len() + 1, Self::HEADER[6].len() + 1, Self::HEADER[7].len() + 1, Self::HEADER[8].len() + 1, Self::HEADER[9].len() + 1, Self::HEADER[10].len() + 1, ];

What do you think it?
(I worry about implement WIDTHS param is complex and boring)

hawkw

oh, I just realized something else: we should make the same change for the resource list view (https://github.com/tokio-rs/console/blob/main/tokio-console/src/view/resources.rs) and the async ops list view (https://github.com/tokio-rs/console/blob/main/tokio-console/src/view/async_ops.rs). that should be pretty straightforward, but it would be nice to add this to every table view as part of this change.

since we'll be using the same icons for ascending and descending sorts on all three views, we may want to just add methods to the Styles type for returning the ascending and descending icons based on whether or not Unicode is enabled. There are existing methods on Styles for other commonly-used symbols, like the warning symbol:

console/tokio-console/src/view/styles.rs

Lines 123 to 128 in b4e6b6a

    
           pub fn warning_narrow(&self) -> Span<'static> { 
        
               Span::styled( 
        
                   self.if_utf8("\u{26A0} ", "! "), 
        
                   self.fg(Color::LightYellow).add_modifier(Modifier::BOLD), 
        
               ) 
        
           }

hawkw · 2022-03-04T20:02:08Z

tokio-console/src/view/tasks.rs

+        match table_list_state.sort_by {
+            SortBy::Warns => warn_width.update_len(warn_width.len() + 1),
+            SortBy::Tid => id_width.update_len(id_width.len() + 1),
+            SortBy::State => state_len += 1,
+            SortBy::Name => name_width.update_len(name_width.len() + 1),
+            SortBy::Polls => polls_width.update_len(polls_width.len() + 1),
+            SortBy::Target => target_width.update_len(target_width.len() + 1),
+            SortBy::Location => location_width.update_len(location_width.len() + 1),
+            SortBy::Total | SortBy::Busy | SortBy::Idle => (),
+        };


We can always address that later!

nrskt · 2022-03-04T20:07:36Z

cargo clippy raise a few warnings of unnecessary use of to_owned. I have tried to fix it but caused compile error...
Should I fix it? If so, how to fix it?

hawkw · 2022-03-04T21:56:05Z

cargo clippy raise a few warnings of unnecessary use of to_owned. I have tried to fix it but caused compile error... Should I fix it? If so, how to fix it?

This is fixed on the main branch, by PR #305. I'll update this branch from main and we should get a passing CI build now.

Add const parameter for the width of every column.

nrskt · 2022-03-05T10:12:50Z

@hawkw

I tried the idea about (feat(console): Add icon that sorting state #301 (comment)).
Fix resources and async_ops tables.
- If I misunderstood your advice (feat(console): Add icon that sorting state #301 (review)), feel free to point it out

hawkw

This looks good to me, thanks!

I had some suggestions, but you can feel free to take them or leave them. Let me know what you think!

hawkw · 2022-03-07T17:52:08Z

tokio-console/src/view/styles.rs

+        let value = format!("{}{}", value, self.if_utf8("▵", "+"));
+        self.selected(&value)
+    }
+
+    pub fn descending(&self, value: &str) -> Span<'static> {
+        let value = format!("{}{}", value, self.if_utf8("▿", "-"));


i think it might look a little bit nicer if we added a space after the symbol in the case where it's a plus or minus sign, but not a blocker.

hawkw · 2022-03-07T17:52:11Z

tokio-console/src/view/resources.rs

+        let viz_len: u16 = Self::WIDTHS[6] as u16;

-        let mut id_width = view::Width::new(Self::HEADER[0].len() as u16);
-        let mut parent_width = view::Width::new(Self::HEADER[1].len() as u16);
+        let mut id_width = view::Width::new(Self::WIDTHS[0] as u16);
+        let mut parent_width = view::Width::new(Self::WIDTHS[1] as u16);

-        let mut kind_width = view::Width::new(Self::HEADER[2].len() as u16);
-        let mut target_width = view::Width::new(Self::HEADER[4].len() as u16);
-        let mut type_width = view::Width::new(Self::HEADER[5].len() as u16);
-        let mut location_width = view::Width::new(Self::HEADER[7].len() as u16);
+        let mut kind_width = view::Width::new(Self::WIDTHS[2] as u16);
+        let mut target_width = view::Width::new(Self::WIDTHS[4] as u16);
+        let mut type_width = view::Width::new(Self::WIDTHS[5] as u16);
+        let mut location_width = view::Width::new(Self::WIDTHS[7] as u16);


The WIDTHS array seems fine to me, but, for what it's worth, think it would also be fine to just change this code to something like

let viz_len: u16 = (Self::HEADER[6].len() + 1) as u16; let mut id_width = view::Width::new((Self::HEADER[0].len() + 1) as u16); let mut parent_width = view::Width::new((Self::HEADER[1].len() + 1) as u16); // ... and so on ...

Since the HEADER array is a const, and the 1 is also a constant, this calculation should probably get const-folded, so I don't think there would be a performance difference between this approach and creating a separate const array. I think the code might be a bit simpler if we avoided the separate array.

But, this is fine either way --- it's not a big deal.

nrskt · 2022-03-08T02:17:41Z

@hawkw
Thank you for reviewing.
Your suggestions are very helpful to me. However, I would like to merge this PR.

I’m interested in this project and I hope I can reflect your suggestions in another PR.

hawkw · 2022-03-08T03:07:00Z

@hawkw Thank you for reviewing. Your suggestions are very helpful to me. However, I would like to merge this PR.

I’m interested in this project and I hope I can reflect your suggestions in another PR.

Sounds good, I'm happy to go ahead and merge this now!

tokio-console/src/view/styles.rs

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

## 0.1.3 (2022-03-09) #### Bug Fixes * exit crossterm before printing panic messages (#307) ([43606b9](43606b9)) * prevent panics if subscriber reports out-of-order times (#295) ([80d7f42](80d7f42)) #### Features * add icon representing column sorting state (#301) ([b9e0a22](b9e0a22))

feat(subscriber): Add icon that sorting state

4bc45d6

hawkw requested changes Mar 4, 2022

View reviewed changes

hawkw changed the title ~~feat(subscriber): Add icon that sorting state~~ feat(console): Add icon that sorting state Mar 4, 2022

feat(console): Support ASCII-only mode

c29d6c9

hawkw requested changes Mar 4, 2022

View reviewed changes

hawkw and others added 5 commits March 4, 2022 13:56

Merge branch 'main' into add_icon_asc_or_desc

762cacf

feat(console): Fix TableList trait

ee30549

Add const parameter for the width of every column.

feat(console): Add the methods for styling

b16c853

feat(console): Fix the style of all tables

b0e971f

chore(console): Remove the unnecessary method.

6bc5e6a

hawkw approved these changes Mar 7, 2022

View reviewed changes

hawkw enabled auto-merge (squash) March 8, 2022 03:07

Merge branch 'main' into add_icon_asc_or_desc

9f660ec

hawkw reviewed Mar 8, 2022

View reviewed changes

tokio-console/src/view/styles.rs Outdated Show resolved Hide resolved

Update tokio-console/src/view/styles.rs

a648f5e

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

auto-merge was automatically disabled March 8, 2022 06:33
Head branch was pushed to by a user without write access

hawkw enabled auto-merge (squash) March 8, 2022 17:11

Merge branch 'main' into add_icon_asc_or_desc

579c566

hawkw requested a review from a team as a code owner March 8, 2022 22:39

hawkw merged commit b9e0a22 into tokio-rs:main Mar 8, 2022

This was referenced Mar 9, 2022

chore(console): prepare to release v0.1.3 #309

Merged

Want to represent sorting state #300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(console): Add icon that sorting state #301

feat(console): Add icon that sorting state #301

nrskt commented Mar 4, 2022 •

edited

Loading

hawkw left a comment

hawkw Mar 4, 2022

nrskt Mar 4, 2022

nrskt Mar 4, 2022

hawkw Mar 4, 2022

nrskt Mar 4, 2022

hawkw Mar 4, 2022

nrskt Mar 5, 2022

hawkw left a comment

hawkw Mar 4, 2022

nrskt commented Mar 4, 2022

hawkw commented Mar 4, 2022

nrskt commented Mar 5, 2022

hawkw left a comment

hawkw Mar 7, 2022

hawkw Mar 7, 2022

nrskt commented Mar 8, 2022

hawkw commented Mar 8, 2022

	pub fn if_utf8<'a>(&self, utf8: &'a str, ascii: &'a str) -> &'a str {
	if self.utf8 {
	utf8
	} else {
	ascii
	}
	}

	pub fn warning_narrow(&self) -> Span<'static> {
	Span::styled(
	self.if_utf8("\u{26A0} ", "! "),
	self.fg(Color::LightYellow).add_modifier(Modifier::BOLD),
	)
	}

feat(console): Add icon that sorting state #301

feat(console): Add icon that sorting state #301

Conversation

nrskt commented Mar 4, 2022 • edited Loading

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrskt commented Mar 4, 2022

hawkw commented Mar 4, 2022

nrskt commented Mar 5, 2022

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrskt commented Mar 8, 2022

hawkw commented Mar 8, 2022

nrskt commented Mar 4, 2022 •

edited

Loading