Common context state by cmyr · Pull Request #970 · linebender/druid

cmyr · 2020-05-21T18:59:44Z

This is two related commits:

one adds the set_menu method to the lifecycle and update contexts
one refactors contexts so that all of the state that is A) shared and B) comes from the window and never changes is in one place. This will let us put common implementations there, and reduce duplicate code.

This is based on #969, which should go in first.

luleyleo

This sure cleans up quite a bit 👍 It would deserve a Maintenance changelog entry as well but as those purely internal only changes, there is no need for it.
Also I got a few more comments.

luleyleo · 2020-05-22T05:56:36Z

druid/src/core.rs

 }

+/// Static state that is shared between most contexts.
+pub(crate) struct RootState<'a> {


How about calling this ContextState akin to WidgetState? Because RootState really could mean anything.

luleyleo · 2020-05-22T05:59:54Z

druid/src/contexts.rs

    }
 }

+impl<'a> RootState<'a> {


It's a bit odd to have the impl in a different file than the struct itself. Maybe put both into contexts.rs?

this is fair, I think the impl really should be here so maybe I'll move RootState/ContextState back here as well.

luleyleo · 2020-05-22T06:02:10Z

druid/src/contexts.rs

+pub struct LayoutCtx<'a, 'b: 'a, 'c> {
+    pub(crate) root_state: &'a mut RootState<'c>,


This might be a bit annoying to change but it would be nice if all 'bs refer to the RootStates lifetime.

yea that would be nice but doesn't work unfortunately. :(

How so? The only 'issue' I see is PainCtx as it does not have RootState 🤔

oh sorry I thought you meant that we reuse 'b here for both inner lifetimes.

luleyleo · 2020-05-22T06:11:53Z

druid/src/contexts.rs

 }

-impl<'a, 'b> LayoutCtx<'a, 'b> {
+impl<'a, 'b, 'c> LayoutCtx<'a, 'b, 'c> {


Might be useful for the macro introduced by #972 :
This 'b and RootStates 'a are the only required lifetimes in this file (for two functions only), all others could be elided using '_.

interesting, I'll play around with that.

cool, I was able to simplify the lifetimes quite a bit, i'd forgotten they'd relaxed that rule :)

I also still keep adding them and then only realize afterwards that it was completely unnecessary 😅

I think it makes sense for show_context_menu to only be available in EventCtx, but for the window menu I can imagine needing it in lifecycle and update (and I need it in update in runebender)

cmyr · 2020-05-22T15:36:38Z

/bloat

github-actions · 2020-05-22T15:49:36Z

🗜 Bloat check ⚖️

Comparing 74fa6a9 against e024f60

target	old size	new size	difference
target/release/examples/calc	1.14 MB	1.14 MB	32 Bytes (0.00%)
target/release/examples/scroll_colors	1.12 MB	1.12 MB	48 Bytes (0.00%)
target/release/examples/multiwin	1.14 MB	1.14 MB	336 Bytes (0.03%)
target/release/examples/flex	1.5 MB	1.49 MB	-3.69 KB (-0.24%)
target/release/examples/styled_text	1.25 MB	1.26 MB	4.45 KB (0.35%)
target/release/examples/custom_widget	1.07 MB	1.07 MB	72 Bytes (0.01%)

xStrom

Generaly looks pretty nice!

CHANGELOG.md

druid/src/contexts.rs

xStrom

Just two more nits!

xStrom · 2020-05-22T17:02:28Z

druid/src/core.rs

        child_state: &mut WidgetState,
-        window_id: WindowId,
-        window: &WindowHandle,
+        root_state: &mut ContextState,


Missed this root_state.

xStrom · 2020-05-22T17:03:12Z

CHANGELOG.md

 - `im::Vector` support for the `List` widget. ([#940] by [@xStrom])
 - `LifeCycle::Size` event to inform widgets that their size changed. ([#953] by [@xStrom])
 - `Button::dynamic` constructor. ([#963] by [@totsteps])
+- Add `set_menu` method to `UpdateCtx` and `LifeCycleCtx` ([#970] by [@cmyr])


Given how all the other items under the Added section are written, probably better to remove the word Add from the start here.

luleyleo

Looks good!
Gotta be pedantic about that changelog entry tho 🙈

luleyleo · 2020-05-22T16:59:41Z

CHANGELOG.md

 - `im::Vector` support for the `List` widget. ([#940] by [@xStrom])
 - `LifeCycle::Size` event to inform widgets that their size changed. ([#953] by [@xStrom])
 - `Button::dynamic` constructor. ([#963] by [@totsteps])
+- Add `set_menu` method to `UpdateCtx` and `LifeCycleCtx` ([#970] by [@cmyr])


Let's keep the style consistent.

Suggested change

- Add `set_menu` method to `UpdateCtx` and `LifeCycleCtx` ([#970] by [@cmyr])

- `UpdateCtx::set_menu` and `LifeCycleCtx::set_menu` ([#970] by [@cmyr])

This adds a new type, RootState that holds onto all of the app/window-scoped state that is shared between all non-paint contexts. Simplify context lifetimes fixup

cmyr force-pushed the base-state-to-widget-state branch from 7cf7b60 to 66db763 Compare May 21, 2020 19:01

cmyr force-pushed the common-context-state branch 2 times, most recently from 701d4e8 to f1d1650 Compare May 21, 2020 19:20

cmyr mentioned this pull request May 22, 2020

Use a macro to implement context methods #972

Merged

luleyleo reviewed May 22, 2020

View reviewed changes

luleyleo added S-waiting-on-author waits for changes from the submitter S-blocked waits for another PR or dependency labels May 22, 2020

cmyr force-pushed the base-state-to-widget-state branch from 66db763 to ddeeca5 Compare May 22, 2020 14:07

cmyr force-pushed the common-context-state branch from f1d1650 to 6713aa7 Compare May 22, 2020 14:37

cmyr force-pushed the base-state-to-widget-state branch from ddeeca5 to 5d1aa86 Compare May 22, 2020 14:47

Base automatically changed from base-state-to-widget-state to master May 22, 2020 15:34

Add set_menu method to lifecycle & update contexts

1327af7

I think it makes sense for show_context_menu to only be available in EventCtx, but for the window menu I can imagine needing it in lifecycle and update (and I need it in update in runebender)

cmyr removed the S-blocked waits for another PR or dependency label May 22, 2020

cmyr force-pushed the common-context-state branch from 6713aa7 to 74fa6a9 Compare May 22, 2020 15:35

cmyr added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 22, 2020

xStrom reviewed May 22, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

druid/src/contexts.rs Outdated Show resolved Hide resolved

cmyr force-pushed the common-context-state branch from 74fa6a9 to 8fc5863 Compare May 22, 2020 16:57

xStrom approved these changes May 22, 2020

View reviewed changes

luleyleo approved these changes May 22, 2020

View reviewed changes

Refactor contexts for more code reuse

ce8a1b2

This adds a new type, RootState that holds onto all of the app/window-scoped state that is shared between all non-paint contexts. Simplify context lifetimes fixup

cmyr force-pushed the common-context-state branch from 8fc5863 to ce8a1b2 Compare May 22, 2020 17:06

cmyr merged commit 7c2fa9c into master May 22, 2020

cmyr deleted the common-context-state branch May 22, 2020 17:37

		pub struct LayoutCtx<'a, 'b: 'a, 'c> {
		pub(crate) root_state: &'a mut RootState<'c>,

	- Add `set_menu` method to `UpdateCtx` and `LifeCycleCtx` ([#970] by [@cmyr])
	- `UpdateCtx::set_menu` and `LifeCycleCtx::set_menu` ([#970] by [@cmyr])

Conversation

cmyr commented May 21, 2020

Uh oh!

luleyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr commented May 22, 2020

Uh oh!

github-actions bot commented May 22, 2020

🗜 Bloat check ⚖️

Uh oh!

xStrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xStrom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luleyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants