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

Expose scope state #1607

Closed
wants to merge 1 commit into from
Closed

Conversation

rjwittams
Copy link
Collaborator

This is mainly for composite widgets that contain a scope: using this they can affect the widgets inside the scope.
Using this facilty, allow getting and setting the tab index of a Tabs widget.
This offers part of a solution to #1390, although it is in terms of the index and not the key.

This is mainly for composite widgets that contain a scope: using this they can affect the widgets inside the scope.
Using this facilty, allow getting and setting the tab index of a Tabs widget.
This offers part of a solution to linebender#1390, although it is in terms of the index and not the key.
Copy link
Member

@cmyr cmyr 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! I tweaked some docs, maybe double-check that I got things right?

The main thing with the docs is having a short first line, or they render weirdly in rustdoc.

@@ -20,6 +20,8 @@ You can find its changes [documented below](#070---2021-01-01).
- WindowSizePolicy: allow windows to be sized by their content ([#1532] by [@rjwittams])
- Implemented `Data` for more datatypes from `std` ([#1534] by [@derekdreery])
- Shell: windows implementation from content_insets ([#1592] by [@HoNile])
- Scope: expose scoped state using state() and state_mut() ([#XXXX] by [@rjwittams]
- Tabs: allow getting and setting the tab index of a Tabs widget ([#XXXX] by [@rjwittams]
Copy link
Member

Choose a reason for hiding this comment

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

will want to fix this up before merging

Comment on lines +217 to +218
/// This allows you to access the content of the Scopes state from
/// outside the widget.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This allows you to access the content of the Scopes state from
/// outside the widget.
/// A reference to the contents of the `Scope`'s state.
///
/// This allows you to access the content from outside the widget.

Comment on lines +227 to +230
/// This allows you to mutably access the content of the Scopes state from
/// outside the widget. Mainly useful for composite widgets. Note that
/// if you modify the state through this reference, the Scope will not
/// call update on its children until the next event it receives.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This allows you to mutably access the content of the Scopes state from
/// outside the widget. Mainly useful for composite widgets. Note that
/// if you modify the state through this reference, the Scope will not
/// call update on its children until the next event it receives.
/// A mutable reference to the contents of the [`Scope`]'s state.
///
/// This allows you to mutably access the content of the `Scope`' s state from
/// outside the widget. Mainly useful for composite widgets.
///
/// # Note:
///
/// If you modify the state through this reference, the Scope will not
/// call update on its children until the next event it receives.

@@ -896,6 +906,12 @@ impl<TP: TabsPolicy> Tabs<TP> {
self
}

/// Return this Tabs widget with the specified (zero based) tab index
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Return this Tabs widget with the specified (zero based) tab index
/// A builder-style method to specify the (zero-based) index of the selected tab.

(is this correct?)

TP::add_tab(tabs, name, child)
} else {
tracing::warn!("Can't add static tabs to a running or complete tabs instance!")
}
}

fn make_scope(&self, tabs_from_data: TP) -> WidgetPod<TP::Input, TabsScope<TP>> {
/// Get the currently selected (zero-based) tab index.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get the currently selected (zero-based) tab index.
/// The (zero-based) index of the currently selected tab.

Comment on lines +943 to +945
/// Set the selected (zero-based) tab index, this tab will become visible if it exists.
/// Animated transitions will apply as if clicking the tab bar if they are enabled and the
/// Tabs widget is laid out.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Set the selected (zero-based) tab index, this tab will become visible if it exists.
/// Animated transitions will apply as if clicking the tab bar if they are enabled and the
/// Tabs widget is laid out.
/// Set the selected (zero-based) tab index.
///
/// This tab will become visible if it exists. If animations are enabled
/// (and the widget is laid out), the tab transition will be animated.

@rjwittams
Copy link
Collaborator Author

Cheers for the comments, I'll have a look later on!

@PoignardAzur
Copy link
Collaborator

@rjwittams Can you integrate Colin's changes and rebase this branch?

If you don't work on this anymore, I can also take ownership of the PR and do the changes myself.

Either way, I think this is ready to merge, minor changes aside. Colin seemed to approve, and the project hasn't really changed direction since.

@maan2003 maan2003 mentioned this pull request Dec 7, 2021
@jneem
Copy link
Collaborator

jneem commented Dec 30, 2021

I think this is superseded by #2082.

@jneem jneem closed this Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants