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

xilem_core: Refactor Memoization (add Frozen view, fix force-rebuild in Arc, and cleanup Memoize) #451

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

Philipp-M
Copy link
Contributor

@Philipp-M Philipp-M commented Jul 27, 2024

This adds a force which is basically a specialized Memoize without any bound data.
This also cleans up Memoize a little bit (mostly code-aesthetic stuff, but also adds State and Action params to the view, so that it doesn't offer weird surprises when composing it with views like Adapt).

The Arc view is also fixed, as it didn't support force rebuilding yet, which is relevant for e.g. async, e.g. the MemoizedAwait view would not work inside an Arc<impl View> currently.

xilem_core/src/views/memoize.rs Outdated Show resolved Hide resolved
xilem_core/src/view.rs Show resolved Hide resolved
}

/// Specialized version of [`memoize`], which doesn't take any data at all, the closure is evaluated only once and when a child view forces a rebuild
pub fn static_view<State, Action, Context, Message, V, InitView>(
Copy link
Member

Choose a reason for hiding this comment

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

I thought against this in Bevy, and I'm going to fight against this here; we do not want components to have names ending in _view .

I know that we can't use static because that's a keyword. Maybe eternal works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eternal sounds epic at least^^ Yeah I'm also not happy with static_view ideal would be static or something. Though eternal probably suggests that this will live forever...

Copy link
Member

Choose a reason for hiding this comment

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

static also suggests it will live forever. We definitely can't end up in a state where this is _view 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also true... Yeah finding good names is hard, probably the hardest thing in programming^^ I'll try to think about it, but I agree, we should remove _view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, what about frozen? I've pushed that for now. Other ideas were fixed, immutable invariant (so of course still open for discussion).

Copy link
Contributor

Choose a reason for hiding this comment

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

I like frozen and immutable.
fixed reminds me more of positioning like in CSS position: fixed

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Some code style suggestions, actual PR content looks good!

xilem_core/src/views/memoize.rs Outdated Show resolved Hide resolved
phantom: PhantomData<fn() -> (State, Action)>,
}

/// Specialized version of [`memoize`], which doesn't take any data at all, the closure is evaluated only once and when a child view forces a rebuild
Copy link
Member

Choose a reason for hiding this comment

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

We should explain this in terms of why you would use it, not in reference to any other value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably skim again, I also added an example doc-comment for Memoize

xilem/examples/memoization.rs Outdated Show resolved Hide resolved
@Philipp-M Philipp-M changed the title xilem_core: Refactor Memoization (add Static view, fix force-rebuild in Arc, and cleanup Memoize) xilem_core: Refactor Memoization (add Frozen view, fix force-rebuild in Arc, and cleanup Memoize) Aug 1, 2024
@Philipp-M Philipp-M added this pull request to the merge queue Aug 1, 2024
Merged via the queue into linebender:main with commit 210afb4 Aug 1, 2024
16 checks passed
@Philipp-M Philipp-M deleted the xilem-refactor-memoization branch August 1, 2024 11:29
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.

3 participants