store [nfc]: Add and use PerAccountStoreAwareStateMixin #241

chrisbobbe · 2023-07-25T01:02:38Z

I'm hoping this helper might be useful for reducing duplicated code, but I'm not certain it's the best pattern and I'd be interested in feedback. 🙂 If it seems good, I plan to use it in the recent DM conversations widget, for #119, soon.

gnprice

Hmm, interesting idea! And good catch re the need (in the future, with #185) for this functionality on the ComposeAutocomplete state.

I think this pattern makes sense. It reminds me of a genre of mixin that's used widely within the Flutter framework, like RenderObjectWithChildMixin.

Various smaller comments below.

gnprice · 2023-07-25T03:01:00Z

lib/model/autocomplete.dart

-  set query(MentionAutocompleteQuery query) {
+  set query(MentionAutocompleteQuery? query) {


When adding this, I got a diagnostic error `getter_not_subtype_setter_types`: https://dart.dev/tools/diagnostic-messages?utm_source=dartdev&utm_medium=redir&

utm_id=diagcode&utm_content=getter_not_subtype_setter_types#getter_not_subtype_setter
_types

Shrug; I guess it's fine to adjust the setter param's type to the higher type, with code to handle the difference.

Cool. My first reaction was actually that it's perfectly reasonable for the setter to take a narrower type than the getter returns, for a use case like this one where the value starts as null or some other placeholder and you don't want to allow setting again to that… but on reading a bit of upstream discussion, following links from here:
https://groups.google.com/a/dartlang.org/g/misc/c/74VgbBxVXoM
the idea is that in the future the getter and setter might be more closely paired, as two aspects of a single member with a single type. Which seems like a fine future to aim for.

Should trim down that URL in the commit message, though. Without the tracking it gets a lot more readable 🙂 — and also avoids getting wrapped across lines in a terminal.

(The messed-up formatting above is how it copy-pasted for me, which typically I'd fix but here it serves as a nice illustration.)

gnprice · 2023-07-25T03:05:01Z

lib/model/autocomplete.dart

+  MentionAutocompleteQuery? get query => _currentQuery;
  MentionAutocompleteQuery? _currentQuery;


Also let's rename the backing field to match the getter and setter: so just _query. (Probably should have written it that way in the first place, to match the setter.)

gnprice · 2023-07-25T03:06:26Z

lib/widgets/store.dart

+    if (_store != storeNow) {
+      onNewStore();
+    }
+    _store = storeNow;


Suggested change

if (_store != storeNow) {

onNewStore();

}

_store = storeNow;

if (_store != storeNow) {

_store = storeNow;

onNewStore();

}

Since we're already going to the trouble of checking whether it changed, might as well set it only when it did.

gnprice · 2023-07-25T03:09:38Z

lib/widgets/store.dart

+/// - add listeners on the new one.
+/// Use this mixin, overriding [onNewStore], to do that concisely.


nit:

Suggested change

/// - add listeners on the new one.

/// Use this mixin, overriding [onNewStore], to do that concisely.

/// - add listeners on the new one.

///

/// Use this mixin, overriding [onNewStore], to do that concisely.

(otherwise the latter line gets interpreted as part of the list item — to see the effect, hover in the IDE over a use of this class)

gnprice · 2023-07-25T03:21:01Z

lib/widgets/store.dart

+  /// Called in the first [State.didChangeDependencies] call, and afterward
+  /// when [PerAccountStoreWidget.of] returns a different store than last time.
+  ///
+  /// In this, remove any listeners on the old store
+  /// and add them on the new store.
+  void onNewStore();


Here's a version which in particular tries to add a succinct summary:

Suggested change

/// Called in the first [State.didChangeDependencies] call, and afterward

/// when [PerAccountStoreWidget.of] returns a different store than last time.

///

/// In this, remove any listeners on the old store

/// and add them on the new store.

void onNewStore();

/// Called when there is a new ambient [PerAccountStore].

///

/// Specifically this is called when this element is first inserted into the tree

/// (so that it has an ambient [PerAccountStore] for the first time),

/// and again whenever dependencies change so that [PerAccountStoreWidget.of]

/// would return a different store from previously.

///

/// In this, remove any listeners on the old store

/// and add them on the new store.

void onNewStore();

gnprice · 2023-07-25T03:40:29Z

lib/widgets/store.dart

+    final storeNow = PerAccountStoreWidget.of(context);
+    if (_store != storeNow) {
+      onNewStore();
+    }
+    _store = storeNow;


It'd be good to have a test for this logic: make a test widget that uses this mixin, put it through its paces, check that onNewStore gets called at the appropriate times.

I think you can arrange such a test by moving the widget from under one PerAccountStoreWidget to under another with a different accountId. (Using key to ensure that the PerAccountStoreWidget's element gets replaced with a new one, rather than updated, so that its initState runs again; and also to ensure that the child element gets moved instead of replaced with a new one.) That isn't exactly something I anticipate we'll do in real life, but I think it's close enough to make an effective test.

That isn't exactly something I anticipate we'll do in real life

I might even go further: couldn't we have the app code assert that the widget doesn't get moved that way in real life? 😅

If we did ask it to do that, maybe accidentally, then I think I'd wonder whether a piece of UI is showing data for the wrong account, or at least not being clear about which account's data it's showing. And those feel like outcomes that are important to avoid.

Adding that assert would kind of put this testing tactic out of reach, though, wouldn't it.

When adding this, I got a diagnostic error `getter_not_subtype_setter_types`: https://dart.dev/tools/diagnostic-messages#getter_not_subtype_setter_types Shrug; I guess it's fine to adjust the setter param's type to the higher type, with code to handle the difference. Discussion: zulip#241 (comment)

chrisbobbe · 2023-07-31T21:37:58Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Small comments on the new tests; otherwise all looks good.

gnprice · 2023-07-31T23:34:40Z

test/widgets/store_test.dart

+    // Nudge PerAccountStoreWidget to send its updated store to MyWidgetWithMixin.
+    //
+    // A change in PerAccountStoreWidget's [accountId] field doesn't by itself
+    // prompt dependant widgets (those using PerAccountStoreWidget.of) to update,


nit:

Suggested change

// prompt dependant widgets (those using PerAccountStoreWidget.of) to update,

// prompt dependent widgets (those using PerAccountStoreWidget.of) to update,

gnprice · 2023-07-31T23:35:04Z

test/widgets/store_test.dart

+              child: MyWidgetWithMixin(key: widgetWithMixinKey))),
+        ));


nit:

Suggested change

child: MyWidgetWithMixin(key: widgetWithMixinKey))),

));

