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

Scope widget cherry-picked from binding-scroll branch. #1151

Merged
merged 5 commits into from
Aug 25, 2020

Conversation

rjwittams
Copy link
Collaborator

As discussed many times on Zulip.

Scope allows you to encapsulate data so local reactivity is possible without polluting your global app state.

Copy link
Collaborator

@jneem jneem left a comment

Choose a reason for hiding this comment

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

I was confused about a couple little things, but my main comment is that it would be really nice to have API docs for the public bits :)


impl<SP: ScopePolicy, W: Widget<SP::State>> Widget<SP::In> for Scope<SP, W> {
fn event(&mut self, ctx: &mut EventCtx, event: &Event, data: &mut SP::In, env: &Env) {
if self.widget_added {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the need for this check: not getting widget_added is bug, but it's a bug everywhere and we usually don't test for it explicitly. What goes wrong here that requires an explicit test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this may be a holdover from bugs with viewswitcher. Probably can go

druid/src/widget/scope.rs Show resolved Hide resolved
druid/src/widget/scope.rs Show resolved Hide resolved
@luleyleo
Copy link
Collaborator

I've not yet had the time to catch up on the Zulip discussion, and if I look at this right now, I have absolutely no idea what to do with it.

Some docs and an example would be great I think.

@rjwittams
Copy link
Collaborator Author

I was asked to extract it in its current state. Some of the motivating use cases are in the mentioned branch which I was told was too big to PR. I will do some docs eventually but that is why I did the PR.

@jneem
Copy link
Collaborator

jneem commented Aug 24, 2020

I understand that it will be convenient to do the example(s) later, but what about docs for the public API functions?

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

This seems good to me. Thanks for splitting it off! Also good to see the request_update propagation logic through Lens fixed.

/// # Examples
/// ```
/// #[derive(Data, Lens)]
/// struct AppState{
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor formatting nits, there seem to be spaces missing; rustfmt apparently doesn't catch issues inside doctests, which seems to me an oversight.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ran rustfmt on the example so it looks standardised now

@rjwittams rjwittams requested a review from jneem August 25, 2020 12:35
Copy link
Collaborator

@jneem jneem left a comment

Choose a reason for hiding this comment

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

Thanks for the nice docs! I found a missing apostrophe, but let's not hold this up for another CI cycle...

///
/// This is useful in circumstances where
/// * A (potentially reusable) widget is composed of a tree of multiple cooperating child widgets
/// * Those widgets communicate amongst themselves using Druids reactive data mechanisms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Druid's

@jneem jneem merged commit 2f9c692 into linebender:master Aug 25, 2020
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.

4 participants