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

Add pane maximize / restore for PaneGrid #1504

Merged
merged 9 commits into from
Nov 8, 2022

Conversation

tarkah
Copy link
Member

@tarkah tarkah commented Nov 3, 2022

Adds the ability to maximize / restore a pane in the pane grid. When maximized, dragging is disabled (we can remove this, since dragging is enabled when only 1 pane exists currently).

API

Two new methods are exposed on State to allow the user control of maximizing and restoring a pane:

/// Maximize the given [`Pane`]. Only this pane will be rendered by the
/// [`PaneGrid`] until [`Self::restore()`] is called.
pub fn maximize(&mut self, pane: &Pane);

/// Restore the currently maximized [`Pane`] to it's normal size. All panes
/// will be rendered by the [`PaneGrid`]
pub fn restore(&mut self);

Once a pane is maximized, a new bool is passed on the view function to specify if the pane is maximized or not. Maybe we create an enum here for a stronger type since the closure is not descriptive:

PaneGrid::new(&self.panes, |id, pane, is_maximized| {
    ..
})

Implementation

State stores the maximized pane in a new field: maximized: Option<Pane> and is set by the above API.

Instead of storing Vec<Content> in the PaneGrid, I've created a new helper Contents that is built considering the maximized pane. This can be iterated over for building the widget tree & layout.

pub enum Contents<T> {
    All(Vec<(Pane, T)>),
    Maximized(Pane, T),
}

A new state::Scoped has been added which either wraps the internal state when no field is maximized, or specifies the maximized pane. The widget methods now take this for determining the layout Node of the PaneGrid.

/// The scoped internal state of the [`PaneGrid`]
pub enum Scoped<'a> {
    /// The state when all panes are visible
    All(&'a Internal),
    /// The state when a pane is maximized
    Maximized(Node),
}

impl<'a> Scoped<'a> {
    /// The layout [`Node`] of the [`Scope`] state
    pub fn layout(&self) -> &Node {
        match self {
            Scoped::Normal(Internal { layout, .. }) => layout,
            Scoped::Maximized(layout) => layout,
        }
    }
}

Iterating on Contents and calling Scoped::layout allowed for very minimal adjustments of the existing widget implementation code.

Demo

simplescreenrecorder-2022-11-02_17.13.37.mp4

@casperstorm
Copy link
Member

This is a great addition to the pane grid.
Code change is easy to follow and the example worked as expected.

@hecrj hecrj added feature New feature or request widget layout labels Nov 8, 2022
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Looking good! Just one suggestion!

Comment on lines 950 to 970
pub fn iter(&self) -> Box<dyn Iterator<Item = (Pane, &T)> + '_> {
match self {
Contents::All(contents) => Box::new(
contents.iter().map(|(pane, content)| (*pane, content)),
),
Contents::Maximized(pane, content) => {
Box::new(std::iter::once((*pane, content)))
}
}
}

fn iter_mut(&mut self) -> Box<dyn Iterator<Item = (Pane, &mut T)> + '_> {
match self {
Contents::All(contents) => Box::new(
contents.iter_mut().map(|(pane, content)| (*pane, content)),
),
Contents::Maximized(pane, content) => {
Box::new(std::iter::once((*pane, content)))
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Great! We avoid a lot of code changes with this API :) Iterators are pretty nifty.

Comment on lines 132 to 133
Contents::Maximized(pane, view(pane, pane_state, true)),
state::Scoped::Maximized(Node::Pane(pane)),
Copy link
Member

Choose a reason for hiding this comment

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

These seem inherently coupled, but the current data model allows us to potentially have desync. Could we simply merge Scoped with Contents?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Good call, fixed in 7de9d24

@hecrj hecrj added this to the 0.5.0 milestone Nov 8, 2022
@tarkah tarkah requested a review from hecrj November 8, 2022 16:50
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Awesome! Let's sail 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request layout widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants