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

Fix: cassowary/layouts: add extra constraints for fixing Min(v)/Max(v) combination. #31

Merged
merged 2 commits into from
Mar 11, 2023

Conversation

orhun
Copy link
Member

@orhun orhun commented Feb 12, 2023

Upstream: #647

Description

These added weak constraints prevent zero width/height widgets when combining Min(v)/Max(v) together in a chunk.

The UI will now reflow correctly when (re)sizing, gracefully preserving Max(v) constraints as much as possible.

This fixes the root cause of the issues identified in:
fdehau/tui-rs#39
fdehau/tui-rs#420
(amongst probably similar reports).

Testing guidelines

Combine Min(v) and Max(v) layout constraints in a chunk.

...
        let chunks = layout::Layout::default()
            .direction(layout::Direction::Horizontal)
            .constraints([Constraint::Min(50), Constraint::Max(25)])
            .split(frame.size());
...

Use any other combination, including Percentage/Ratio/Length with Min/Max.

Resize the window width/height to investigate how the two constraints are now satisfied without 'freaking out' and generating zero-width/height widgets. If all Min(v)/Max(v) constraints cannot be satisfied, they will gracefully degrade.

Checklist

These added weak constraints prevent zero width/height widgets when combining Min(v)/Max(v) together in a chunk.

The UI will now reflow correctly when (re)sizing, gracefully preserving Max(v) constraints as much as possible.
@mindoodoo mindoodoo added the Type: Bug Something isn't working label Feb 14, 2023
@mindoodoo mindoodoo added this to the First Release milestone Feb 19, 2023
Copy link
Member

@sayanarijit sayanarijit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally. Looks good.

@orhun
Copy link
Member Author

orhun commented Mar 11, 2023

LGTM! (cannot approve my own PR)

@sayanarijit sayanarijit merged commit 0dc3943 into main Mar 11, 2023
@sayanarijit sayanarijit deleted the 647/master branch March 11, 2023 11:41
joshka pushed a commit that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants