Skip to content

Change Widget::update to mutate the Widget::State rather than return a new one. Move input capturing methods from Widget trait to UiCell. #595

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

Merged
merged 6 commits into from
Oct 19, 2015

Conversation

mitchmindtree
Copy link
Contributor

This change addresses the problem highlighted in #554 where large widgets would be required to allocate an entire new Widget::State even for the slightest of changes. This is because the only way of updating the state was to return an entirely new state from the Widget::update method.

This PR changes the Widget::update method so that it no longer returns an Option<Widget::State> but instead provides a way of mutating the state via the State wrapper which is given via the UpdateArgs. All fields not relating to the unique widget state have been moved out of the State type and into a CommonState type. This allows us to use the State type purely as a "cell"-like wrapper around the WidgetState.

State::view provides immutable access to the Widget::State.
State::update allows for mutation of the Widget::State. When called, it sets a has_updated flag, allowing us to check whether or not the Widget::State was updated at all following the call of Widget::update and in turn allowing us to know whether or not we need to produce a new Element for the Widget.

This change also required moving the input capturing methods from the Widget trait to the UiCell, to be called within the Widget::update if necessary.

let state_mut_ptr: *mut State<W::State> = &mut state;
widget.update(UpdateArgs {
idx: idx,
state: unsafe { ::std::mem::transmute(state_mut_ptr) },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This really shouldn't be required, however I couldn't find any other way to appease the borrow checker. Perhaps there is something I'm missing? Would be nice if someone could review and see if they can get it working without the unsafe. @bvssvni ?

Copy link
Member

Choose a reason for hiding this comment

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

Checking now.

Copy link
Member

Choose a reason for hiding this comment

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

I've reduced it http://is.gd/Xu6jc0

Copy link
Member

Choose a reason for hiding this comment

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

Here is the solution: http://is.gd/LEoLyJ

You need to add another lifetime UpdateArgs<'a, 'b: 'a, W, C: 'a> and pass it in to pub state: &'a mut State<'b, W::State>. Notice that the second lifetime 'b inherits the first 'a such that 'b always outlive 'a.

This error happened because UpdateArgs moved, and the compiler only has 'a to reason about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woah, great stuff @bvssvni ! I tried multiple lifetimes but didn't consider bounding the second lifetime by the first, though it does make a lot of sense :) thanks! I'll implement the change now

@mitchmindtree
Copy link
Contributor Author

All passing unsafe-less 😸 going to merge!

Published as v0.21.0.

mitchmindtree added a commit that referenced this pull request Oct 19, 2015
Change Widget::update to mutate the Widget::State rather than return a new one. Move input capturing methods from Widget trait to UiCell.
@mitchmindtree mitchmindtree merged commit 68be3b0 into PistonDevelopers:master Oct 19, 2015
@mitchmindtree mitchmindtree deleted the update_mutate branch October 23, 2015 03:23
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.

2 participants