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

feat(layout)!: Change Flex::default() #881

Merged
merged 11 commits into from
Jan 29, 2024
Merged

Conversation

kdheepak
Copy link
Collaborator

@kdheepak kdheepak commented Jan 28, 2024

This PR makes a number of simplifications to the layout and constraint features that were added after v0.25.0.

For users upgrading from v0.25.0, the net effect of this PR (along with the other PRs) is the following:

  • New Flex modes have been added.
    • Flex::Start (new default)
    • Flex::Center
    • Flex::End
    • Flex::SpaceAround
    • Flex::SpaceBetween
    • Flex::Legacy (old default)
  • Min(v) grows to allocate excess space in all Flex modes instead of shrinking (except in Flex::Legacy where it retains old behavior).
  • Fill(1) grows to allocate excess space, growing equally with Min(v).

The following contains a summary of the changes in this PR and the motivation behind them.

Flex

  • Removes Flex::Stretch
  • Renames Flex::StretchLast to Flex::Legacy

Constraint

  • Removes Fixed
  • Makes Min(v) grow as much as possible everywhere (except Flex::Legacy where it retains the old behavior)
  • Makes Min(v) grow equally as Fill(1) while respecting Min lower bounds. When Fill and Min are used together, they both fill excess space equally.

Allowing Min(v) to grow still allows users to build the same layouts as before with Flex::Start with no breaking changes to the behavior.

This PR also removes the unstable feature SegmentSize.

This is a breaking change to the behavior of constraints. If users want old behavior, they can use Flex::Legacy.

Layout::vertical([Length(25), Length(25)]).flex(Flex::Legacy)

Users that have constraint that exceed the available space will probably not see any difference or see an improvement in their layouts. Any layout with Min will be identical in Flex::Start and Flex::Legacy so any layout with Min will not be breaking.

Previously, Table used EvenDistribution internally by default, but with that gone the default is now Flex::Start. This changes the behavior of Table (for the better in most cases). The only way for users to get exactly the same as the old behavior is to change their constraints. I imagine most users will be happier out of the box with the new Table default.

Resolves #843

Thanks to @joshka for the direction

Background

This is a follow up to the following PRs:

Flex::Stretch was introduced as a stable alternative to Flex::StretchLast, as well as a backward compatible way to make SegmentSize::EvenDistribution work.
Constraint::Fixed was introduced to fix column spacing.

Motivation

Typically, there's two ways a layout's constraints are not met:

  • When there's too much space

e.g. there's a 100px wide space but a user is only requesting for a total of 75:

image image
  • When there's not enough space

e.g. there's a 50px wide space but a user is requesting for a total of 75:

image

In Stretch flex modes, constraints were usually violated in both cases.

This caused instability, and needed constraints like Fixed to ensure that some lengths were prioritized over others (e.g. we typically want table column spacing should take precedence over column constraints provided by users).

In Flex::Start, Flex::Center, Flex::End, Flex::SpaceBetween, Flex::SpaceAround, when there's excess space constraints are not violated:

flex-too-much-space.mov

When there's no enough space constraints are still violated:

image

But the priority order of constraints allows users to control the layout as they see fit.

TLDR: Flex::Stretch broke more constraints more often than other layouts and with Min growing, it has marginal utility over other Flex modes, and we can remove it.


Another change in this PR is the behavior of Min. Now Min grows in all Flex layouts (except Flex::Legacy).
This allows Flex::Stretch like behavior with Min(0) instead. e.g. the following two behave identically for any available space:

- Layout::horizontal([Length(25), Length(25), Length(25)]).flex(Flex::Stretch)
+ Layout::horizontal([Min(0), Min(0), Min(0)])

Previously, Min(v) would always try to collapse to the value v in every Flex mode, but because of Stretch semantics it would grow to fill remaining space. This behavior made people use Min(0) as a space filler. e.g.

