Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Using Constraint::Min in in Table::widths results in constantly changing column layout. #499

Closed
medwards opened this issue Jun 19, 2021 · 8 comments
Labels

Comments

@medwards
Copy link

Description

I added the following constraints to a new table. These aren't the final values I want, so they're just preliminary to see what it looks like:

                 .widths(&[
                      tui::layout::Constraint::Min(2),
                      tui::layout::Constraint::Max(20),
                      tui::layout::Constraint::Max(10), // Commit date (truncates time by default)
                  ])

I end up seeing this:
Peek 2021-06-19 20-16

To Reproduce

The latest commit /w this problem in my application is at https://github.com/medwards/gitt/tree/buggy_table_constraints

You can also change examples/table.rs with the following patch:

index 846a55c..beda582 100644
--- a/examples/table.rs
+++ b/examples/table.rs
@@ -118,7 +118,7 @@ fn main() -> Result<(), Box<dyn Error>> {
                 .highlight_style(selected_style)
                 .highlight_symbol(">> ")
                 .widths(&[
-                    Constraint::Percentage(50),
+                    Constraint::Min(10),
                     Constraint::Length(30),
                     Constraint::Max(10),
                 ]);
diff --git a/src/text.rs b/src/text.rs
index 70a71d0..ae55981 100644
--- a/src/text.rs
+++ b/src/text.rs
@@ -183,13 +183,13 @@ impl<'a> Span<'a> {
 
 impl<'a> From<String> for Span<'a> {
     fn from(s: String) -> Span<'a> {
-        Span::raw(s)
+        Span::raw(s.replace("\r", "").replace("\n", ""))
     }
 }
 
 impl<'a> From<&'a str> for Span<'a> {
     fn from(s: &'a str) -> Span<'a> {
-        Span::raw(s)
+        Span::raw(s.replace("\r", "").replace("\n", ""))
     }
 }

And then:

$ cargo run --example table

Expected behavior

Minimally, I expected the column widths to remain the same if the rows and no other state change. In this case I expect there to be 30 characters spent for the right-most columns and whatever is left is used by the left-most column (ie Constraint::Min(n) means it should be at least n wide but will also use any remaining width available).

This might just be a docs snag, maybe Min means something else, but Constraint has no docs to explain otherwise.

Environment

  • OS: Linux
  • Terminal: gnome-terminal
  • Font: Ubuntu Mono Regular
  • Crate version: 0.14
  • Backend: crossterm
@medwards medwards added the bug label Jun 19, 2021
@medwards
Copy link
Author

medwards commented Jun 19, 2021

The root cause of this is likely #420. I confirmed this also happens with Max

How do I satisfy the solver and provide more precise constraints? Nested tables? It would be much more convenient to be able to combine these and I don't see how it is imprecise to have one column using Max or Min if the remaining columns have static widths (maybe #295 is necessary to make this precise?).

@medwards
Copy link
Author

medwards commented Jun 19, 2021

You can't nest Table into the row (because Text has no From impl for Table)

      tui::widgets::Row::new(vec![
          tui::widgets::Table::new(vec![tui::widgets::Row::new(vec!["message"])]),                                                     
          tui::widgets::Table::new(vec![tui::widgets::Row::new(vec!["author", "time"])]),
      ]) 

I don't think this is as simple as implementing the From impl either, since the sub-table needs the Rect in order to render itself.

Two tables is still possible, but now they need their own Rect and I need to re-do my "commit summary to Row" stuff. There's a lot of mess here to avoid angering the constraint solver.

@asomers
Copy link

asomers commented Jul 16, 2021

I agree that this is a bug in the constraint solver; your table specification isn't underconstrainted. As a workaround, have you tried switching one or more columns to Constraint::Length?

@medwards
Copy link
Author

Yes, I switched everything to use Length but a fixed length doesn't work even for me alone. I switch often from my desktop to my laptop and the available width changes.

I wrote some hot garbage to try to gracefully expand to fit larger windows: https://github.com/medwards/gitt/blob/column_width_calculator/src/main.rs#L88 (for context, this is supposed to be a tui gitk clone)

Basically I tried to write my own layout solver, found it to be super buggy, and gave up.

@fdehau
Copy link
Owner

fdehau commented Aug 1, 2021

This is a somewhat a known limitation of the current architecture of the crate. Under the hood, tui is using cassowary. For performance reasons, it does not provide deterministic results. But traditionally this solver is used in an incremental fashion: you compute a first layout, change a variable then get the updated layout. Therefore, you are not really exposed to all the other outcomes possible. Now, in the case of Table we were computing the layout every frame and in your case the constraints could be met with different solutions, hence the flickering.

Good news: because of an other issue I migrated Table to use the already existing Layout tool which caches layout computation results (#514). Therefore, for a given area and set of constraints you should see a single layout.
Bad news: the layout may still shift to something else when you restarts your app or if your table size changes. This can only be fixed by rethinking the layout algorithm.

My recommendation in any case would be to use more precise layout constraints (Length, Percent) as they will give you a more predictable output whatever the layout system :).

@medwards
Copy link
Author

medwards commented Aug 5, 2021

Bad news: the layout may still shift to something else when you restarts your app or if your table size changes. This can only be fixed by rethinking the layout algorithm.

I think this is fine.

My recommendation in any case would be to use more precise layout constraints (Length, Percent) as they will give you a more predictable output whatever the layout system :).

"Precise" is relative here. Percents work fine on big screens, but as the screen size gets smaller you need to make trade-offs, and some column widths are more disposable than others.

cassowary-rs keeps getting the blame in these tickets but their docs say

the constraints can be violated if necessary, with the order in which they are violated specified by setting a "strength" for each constraint. This allows the solution to gracefully degrade, which is useful for when a user interface needs to compromise on its constraints in order to still be able to display something.

The split function looks gnarly and I only skimmed it but notice only REQUIRED and WEAK constraints. Downgrading/upgrading or exposing the strength of the constraint might be the answer here. At minimum I'm not convinced this is a deficiency in cassowary-rs.

I notice a few other options here too, like expand_to_fill that I should experiment with.

@medwards
Copy link
Author

medwards commented Aug 7, 2021

The layout caching for tables is working fine, so going to close this bug as that was the original issue.

There's of course the broader issue of the tui::layout::Constraint not being expressive enough. In the end I just brought in cassowary to provide the constraints I wanted (WEAK percentage based bounds and STRONG + REQUIRED lower and upper bounds for known length columns like date times).

@fdehau
Copy link
Owner

fdehau commented Aug 8, 2021

cassowary-rs keeps getting the blame in these tickets

That was not my intention. My point was more that we are probably not using it the way it was intended for.

You made a good point about giving users more control over their constraints. I played a bit with the idea in #519. Let me know what you think if you have time :).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants