Skip to content

Commit

Permalink
app_bar [nfc]: Centralize _getEffectiveCenterTitle in our wrapper
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
chrisbobbe committed Jan 23, 2025
1 parent dbe1cad commit 79ada37
Show file tree
Hide file tree
Showing 3 changed files with 186 additions and 33 deletions.
56 changes: 54 additions & 2 deletions lib/widgets/app_bar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,66 @@ import 'store.dart';
///
/// This should be used for most of the pages with access to [PerAccountStore].
class ZulipAppBar extends AppBar {
/// Creates our Zulip custom app bar based on [AppBar].
///
/// [buildTitle] is passed a boolean `willCenterTitle` that answers
/// whether the underlying [AppBar] will decide to center [title]
/// 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(upstream) send a PR to replace our `willCenterTitle` code
ZulipAppBar({
super.key,
super.titleSpacing,
required super.title,
Widget? title,
Widget Function(bool willCenterTitle)? buildTitle,
super.centerTitle,
super.backgroundColor,
super.shape,
super.actions,
}) : super(bottom: _ZulipAppBarBottom(backgroundColor: backgroundColor));
}) :
assert((title == null) != (buildTitle == null)),
super(
bottom: _ZulipAppBarBottom(backgroundColor: backgroundColor),
title: title ?? _Title(centerTitle: centerTitle, actions: actions, buildTitle: buildTitle!)
);
}

class _Title extends StatelessWidget {
const _Title({
required this.centerTitle,
required this.actions,
required this.buildTitle,
});

final bool? centerTitle;
final List<Widget>? actions;
final Widget Function(bool centerTitle) buildTitle;

// A copy of [AppBar._getEffectiveCenterTitle].
bool _getEffectiveCenterTitle(ThemeData theme) {
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 actions == null || actions!.length < 2;
}
}

return centerTitle ?? theme.appBarTheme.centerTitle ?? platformCenter();
}

@override
Widget build(BuildContext context) {
final theme = Theme.of(context);
final willCenterTitle = _getEffectiveCenterTitle(theme);
return buildTitle(willCenterTitle);
}
}

