app_bar [nfc]: Centralize _getEffectiveCenterTitle in our wrapper by chrisbobbe · Pull Request #1141 · zulip/zulip-flutter

chrisbobbe · 2024-12-12T00:10:19Z

And make TestZulipApp more helpful, too, since I noticed an opportunity for that.

PIG208 · 2024-12-12T00:48:31Z

    super.key,
    super.titleSpacing,
-    required super.title,
+    required Widget? Function(bool Function(ThemeData) willCenterTitle) buildTitle,


It took me a while to parse how this works. Maybe we can try to implement this as a widget? A concept would be like:

class _TitleBuilder extends StatelessWidget { const _TitleBuilder({ required this.centerTitle, required this.actionsCount, required this.buildTitle, }); final bool? centerTitle; final int actionsCount; final Widget Function(bool centerTitle) buildTitle; // Copied from [AppBar._getEffectiveCenterTitle]. bool willCenterTitle(BuildContext context) { final theme = Theme.of(context); bool platformCenter() { switch (theme.platform) { case TargetPlatform.android: case TargetPlatform.fuchsia: case TargetPlatform.linux: case TargetPlatform.windows: return false; case TargetPlatform.iOS: case TargetPlatform.macOS: return actionsCount == 0 || actionsCount < 2; } } return centerTitle ?? theme.appBarTheme.centerTitle ?? platformCenter(); } @override Widget build(BuildContext context) { return buildTitle(willCenterTitle(context)); } }

Letting buildTitle take a bool simplifies the API a bit and with a widget we can replace the callback constructed below with a method.

Oh nice, this is great! Thanks for the idea.

PIG208 · 2024-12-12T00:57:31Z

Thanks for working on this! Left a comment on the proposed buildTitle API. Otherwise this looks good to me.

chrisbobbe · 2024-12-12T23:24:53Z

Thanks for the review! Revision pushed. I arranged it so it includes an exact copy of AppBar._getEffectiveCenterTitle, to make it as easy as possible to verify that it behaves the same.

chrisbobbe · 2024-12-13T08:17:50Z

Note to self; I think there's a comment in the message-list code I've forgotten to remove in this revision:

      // The helper [_getEffectiveCenterTitle] relies on the fact that we
      // have at most one action here.

gnprice

Thanks to you both!

Some small comments below, and then a further adjustment to the API.

gnprice · 2024-12-17T21:53:24Z