let [header, main] = area.split(&Layout::vertical([Length(3), Min(0)]);

This PR makes this "space filler" growing behavior the default in all Flex layouts (except Flex::Legacy for backwards compatibility reasons), i.e. Min(v) does not collapse to the value v but instead tries to grow to the area size with low priority.

Current main branch:

image image

This PR:

Flex::Legacy vs Flex::Start

image image

Min(v) has another behavior which is that is limits the lower bound on the segment size with the user provided value. Previously, breaking a Min(v) constraint was more "costly" than Length, Percentage, Ratio etc, so people also used it like this:

let [main, input] = area.split(&Layout::vertical([Percentage(100), Min(3)]);

This PR retains that behavior too:

image

The mechanics by which this behavior is achieved is by ordering the weights for the constraints appropriately.

TLDR: This effectively means that in any layout with Min, there shouldn't be any breaking change that a user experiences between Flex::Legacy (old default) and Flex::Start (new default).

Additionally, in this PR, Min growing is weighted equally to Fill growing, which means you can get desirable behavior like this:

min-grow-fill-spacing.mov

Since we are removing Flex::Stretch, we are also removing SegmentSize::EvenDistribution. Since SegmentSize is unstable, this PR removes SegmentSize entirely.

This PR also removes Fixed since Fixed was only necessary to have something higher priority in Stretch layouts. Without Stretch we don't need it anymore.
With the refactor in #788 spacers are used instead of Fixed for column spacing, and these spacers have a high priority. This makes Fixed unnecessary.

@kdheepak kdheepak changed the title feat!(layout): Change Flex::default() to Flex::Stretch feat(layout)!: Change Flex::default() to Flex::Stretch Jan 28, 2024
@ratatui ratatui deleted a comment from github-actions bot Jan 28, 2024
@github-actions github-actions bot added the Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc. label Jan 28, 2024
@kdheepak kdheepak changed the title feat(layout)!: Change Flex::default() to Flex::Stretch feat(layout)!: Change Flex::default() Jan 29, 2024
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (1cbe1f5) 92.2% compared to head (872a602) 92.1%.
Report is 2 commits behind head on main.

Files Patch % Lines
src/layout/layout.rs 93.0% 3 Missing ⚠️
src/widgets/table/table.rs 92.8% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #881     +/-   ##
=======================================
- Coverage   92.2%   92.1%   -0.1%     
=======================================
  Files         61      60      -1     
  Lines      15977   15819    -158     
=======================================
- Hits       14731   14573    -158     
  Misses      1246    1246             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kdheepak kdheepak marked this pull request as ready for review January 29, 2024 10:47
@kdheepak
Copy link
Collaborator Author

I ran every example in the examples folder and with no changes to the code no UI was broken as far as I can tell, which I think is a good sign that Flex::Start with Min growing is fine as a default?

I imagine there will be some users that depend on Flex::Legacy as the default but they can opt into that with .flex(Flex::Legacy).

I also updated taskwarrior-tui to use this branch and the UI looks identical to this without any changes to the Layout, which is again a good sign.

This was the only related diff to get my code to compile:

        ccs.push(match *constraint {
          Constraint::Length(v) => ...,
          Constraint::Percentage(v) => ...,
          Constraint::Ratio(n, d) => ...,
          Constraint::Min(v) => ...,
          Constraint::Max(v) => ...,
+         Constraint::Fill(v) => ...,
        });

@kdheepak kdheepak added this to the v0.26.0 milestone Jan 29, 2024
Copy link
Member

@Valentin271 Valentin271 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, had a small comment but I'm find merging as is.

We might want (in a later PR) to rework some doc examples to not use Legacy.

For the merger: don't forget to rework the commit description. Btw great description @kdheepak

src/layout/layout.rs Outdated Show resolved Hide resolved
@kdheepak kdheepak merged commit 540fd2d into main Jan 29, 2024
38 checks passed
@kdheepak kdheepak deleted the kd/flex-stretch-default branch January 29, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate StretchLast and choose new default for Flex
3 participants