child: MyWidgetWithMixin(key: widgetWithMixinKey)))));

gnprice · 2023-07-31T23:40:25Z

test/widgets/store_test.dart

+    // But it will prompt widgets to update when it receives a notification
+    // from the GlobalStore; see its state's didChangeDependencies.
+    // So, take advantage of that.


Hmm, interesting. That behavior seems undesirable.

… Oh, but I think it only causes such an update if it gets a different PerAccountStore from what it had before. Right? Because the update happens as a consequence of the PerAccountStoreWidget element rebuilding, which is caused by this setState:

void _setStore(PerAccountStore store) { if (store != this.store) { setState(() {

So that's fine then, but the comment should just be clarified.

See what you think of the comment in my next revision; I'm not sure I've found the right balance of explaining and being brief. 🙂

Looks fine. :-) Might be possible to make it briefer; but the other thing to balance is effort spent, so for this context this new version is good. (If it were some central API's doc rather than an implementation comment in a test, I'd probably give it another pass.)

gnprice · 2023-08-01T00:01:25Z

test/widgets/store_test.dart

+    await pumpWithParams(light: false, accountId: accountId);
+    await tester.pumpAndSettle();
+    check(widgetWithMixinKey).currentState.isNotNull()
+      ..anyDepChangeCounter.equals(3) // (Dunno why 3 instead of 2. Anyway, more than 1…)


I was curious too, so I tried setting debugPrintRebuildDirtyWidgets = true; before this pumpAndSettle call.

(Actually I started by figuring there'd be some debug variable I could turn on to get some relevant logging for this; so I typed "debug" and then browsed through the IDE's suggestions. The other one I tried was debugPrintScheduleBuildForStacks, and I think either one would have found the answer.)

(More accurately, I started by inserting print(StackTrace.current); into MyWidgetWithMixinState.didChangeDependencies. But the call stack here is generic framework machinery that doesn't really illuminate this question.)

The thing that kicks the dependency change off is:

Building AnimatedTheme(duration: 200ms, dirty, state: _AnimatedThemeState#0c4a7(ticker active, ThemeDataTween(ThemeData#187b1 → ThemeData#2deee)))

Note the "animated" in the name. Apparently by default a theme change happens over 200ms.

And then pumpAndSettle has a default per-frame duration of 100ms. So we get one frame where the theme is halfway between light and dark, and then one where it's dark.

Knowing that: to make this a bit cleaner, you can replace the pumpAndSettle with a single pump(const Duration(seconds: 1)) (or any other duration of at least 200ms).

Building AnimatedTheme(duration: 200ms, dirty, state: _AnimatedThemeState#0c4a7(ticker active, ThemeDataTween(ThemeData#187b1 → ThemeData#2deee)))

Interesting, thanks for the tip!

I'm curious; to me, that line is kind of a needle in a haystack, when I look at it with all the additional output I get:

debugoutput.txt

Did it just stand out to you because of the "200ms"?

Good question. The output I was looking at looked different, and I guess there are two things there:

One is that I only set those variables just before the pumpAndSettle in question, so all the setup up to that point wouldn't cause logging:

// [onNewStore] not called on unrelated dependency change await pumpWithParams(light: false, accountId: accountId); + debugPrintRebuildDirtyWidgets = true; await tester.pumpAndSettle();

Then that line is the very first one, so it naturally stands out for that reason.

The other is that perhaps those other two kinds of logging I mentioned above were helpful after all, just for punctuating what's happening. In particular this:

@@ -28,6 +28,7 @@ class MyWidgetWithMixinState extends State<MyWidgetWithMixin> with PerAccountSto @override void didChangeDependencies() { + print(StackTrace.current); super.didChangeDependencies(); anyDepChangeCounter++; }

So then those stack traces stand out, and they mark where the test widget is getting rebuilt. At the end of some of them, it looks like

[… bunch more stack frames …] #20 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1008:5) <asynchronous suspension> #21 StackZoneSpecification._registerCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:114:42) <asynchronous suspension> Building Text("brightness: Brightness.dark; accountId: 1001", dependencies: [DefaultSelectionStyle, DefaultTextStyle, MediaQuery]) Rebuilding AnimatedTheme(duration: 200ms, dirty, state: _AnimatedThemeState#1d770(ticker active, ThemeDataTween(ThemeData#b08bd → ThemeData#fb850))) Rebuilding Theme(ThemeData#fb850, dependencies: [DefaultSelectionStyle]) [… bunch more rebuilds ….]

So the test widget is getting rebuilt, and then the Text that's its child… and then the next thing is this AnimatedTheme, so that must be what prompted a fresh round of rebuilds.

Here's the resulting full output (from flutter test test/widgets/store_test.dart >& output.log):
output.log

gnprice · 2023-08-01T00:03:26Z

test/widgets/store_test.dart

+    await pumpWithParams(light: true, accountId: accountId);
+    await tester.pump(); // global store
+    await tester.pump(); // per-account store
+    check(widgetWithMixinKey).currentState.isNotNull().storeChangeCounter.equals(1);


nit: make more parallel with the other similar statements:

Suggested change

check(widgetWithMixinKey).currentState.isNotNull().storeChangeCounter.equals(1);

check(widgetWithMixinKey).currentState.isNotNull()

.storeChangeCounter.equals(1);

… Hmm but seeing it that way, it's redundant with the next check, right? So I think it can just be dropped.

When adding this, I got a diagnostic error `getter_not_subtype_setter_types`: https://dart.dev/tools/diagnostic-messages#getter_not_subtype_setter_types Shrug; I guess it's fine to adjust the setter param's type to the higher type, with code to handle the difference. Discussion: zulip#241 (comment)

chrisbobbe · 2023-08-01T00:37:45Z

Thanks for the review! Revision pushed.

When adding this, I got a diagnostic error `getter_not_subtype_setter_types`: https://dart.dev/tools/diagnostic-messages#getter_not_subtype_setter_types Shrug; I guess it's fine to adjust the setter param's type to the higher type, with code to handle the difference. Discussion: zulip#241 (comment)

gnprice · 2023-08-01T02:32:33Z

test/flutter_checks.dart

 }
+extension GlobalKeyChecks<T extends State<StatefulWidget>> on Subject<GlobalKey<T>> {


Suggested change

}

extension GlobalKeyChecks<T extends State<StatefulWidget>> on Subject<GlobalKey<T>> {

}

extension GlobalKeyChecks<T extends State<StatefulWidget>> on Subject<GlobalKey<T>> {

gnprice · 2023-08-01T02:46:52Z

test/widgets/store_test.dart

+    await pumpWithParams(light: false, accountId: accountId);
+    await tester.pumpAndSettle();
+    check(widgetWithMixinKey).currentState.isNotNull()
+      ..anyDepChangeCounter.equals(3) // (Dunno why 3 instead of 2. Anyway, more than 1…)


Good question. The output I was looking at looked different, and I guess there are two things there:

One is that I only set those variables just before the pumpAndSettle in question, so all the setup up to that point wouldn't cause logging:

// [onNewStore] not called on unrelated dependency change await pumpWithParams(light: false, accountId: accountId); + debugPrintRebuildDirtyWidgets = true; await tester.pumpAndSettle();

Then that line is the very first one, so it naturally stands out for that reason.

The other is that perhaps those other two kinds of logging I mentioned above were helpful after all, just for punctuating what's happening. In particular this:

@@ -28,6 +28,7 @@ class MyWidgetWithMixinState extends State<MyWidgetWithMixin> with PerAccountSto @override void didChangeDependencies() { + print(StackTrace.current); super.didChangeDependencies(); anyDepChangeCounter++; }

So then those stack traces stand out, and they mark where the test widget is getting rebuilt. At the end of some of them, it looks like

[… bunch more stack frames …] #20 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1008:5) <asynchronous suspension> #21 StackZoneSpecification._registerCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:114:42) <asynchronous suspension> Building Text("brightness: Brightness.dark; accountId: 1001", dependencies: [DefaultSelectionStyle, DefaultTextStyle, MediaQuery]) Rebuilding AnimatedTheme(duration: 200ms, dirty, state: _AnimatedThemeState#1d770(ticker active, ThemeDataTween(ThemeData#b08bd → ThemeData#fb850))) Rebuilding Theme(ThemeData#fb850, dependencies: [DefaultSelectionStyle]) [… bunch more rebuilds ….]

So the test widget is getting rebuilt, and then the Text that's its child… and then the next thing is this AnimatedTheme, so that must be what prompted a fresh round of rebuilds.

Here's the resulting full output (from flutter test test/widgets/store_test.dart >& output.log):
output.log

gnprice

Thanks for the revision! All looks good modulo one nit above. (See also reply further above at #241 (comment) .) Please go ahead and merge when ready.

When adding this, I got a diagnostic error `getter_not_subtype_setter_types`: https://dart.dev/tools/diagnostic-messages#getter_not_subtype_setter_types Shrug; I guess it's fine to adjust the setter param's type to the higher type, with code to handle the difference. Discussion: zulip#241 (comment)

As with other uses of PerAccountStoreAwareStateMixin, we expect the benefit to be noticeable after our planned implementation of zulip#185, "Start new event queue when old one has expired". At that time, this should mean that an expiring event queue won't break autocomplete: if the lifetime of your autocomplete intent spans across a queue-renewal, you can continue to edit your query after the renewal, and the app will ask the new store for the query results, instead of asking the old one.

chrisbobbe · 2023-08-01T18:04:49Z

Thanks for the review and for that further reply! Done.

chrisbobbe added the a-model Implementing our data model (PerAccountStore, etc.) label Jul 25, 2023

chrisbobbe requested a review from gnprice July 25, 2023 01:02

gnprice reviewed Jul 25, 2023

View reviewed changes

chrisbobbe force-pushed the pr-per-account-store-aware-state-mixin branch from af4a1f4 to d967b94 Compare July 31, 2023 21:32

gnprice reviewed Aug 1, 2023

View reviewed changes

chrisbobbe force-pushed the pr-per-account-store-aware-state-mixin branch from d967b94 to e887fd8 Compare August 1, 2023 00:37

chrisbobbe force-pushed the pr-per-account-store-aware-state-mixin branch from e887fd8 to f3e9828 Compare August 1, 2023 00:43

gnprice reviewed Aug 1, 2023

View reviewed changes

gnprice approved these changes Aug 1, 2023

View reviewed changes

chrisbobbe added 4 commits August 1, 2023 11:02

store [nfc]: Add and use PerAccountStoreAwareStateMixin

d4772e2

autocomplete [nfc]: s/_currentQuery/_query/ in MentionAutocompleteView

2eb6d28

chrisbobbe force-pushed the pr-per-account-store-aware-state-mixin branch from f3e9828 to f29f3cc Compare August 1, 2023 18:04

chrisbobbe merged commit f29f3cc into zulip:main Aug 1, 2023

chrisbobbe deleted the pr-per-account-store-aware-state-mixin branch August 1, 2023 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store [nfc]: Add and use PerAccountStoreAwareStateMixin #241

store [nfc]: Add and use PerAccountStoreAwareStateMixin #241

chrisbobbe commented Jul 25, 2023

gnprice left a comment

gnprice Jul 25, 2023

gnprice Jul 25, 2023

gnprice Jul 25, 2023

gnprice Jul 25, 2023

gnprice Jul 25, 2023

gnprice Jul 25, 2023

chrisbobbe Jul 25, 2023

gnprice Jul 25, 2023

chrisbobbe Jul 25, 2023 •

edited

Loading

chrisbobbe commented Jul 31, 2023

gnprice left a comment

gnprice Jul 31, 2023

gnprice Jul 31, 2023

gnprice Jul 31, 2023

chrisbobbe Aug 1, 2023

gnprice Aug 1, 2023

gnprice Aug 1, 2023

chrisbobbe Aug 1, 2023

gnprice Aug 1, 2023

gnprice Aug 1, 2023

chrisbobbe commented Aug 1, 2023

gnprice Aug 1, 2023

gnprice Aug 1, 2023

gnprice left a comment

chrisbobbe commented Aug 1, 2023

		set query(MentionAutocompleteQuery query) {
		set query(MentionAutocompleteQuery? query) {

		MentionAutocompleteQuery? get query => _currentQuery;
		MentionAutocompleteQuery? _currentQuery;

		/// - add listeners on the new one.
		/// Use this mixin, overriding [onNewStore], to do that concisely.

	// prompt dependant widgets (those using PerAccountStoreWidget.of) to update,
	// prompt dependent widgets (those using PerAccountStoreWidget.of) to update,

	child: MyWidgetWithMixin(key: widgetWithMixinKey))),
	));
	child: MyWidgetWithMixin(key: widgetWithMixinKey)))));

	check(widgetWithMixinKey).currentState.isNotNull().storeChangeCounter.equals(1);
	check(widgetWithMixinKey).currentState.isNotNull()
	.storeChangeCounter.equals(1);

		}
		extension GlobalKeyChecks<T extends State<StatefulWidget>> on Subject<GlobalKey<T>> {

store [nfc]: Add and use PerAccountStoreAwareStateMixin #241

store [nfc]: Add and use PerAccountStoreAwareStateMixin #241

Conversation

chrisbobbe commented Jul 25, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Jul 31, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 1, 2023

chrisbobbe Jul 25, 2023 •

edited

Loading