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 UpdateCtx::env_changed and UpdateCtx::env_key_changed #1207

Merged
merged 3 commits into from
Sep 16, 2020

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Sep 9, 2020

This is my attempt to address #1167.

I'm took one of the alternative approaches that came up in that discussion, which is to just include the previous Env in the UpdateCtx, and expose methods there for checking for changes. This goes slightly further and also includes an extra reference to the current Env in UpdateCtx; this is just for ergonomics, so that you can check for changes without needing to pass the current Env explicitly.

This has one other 'just for ergonomics' addition; I've added a trait, EnvResolveable, that encapsulates the idea of a type that can check if it has changed between two versions of an Env. The rationale here is that it is implemented both just for Key as well as for KeyOrValue; I also hope to expand it in future work to encapsulate the idea of a type that can compute a value of some type from the Env, which will be useful in some cases for synthesizing env values.

This also includes updates to TextLayout and Label in order to actually use this functionality, but it does not adopt it anywhere else; I'll either do that as part of a follow-up or I'll open an issue outlining the work.

I'd also like to update the docs of the Widget::update method in order to cover handling changes in the Env.

@luleyleo luleyleo added the S-needs-review waits for review label Sep 10, 2020
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.

As discussed extensively on Zulip, I'm approving on the basis that this should improve performance in real cases, and the API seems pretty reasonable. There is more architecture work needed on env, but that's for a future cycle.

druid/src/env.rs Outdated
///
/// [`Env`]: struct.Env.html
/// [`KeyOrValue`]: enum.KeyOrValue.html
pub trait EnvResolveable<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

A naming quibble: using adjectives for interface names feels Java-ish. I think a more Rust idiomatic name is EnvResolve. I don't feel strongly about this, and have definitely been tempted by adjectives myself, especially for more abstract interfaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel you, and I struggled to come up with another name. 'Resolve' does not sound right to me– perhaps EnvResolver would be better. I suppose you don't like Enviable either then, huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now KeyLike. 🤷


impl<T: ValueType> KeyLike<T> for Key<T> {
fn changed(&self, old: &Env, new: &Env) -> bool {
!old.get_untyped(self).same(new.get_untyped(self))

Choose a reason for hiding this comment

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

I've got a panic on this line.
I'm not sure if I am doing anything wrong, but I think this would be better written as:

Suggested change
!old.get_untyped(self).same(new.get_untyped(self))
match (old.try_get_untyped(self), new.try_get_untyped(self)) {
(Err(_), Err(_)) => false,
(Err(_), Ok(_)) => true,
(Ok(_), Err(_)) => true,
(Ok(old_value), Ok(new_value)) => !old_value.same(new_value),
}

fn changed(&self, old: &Env, new: &Env) -> bool {
match self {
KeyOrValue::Concrete(_) => false,
KeyOrValue::Key(key) => !old.get_untyped(key).same(new.get_untyped(key)),

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

huh, is your panic that some key is missing from the env?

Choose a reason for hiding this comment

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

Yeah, pretty sure that's it.

I am trying to change the color of a menu label based on the current selection and hot state.
I might be doing something wrong, because I'm in the middle of a upgrade to the latest master (including this PR) and haven't yet looked at all the changes and only tried to get everything in a working state again.

I've created a somewhat "minimal" example repo for this, if you want to take a look:
https://github.com/mmitteregger/druid-example/blob/master/src/widget/menu/menu_item.rs#L123-L125

Copy link
Member Author

Choose a reason for hiding this comment

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

In your example, I wouldn't use a Key at all for setting the color; I would just explicitly pass the color you want to the label.

I also wouldn't use WidgetPod for your label; if you are just using it for layout, you can draw the label where you want in Paint by using Label::draw_at (this is new).

Copy link
Member Author

@cmyr cmyr Sep 16, 2020

Choose a reason for hiding this comment

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

Having dug into this a bit more: you have three keys in the Env, MENU_ITEM_SELECTED_COLOR and three other variants. you then also have a key Color that you're setting on the label, and then you're dynamically updating this key based on the other keys.

when you set a Key as the value for something like text color, it means that when drawing we will figure out what that key's current color is, and then we'll use that color. So if you want to change the color, one option is to never change the key assigned to the label, and instead change the value for that key in the Env. An alternative is to change the key assigned to the label; for instance when your text is selected, you could do label.set_text_color(MENU_ITEM_SELECTED_COLOR). In your particular case, I think this is probably the easiest approach?

The rationale for this is that you could have different themes that provide different values for various keys, and it would 'just work'. Similarly, if you were using built-in keys we could support things like dark mode, or high-contrast colors for accessibility, and it would 'just work'.

Your crash is happening because the COLOR key you're using doesn't always exist in the Env; https://linebender.org/druid/env.html#keys-values-and-themes. I think this crash is okay; by the time update happens, any key you've assigned to a label must exist, and we crashing if it doesn't is reasonable behaviour.

Thanks for the detailed example, I'm glad I understand what was going on. :)

mmitteregger added a commit to mmitteregger/druid-example that referenced this pull request Sep 13, 2020
mmitteregger added a commit to mmitteregger/druid-example that referenced this pull request Sep 13, 2020
@rjwittams
Copy link
Collaborator

I think LensWrap needs to check if the environment has changed or it will skip environment only updates (meaning e.g label children of LensWrap would not get the update).

These methods can be used by widgets and other components to check
if particular items in the env have been modified, so they may
invalidate and rebuild resources as needed.

- closes #1167
This now more efficiently invalidates the TextLayout object by
checking for changes to specific keys.
@cmyr cmyr merged commit 00161ee into master Sep 16, 2020
@cmyr cmyr deleted the check-env-changes branch September 16, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-review waits for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants