bug: dynamic alloc of layout constraints in task details screen #614

vrn21 · 2025-03-16T10:53:13Z

Changes I've made is for accommodating loong location names; ive changed the .. to the long name; it widens the task rectangle if its longer than the default (50%) and goes to the next line if its longer than what we can accommodate in a single line. This fixes issue #523 have attached screenshots of the same in the PR, before and after changes

These screenshots are what was the UI before my changes.

and these are the screenshots after my changes.

Changes i've made is for accomadating loong location names, ive changed the ... to the long name, it widens the task rectangle if its longer than the default 50% and goes to the next line if its longer than we can accomadate in a single line. This fixes issue tokio-rs#523 i have attached screenshots of the same in the PR, before and after changes

hds · 2025-03-17T14:02:37Z

@vrn21 Thanks for working on this topic! So far this PR looks good. However, with a fixed number of rows, when you go to 2 lines for the location text, the "Idle" time is getting dropped off, so that needs to be resolved too. I think that there are some hints in the original issue regarding how to approach that issue.

vrn21 · 2025-03-19T14:21:25Z

Thank you for looking into this @hds and Sorry for the delay,

but i'm still not sure how i'm supposed to move forward with this, i cant really determine what the height of the task_area is, before hand as in the current code, number of lines extra needed can be only computed after the each of the constraint is set. Could you help on this?

hds · 2025-03-19T15:17:55Z

In the code that exists, the areas are computed up front, based on the constraints provided. However, those areas aren't actually used until the Paragraph for each of the task overview and the waker stats are actually rendered at the bottom.

If the creation of the task and waker "widgets" (that's the variable name), is performed before the area is split on constraints, then we can already compute the number of lines that each one needs and use that as input to the Constraints::Length for the stats_area which is where they sit.

Does that help?

…ask details screen I've made the logic to look whether Location is longer than a single viewable length, even before setting constraints both vertically and horizontally. This fixes issue tokio-rs#523

vrn21 · 2025-03-21T14:43:58Z

Sorry again for the delay, Does this work @hds?

I've changed the logic to check whether locations is longer than what is viewable in a single line, before hand, assigning constraints (both vertical and horizontal) afterwards.

hds · 2025-03-22T09:53:53Z

This is looking good! There are a few style and layout things, but the functionality appears correct. I'll get you a proper review of this on Monday.

Thanks for your work!

hds

This is looking good, thanks! There are some changes I've suggested.

hds · 2025-03-24T09:45:07Z

tokio-console/src/view/task.rs

+        let first_line = location_lines_vector[0].clone();
+        location_lines_vector.remove(0);
+        let location_vector = vec![bold(location_heading), Span::raw(first_line)];


I think that we can use the reference here, can't we? We don't need to clone or manipulate the Vec.

Suggested change

let first_line = location_lines_vector[0].clone();

location_lines_vector.remove(0);

let location_vector = vec![bold(location_heading), Span::raw(first_line)];

let location_vector = vec![bold(location_heading), Span::raw(location_lines_vector[0])];

hds · 2025-03-24T09:45:49Z

tokio-console/src/view/task.rs

+        for line in location_lines_vector {
+            overview.push(Line::from(Span::raw(format!("    {}", line))));
+        }


Then here we can use the vec slice:

Suggested change

for line in location_lines_vector {

overview.push(Line::from(Span::raw(format!(" {}", line))));

}

for line in location_lines_vector[1..] {

overview.push(Line::from(Span::raw(format!(" {}", line))));

}

hds · 2025-03-24T09:57:31Z

tokio-console/src/view/task.rs

+
+        let location_heading = "Location: ";
+        let location_max_width = stats_area_check[0].width as usize - 2 - location_heading.len(); // NOTE: -2 for the border
+        let max_width_stats_area = area.width - 45;


Can we give the 45 a name? I'm guessing this is the minimum waker area needed?

hds · 2025-03-24T10:17:13Z

tokio-console/src/view/task.rs

+            .chunks(max_width_stats_area as usize)
+            .map(|chunk| chunk.iter().collect())
+            .collect();
+        let no_of_lines_extra_required_to_accomadate_location = location_lines_vector.len() - 1;


These very long variable names don't really fit in with the style of the rest of the codebase, we probably don't need the extra variable at all.

Instead, why don't we extract out the task stats height into a variable:

Suggested change

let no_of_lines_extra_required_to_accomadate_location = location_lines_vector.len() - 1;

// Id, Name, Target, Location (multiple), total, busy, scheduled, and idle times + top/bottom borders

let task_stats_height = 9 + location_lines_vector.len();

hds · 2025-03-24T11:01:58Z

tokio-console/src/view/task.rs

-        ) = if warnings.is_empty() {
-            let chunks = Layout::default()
-                .direction(layout::Direction::Vertical)
+        ) = if task.location().len() > location_max_width {


We definitely don't want to add another layer of if-else statements here. I think that be specifying the task stats height outside, we can leave this like it was (just depending on the presense of warnings).

hds · 2025-03-24T11:02:23Z

tokio-console/src/view/task.rs

@@ -34,7 +34,6 @@ impl TaskView {
    pub(crate) fn update_input(&mut self, _event: input::Event) {
        // TODO :D
    }
-


I don't think we want to remove this line.

hds · 2025-03-24T11:04:35Z

tokio-console/src/view/task.rs

+        let location_vector = vec![bold(location_heading), Span::raw(first_line)];
+        overview.push(Line::from(location_vector));
+        for line in location_lines_vector {
+            overview.push(Line::from(Span::raw(format!("    {}", line))));


I think we need to add more spaces here so that the beginning of the text aligns with the first line location text.

If you leave only 4 spaces here, then the chunks for the 2nd line and onward should be bigger than the first chunk (which is also an option).

vrn21 · 2025-03-24T12:40:35Z

@hds i've made the changes you've told to

hds

One point here where I think I wasn't clear regarding what I meant. And one other thing that I didn't notice before, sorry about that.

tokio-console/src/view/task.rs

hds · 2025-03-24T13:42:27Z

tokio-console/src/view/task.rs

@@ -83,7 +94,7 @@ impl TaskView {
                        // controls
                        layout::Constraint::Length(controls.height()),
                        // task stats
-                        layout::Constraint::Length(10),
+                        layout::Constraint::Length((10 + location_lines_vector.len() - 1) as u16),


And use that variable here:

Suggested change

layout::Constraint::Length((10 + location_lines_vector.len() - 1) as u16),

layout::Constraint::Length(task_stats_height),

hds · 2025-03-24T13:42:46Z

tokio-console/src/view/task.rs

@@ -105,7 +116,7 @@ impl TaskView {
                        // warnings (add 2 for top and bottom borders)
                        layout::Constraint::Length(warnings.len() as u16 + 2),
                        // task stats
-                        layout::Constraint::Length(10),
+                        layout::Constraint::Length((10 + location_lines_vector.len() - 1) as u16),


And here too:

Suggested change

layout::Constraint::Length((10 + location_lines_vector.len() - 1) as u16),

layout::Constraint::Length(task_stats_height),

hds · 2025-03-24T13:47:20Z

tokio-console/src/view/task.rs

+        let stats_area = if location_lines_vector.len() != 1 {
+            let area_needed_to_render_location = task.location().len() as u16;
+            Layout::default()
+                .direction(layout::Direction::Horizontal)
+                .constraints(
+                    [
+                        layout::Constraint::Min(area_needed_to_render_location + 15), //Note: 15 is the length of "| Location:     |"
+                        layout::Constraint::Min(32),
+                    ]
+                    .as_ref(),
+                )
+                .split(stats_area)
+        } else {
+            Layout::default()
+                .direction(layout::Direction::Horizontal)
+                .constraints(
+                    [
+                        layout::Constraint::Percentage(50),
+                        layout::Constraint::Percentage(50),
+                    ]
+                    .as_ref(),
+                )
+                .split(stats_area)
+        };


We can simplify this a bit by just setting different constraints:

Suggested change

let stats_area = if location_lines_vector.len() != 1 {

let area_needed_to_render_location = task.location().len() as u16;

Layout::default()

.direction(layout::Direction::Horizontal)

.constraints(

[

layout::Constraint::Min(area_needed_to_render_location + 15), //Note: 15 is the length of "| Location: |"

layout::Constraint::Min(32),

]

.as_ref(),

)

.split(stats_area)

} else {

Layout::default()

.direction(layout::Direction::Horizontal)

.constraints(

[

layout::Constraint::Percentage(50),

layout::Constraint::Percentage(50),

]

.as_ref(),

)

.split(stats_area)

};

let stats_constraints = if location_lines_vector.len() > 1 {

[

// 15 is the length of "| Location: |"

layout::Constraint::Min(area_needed_to_render_location + 15),

layout::Constraint::Min(32),

]

} else {

[

layout::Constraint::Percentage(50),

layout::Constraint::Percentage(50),

]

};

let stats_area = Layout::default()

.direction(layout::Direction::Horizontal)

.constraints(stats_constraints.as_ref())

.split(stats_area);

please refer to earlier commits to see what changes have been made

vrn21 · 2025-03-24T17:31:52Z

@hds i've updated the latest changes requested

tokio-console/src/view/task.rs

hds · 2025-03-25T08:58:49Z

tokio-console/src/view/task.rs

+            let area_needed_to_render_location = task.location().len() as u16;
+            [
+                // 15 is the length of "| Location:     |"
+                layout::Constraint::Min(area_needed_to_render_location + 15),


What happens when the 2 Min constraints add up to more than the available width?

from this, i think the view would overflow according to the length the view has.

I think the if statement is incorrect then (and also back to front). In fact, I think that if instead the constraints look just like this then it might be enough (always, no if statement at all). Could you try and show some screenshots?

[ layout::Constraint::Min(location_lines_vector[0].len()), layout::Constraint::Min(32), ]

long var names changed to shorter names Co-authored-by: Hayden Stainsby <[email protected]>

hds

What about this idea?

hds · 2025-03-25T14:43:50Z

tokio-console/src/view/task.rs

+            let area_needed_to_render_location = task.location().len() as u16;
+            [
+                // 15 is the length of "| Location:     |"
+                layout::Constraint::Min(area_needed_to_render_location + 15),


I think the if statement is incorrect then (and also back to front). In fact, I think that if instead the constraints look just like this then it might be enough (always, no if statement at all). Could you try and show some screenshots?

[ layout::Constraint::Min(location_lines_vector[0].len()), layout::Constraint::Min(32), ]

vrn21 requested a review from a team as a code owner March 16, 2025 10:53

bug: idle line visible after dynamic alloc of layout constraints in t…

0f79e65

…ask details screen I've made the logic to look whether Location is longer than a single viewable length, even before setting constraints both vertically and horizontally. This fixes issue tokio-rs#523

hds requested changes Mar 24, 2025

View reviewed changes

vrn21 force-pushed the main branch from 4b4609d to b04ad50 Compare March 24, 2025 12:47

hds requested changes Mar 24, 2025

View reviewed changes

bug: dynamic alloc of layout constraints, requested changes changed

2e5a512

please refer to earlier commits to see what changes have been made

vrn21 force-pushed the main branch from b04ad50 to 2e5a512 Compare March 24, 2025 17:31

vrn21 requested a review from hds March 25, 2025 06:46

hds reviewed Mar 25, 2025

View reviewed changes

bug fix: dynamic alloc of layout constraints in task details screen,

93319d5

long var names changed to shorter names Co-authored-by: Hayden Stainsby <[email protected]>

hds reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: dynamic alloc of layout constraints in task details screen #614

bug: dynamic alloc of layout constraints in task details screen #614

vrn21 commented Mar 16, 2025 •

edited

Loading

hds commented Mar 17, 2025

vrn21 commented Mar 19, 2025 •

edited

Loading

hds commented Mar 19, 2025

vrn21 commented Mar 21, 2025

hds commented Mar 22, 2025

hds left a comment

hds Mar 24, 2025

hds Mar 24, 2025

hds Mar 24, 2025

hds Mar 24, 2025

hds Mar 24, 2025

hds Mar 24, 2025

hds Mar 24, 2025

vrn21 commented Mar 24, 2025

hds left a comment

hds Mar 24, 2025

hds Mar 24, 2025

hds Mar 24, 2025

vrn21 commented Mar 24, 2025

hds Mar 25, 2025

vrn21 Mar 25, 2025

hds Mar 25, 2025

hds left a comment

hds Mar 25, 2025

	let no_of_lines_extra_required_to_accomadate_location = location_lines_vector.len() - 1;
	// Id, Name, Target, Location (multiple), total, busy, scheduled, and idle times + top/bottom borders
	let task_stats_height = 9 + location_lines_vector.len();

	layout::Constraint::Length((10 + location_lines_vector.len() - 1) as u16),
	layout::Constraint::Length(task_stats_height),

bug: dynamic alloc of layout constraints in task details screen #614

Are you sure you want to change the base?

bug: dynamic alloc of layout constraints in task details screen #614

Conversation

vrn21 commented Mar 16, 2025 • edited Loading

hds commented Mar 17, 2025

vrn21 commented Mar 19, 2025 • edited Loading

hds commented Mar 19, 2025

vrn21 commented Mar 21, 2025

hds commented Mar 22, 2025

hds 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

vrn21 commented Mar 24, 2025

hds 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

vrn21 commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrn21 commented Mar 16, 2025 •

edited

Loading

vrn21 commented Mar 19, 2025 •

edited

Loading