class _ZulipAppBarBottom extends StatelessWidget implements PreferredSizeWidget {
Expand Down
41 changes: 10 additions & 31 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,6 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis

List<Widget>? actions;
if (narrow case TopicNarrow(:final streamId)) {
// The helper [_getEffectiveCenterTitle] relies on the fact that we
// have at most one action here.
(actions ??= []).add(IconButton(
icon: const Icon(ZulipIcons.message_feed),
tooltip: zulipLocalizations.channelFeedButtonTooltip,
Expand All @@ -281,7 +279,8 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis

return Scaffold(
appBar: ZulipAppBar(
title: MessageListAppBarTitle(narrow: narrow),
buildTitle: (willCenterTitle) =>
MessageListAppBarTitle(narrow: narrow, willCenterTitle: willCenterTitle),
actions: actions,
backgroundColor: appBarBackgroundColor,
shape: removeAppBarBottomBorder
Expand Down Expand Up @@ -321,9 +320,14 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
}

class MessageListAppBarTitle extends StatelessWidget {
const MessageListAppBarTitle({super.key, required this.narrow});
const MessageListAppBarTitle({
super.key,
required this.narrow,
required this.willCenterTitle,
});

final Narrow narrow;
final bool willCenterTitle;

Widget _buildStreamRow(BuildContext context, {
ZulipStream? stream,
Expand Down Expand Up @@ -367,29 +371,6 @@ class MessageListAppBarTitle extends StatelessWidget {
]);
}

// TODO(upstream): provide an API for this
// Adapted from [AppBar._getEffectiveCenterTitle].
bool _getEffectiveCenterTitle(ThemeData theme) {
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:
// We rely on the fact that there is at most one action
// on the message list app bar, so that the expression returned
// in the original helper, `actions == null || actions!.length < 2`,
// always evaluates to `true`:
return true;
}
}

return theme.appBarTheme.centerTitle ?? platformCenter();
}

@override
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
Expand All @@ -410,19 +391,17 @@ class MessageListAppBarTitle extends StatelessWidget {
return _buildStreamRow(context, stream: stream);

case TopicNarrow(:var streamId, :var topic):
final theme = Theme.of(context);
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
final centerTitle = _getEffectiveCenterTitle(theme);
return SizedBox(
width: double.infinity,
child: GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () => showTopicActionSheet(context,
channelId: streamId, topic: topic),
child: Column(
crossAxisAlignment: centerTitle ? CrossAxisAlignment.center
: CrossAxisAlignment.start,
crossAxisAlignment: willCenterTitle ? CrossAxisAlignment.center
: CrossAxisAlignment.start,
children: [
_buildStreamRow(context, stream: stream),
_buildTopicRow(context, stream: stream, topic: topic),
Expand Down
122 changes: 122 additions & 0 deletions test/widgets/app_bar_test.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'package:checks/checks.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter_checks/flutter_checks.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/widgets/app_bar.dart';
import 'package:zulip/widgets/profile.dart';
Expand Down Expand Up @@ -35,4 +37,124 @@ void main() {
check(tester.getRect(find.byType(ZulipAppBar))).equals(rectBefore);
check(finder.evaluate()).single;
});

group("buildTitle's willCenterTitle agrees with Material AppBar", () {
/// Build an [AppBar]; inspect and return whether it decided to center.
Future<bool> material(WidgetTester tester, {
required bool? paramValue,
required bool? themeValue,
required List<Widget>? actions,
}) async {
testBinding.reset();

final themeData = ThemeData(appBarTheme: AppBarTheme(centerTitle: themeValue));
final widget = TestZulipApp(
child: Theme(data: themeData,
child: AppBar(
centerTitle: paramValue,
actions: actions,
title: const Text('a'))));

await tester.pumpWidget(widget);
await tester.pump();

// test assumes LTR text direction
check(tester.platformDispatcher.locale).equals(const Locale('en', 'US'));
assert(actions == null || actions.isNotEmpty);
final titleAreaRightEdgeOffset = actions == null
? (tester.view.physicalSize / tester.view.devicePixelRatio).width
: tester.getTopLeft(find.byWidget(actions.first)).dx;
final titlePosition = tester.getTopLeft(find.text('a')).dx;
final isCentered = titlePosition > ((1 / 3) * titleAreaRightEdgeOffset);
check(titlePosition).isLessThan((2 / 3) * titleAreaRightEdgeOffset);

return isCentered;
}

/// Build a [ZulipAppBar]; return willCenterTitle from the buildTitle call.
Future<bool> ours(WidgetTester tester, {
required bool? paramValue,
required bool? themeValue,
required List<Widget>? actions,
}) async {
testBinding.reset();
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());

bool? result;

final widget = TestZulipApp(
// ZulipAppBar expects a per-account context (for the loading indicator)
accountId: eg.selfAccount.id,
child: Builder(builder: (context) => Theme(
data: Theme.of(context).copyWith(appBarTheme: AppBarTheme(centerTitle: themeValue)),
child: ZulipAppBar(
centerTitle: paramValue,
actions: actions,
buildTitle: (willCenterTitle) {
result = willCenterTitle;
return const Text('a');
}))));

await tester.pumpWidget(widget);
await tester.pump(); // global store
await tester.pump(); // per-account store
check(find.widgetWithText(ZulipAppBar, 'a')).findsOne();

check(result).isNotNull();
return result!;
}

void doTest(String description, bool expectedWillCenter, {
bool? paramValue,
bool? themeValue,
TargetPlatform? platform,
List<Widget>? actions,
}) {
testWidgets(description, (tester) async {
addTearDown(testBinding.reset);
debugDefaultTargetPlatformOverride = platform;

check(
await ours(tester, paramValue: paramValue, themeValue: themeValue, actions: actions)
)..equals(
await material(tester, paramValue: paramValue, themeValue: themeValue, actions: actions)
)..equals(expectedWillCenter);

// TODO(upstream) Do this in an addTearDown, once we can:
// https://github.com/flutter/flutter/issues/123189
debugDefaultTargetPlatformOverride = null;
});
}

const iOS = TargetPlatform.iOS;
const android = TargetPlatform.android;

Widget button() => IconButton(icon: const Icon(Icons.add), onPressed: () {});
final oneButton = [button()];
final twoButtons = [button(), button()];
final threeButtons = [button(), button(), button()];

doTest('ios', true, platform: iOS);
doTest('android', false, platform: android);

doTest('ios, theme false', false, platform: iOS, themeValue: false);
doTest('android, theme true', true, platform: android, themeValue: true);

doTest('ios, param false', false, platform: iOS, paramValue: false);
doTest('android, param true', true, platform: android, paramValue: true);

doTest('ios, theme true, param false', false, platform: iOS, themeValue: true, paramValue: false);
doTest('ios, theme false, param true', true, platform: iOS, themeValue: false, paramValue: true);

doTest('android, theme true, param false', false, platform: android, themeValue: true, paramValue: false);
doTest('android, theme false, param true', true, platform: android, themeValue: false, paramValue: true);

doTest('ios, no actions', true, platform: iOS, actions: null);
doTest('ios, one action', true, platform: iOS, actions: oneButton);
doTest('ios, two actions' , false, platform: iOS, actions: twoButtons);
doTest('ios, three actions', false, platform: iOS, actions: threeButtons);

doTest('ios, two actions but param true', true, platform: iOS, paramValue: true, actions: twoButtons);
doTest('ios, two actions but theme true', true, platform: iOS, themeValue: true, actions: twoButtons);
});
}

0 comments on commit 79ada37

Please sign in to comment.