  const TestZulipApp({
    super.key,
    this.accountId,
+    this.skipAssertAccountExists,


Suggested change

this.skipAssertAccountExists,

this.skipAssertAccountExists = false,

(and make the field non-nullable)

This is meant to treat null and false the same, right? So may as well make that more explicit by defaulting straight to false.

gnprice · 2024-12-17T21:56:07Z

+      if (accountId != null && skipAssertAccountExists != true) {
+        final account = GlobalStoreWidget.of(context).getAccount(accountId!);
+        assert(account != null,
+          'TestZulipApp() was called with [accountId] but a corresponding '


nit: put these conditionals inside the assert (with an IIFE)

gnprice · 2024-12-17T21:57:35Z

+          'TestZulipApp() was called with [accountId] but a corresponding '
+          'Account was not found in the GlobalStore. '
+          'If [child] needs per-account data, consider calling '
+          '`testBinding.globalStore.add` before pumping `TestZulipApp`. '
+          'If [child] is not specific to an account, omit [accountId]. '
+          'If you are testing behavior when an account is logged out, '
+          'consider building ZulipApp instead of TestZulipApp '
+          'or passing skipAssertAccountExists.');


nit: for a long dev-facing error message like this, use FlutterError.fromParts — grep our code for a few examples

gnprice · 2024-12-17T21:59:41Z

+  /// based on [centerTitle], the theme, the platform, and [actions].
+  /// Useful if [title] is a container whose children should align the same way,
+  /// such as a [Column] with multiple lines of text.
+  // TODO send a PR upstream to replace our `willCenterTitle` code


nit:

Suggested change

// TODO send a PR upstream to replace our `willCenterTitle` code

// TODO(upstream) send a PR to replace our `willCenterTitle` code

I think the TODO(upstream) form is helpful for grepping.

gnprice · 2024-12-17T22:02:18Z


    return Scaffold(
-      appBar: ZulipAppBar(title: Text(user.fullName)),
+      appBar: ZulipAppBar(buildTitle: (_) => Text(user.fullName)),


These diff hunks are kind of unfortunate, and I think an upstream PR for this would need to avoid them. Instead the new more-complex way of building a title should be opt-in.

gnprice · 2024-12-17T22:06:22Z

    super.key,
    super.titleSpacing,
-    required super.title,
+    required Widget Function(bool willCenterTitle) buildTitle,


Suggested change

required Widget Function(bool willCenterTitle) buildTitle,

Widget? title,

Widget Function(bool willCenterTitle)? buildTitle,

Then assert exactly one of those is non-null; and use that one. Much like hint and hintText etc. on upstream's InputDecoration.

chrisbobbe · 2025-01-18T00:02:46Z

Thanks for the review! Revision pushed, atop #1290 for the sake of CI.

gnprice

Thanks! This looks good now; and I've also now read the tests, and I appreciate the thorough set of test cases. One small comment in the tests.

gnprice · 2025-01-23T06:31:51Z

+      final titlePosition = tester.getCenter(find.text('a')).dx;
+      final isCentered = titlePosition > ((1 / 3) * titleAreaRightEdgeOffset);
+      check(titlePosition).isLessThan((2 / 3) * titleAreaRightEdgeOffset);


How about:

Suggested change

final titlePosition = tester.getCenter(find.text('a')).dx;

final isCentered = titlePosition > ((1 / 3) * titleAreaRightEdgeOffset);

check(titlePosition).isLessThan((2 / 3) * titleAreaRightEdgeOffset);

final titlePosition = tester.getTopLeft(find.text('a')).dx;

final isCentered = titlePosition > ((1 / 3) * titleAreaRightEdgeOffset);

check(titlePosition).isLessThan((2 / 3) * titleAreaRightEdgeOffset);

Otherwise this test implicitly relies on the title having its size shrink to its contents, rather than filling its parent's size.

This is nice and explicit compared to the behavior I got when I simply forgot to call `testBinding.globalStore.add` when working on some upcoming tests.

This logic is sort of complicated, and duplicated from upstream. Better to put it centrally in our ZulipAppBar wrapper, instead of in message_list.dart. As a bonus, we also have it handle `actions` instead of assuming there are none.

chrisbobbe · 2025-01-23T21:12:31Z

Thanks for the review! Revision pushed.

gnprice · 2025-01-23T21:21:56Z

Thanks! Looks good; merging.

chrisbobbe requested a review from PIG208 December 12, 2024 00:10

chrisbobbe assigned PIG208 Dec 12, 2024

PIG208 reviewed Dec 12, 2024

View reviewed changes

chrisbobbe force-pushed the pr-centralize-will-center-app-bar-title branch from 8633634 to c097bec Compare December 12, 2024 23:22

PIG208 assigned gnprice and unassigned PIG208 Dec 13, 2024

PIG208 requested a review from gnprice December 13, 2024 00:33

PIG208 added the integration review Added by maintainers when PR may be ready for integration label Dec 13, 2024

gnprice reviewed Dec 17, 2024

View reviewed changes

chrisbobbe force-pushed the pr-centralize-will-center-app-bar-title branch from c097bec to 6e734b8 Compare January 18, 2025 00:01

gnprice reviewed Jan 23, 2025

View reviewed changes

chrisbobbe added 2 commits January 23, 2025 13:08

test_app: Assert the store has an Account for the passed accountId

dbe1cad

This is nice and explicit compared to the behavior I got when I simply forgot to call `testBinding.globalStore.add` when working on some upcoming tests.

chrisbobbe force-pushed the pr-centralize-will-center-app-bar-title branch from 6e734b8 to 79ada37 Compare January 23, 2025 21:12

gnprice merged commit 79ada37 into zulip:main Jan 23, 2025

	this.skipAssertAccountExists,
	this.skipAssertAccountExists = false,

	// TODO send a PR upstream to replace our `willCenterTitle` code
	// TODO(upstream) send a PR to replace our `willCenterTitle` code

	required Widget Function(bool willCenterTitle) buildTitle,
	Widget? title,
	Widget Function(bool willCenterTitle)? buildTitle,

Conversation

chrisbobbe commented Dec 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PIG208 commented Dec 12, 2024

Uh oh!

chrisbobbe commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Dec 13, 2024

Uh oh!

gnprice 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!

chrisbobbe commented Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jan 23, 2025

Uh oh!

gnprice commented Jan 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Dec 12, 2024 •

edited

Loading

chrisbobbe commented Jan 18, 2025 •

edited

Loading