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.

Sign up for GitHub

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

inbox: Fix collapsing a section from its sticky header #560

Merged
merged 1 commit into from
Mar 25, 2024
Merged

inbox: Fix collapsing a section from its sticky header #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions lib/widgets/inbox.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,19 @@ abstract class _HeaderItem extends StatelessWidget {
final int count;
final bool hasMention;

/// A build context within the [_StreamSection] or [_AllDmsSection].
///
/// Used to ensure the [_StreamSection] or [_AllDmsSection] that encloses the
/// current [_HeaderItem] is visible after being collapsed through this
/// [_HeaderItem].
final BuildContext sectionContext;

const _HeaderItem({
required this.collapsed,
required this.pageState,
required this.count,
required this.hasMention,
required this.sectionContext,
});

String get title;
Expand All @@ -223,7 +231,15 @@ abstract class _HeaderItem extends StatelessWidget {
Color get uncollapsedBackgroundColor;
Color? get unreadCountBadgeBackgroundColor;

void Function() get onCollapseButtonTap;
Future<void> Function() get onCollapseButtonTap => () async {
if (!collapsed) {
await Scrollable.ensureVisible(
sectionContext,
alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
);
}
};

void Function() get onRowTap;

@override
Expand Down Expand Up @@ -271,6 +287,7 @@ class _AllDmsHeaderItem extends _HeaderItem {
required super.pageState,
required super.count,
required super.hasMention,
required super.sectionContext,
});

@override get title => 'Direct messages'; // TODO(i18n)
Expand All @@ -280,7 +297,8 @@ class _AllDmsHeaderItem extends _HeaderItem {
@override get uncollapsedBackgroundColor => const Color(0xFFF3F0E7);
@override get unreadCountBadgeBackgroundColor => null;

@override get onCollapseButtonTap => () {
@override get onCollapseButtonTap => () async {
await super.onCollapseButtonTap();
pageState.allDmsCollapsed = !collapsed;
};
@override get onRowTap => onCollapseButtonTap; // TODO open all-DMs narrow?
Expand All @@ -304,6 +322,7 @@ class _AllDmsSection extends StatelessWidget {
hasMention: data.hasMention,
collapsed: collapsed,
pageState: pageState,
sectionContext: context,
);
return StickyHeaderItem(
header: header,
Expand Down Expand Up @@ -386,6 +405,7 @@ class _StreamHeaderItem extends _HeaderItem {
required super.pageState,
sm-sayedi marked this conversation as resolved.
Show resolved Hide resolved
required super.count,
required super.hasMention,
required super.sectionContext,
});

@override get title => subscription.name;
Expand All @@ -397,7 +417,8 @@ class _StreamHeaderItem extends _HeaderItem {
@override get unreadCountBadgeBackgroundColor =>
subscription.colorSwatch().unreadCountBadgeBackground;

@override get onCollapseButtonTap => () {
@override get onCollapseButtonTap => () async {
await super.onCollapseButtonTap();
if (collapsed) {
pageState.uncollapseStream(subscription.streamId);
} else {
Expand Down Expand Up @@ -427,6 +448,7 @@ class _StreamSection extends StatelessWidget {
hasMention: data.hasMention,
collapsed: collapsed,
pageState: pageState,
sectionContext: context,
);
return StickyHeaderItem(
header: header,
Expand Down
2 changes: 2 additions & 0 deletions test/example_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ final Account otherAccount = account(

final User thirdUser = user(fullName: 'Third User', email: 'third@example');

final User fourthUser = user(fullName: 'Fourth User', email: 'fourth@example');

////////////////////////////////////////////////////////////////
// Streams and subscriptions.
//
Expand Down
115 changes: 114 additions & 1 deletion test/widgets/inbox_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,38 @@ import '../flutter_checks.dart';
import '../model/binding.dart';
import '../model/test_store.dart';

/// Repeatedly drags `view` by `moveStep` until `finder` is invisible.
///
/// Between each drag, advances the clock by `duration`.
///
/// Throws a [StateError] if `finder` is still visible after `maxIteration`
/// drags.
///
/// See also:
/// * [WidgetController.dragUntilVisible], which does the inverse.
Future<void> dragUntilInvisible(
WidgetTester tester,
FinderBase<Element> finder,
FinderBase<Element> view,
Offset moveStep, {
int maxIteration = 50,
Duration duration = const Duration(milliseconds: 50),
}) {
return TestAsyncUtils.guard<void>(() async {
final iteration = maxIteration;
while (maxIteration > 0 && finder.evaluate().isNotEmpty) {
await tester.drag(view, moveStep);
await tester.pump(duration);
maxIteration -= 1;
}
if (maxIteration <= 0 && finder.evaluate().isNotEmpty) {
throw StateError(
'Finder is still visible after $iteration iterations.'
' Consider increasing the number of iterations.');
}
});
}

void main() {
TestZulipBinding.ensureInitialized();

Expand Down Expand Up @@ -51,6 +83,15 @@ void main() {
await tester.pumpAndSettle();
}

List<StreamMessage> generateStreamMessages({
required ZulipStream stream,
required int count,
required List<MessageFlag> flags,
}) {
return List.generate(count, (index) => eg.streamMessage(
stream: stream, topic: '${stream.name} topic $index', flags: flags));
}

/// Set up an inbox view with lots of interesting content.
Future<void> setupVarious(WidgetTester tester) async {
final stream1 = eg.stream(streamId: 1, name: 'stream 1');
Expand All @@ -61,12 +102,16 @@ void main() {
await setupPage(tester,
streams: [stream1, stream2],
subscriptions: [sub1, sub2],
users: [eg.selfUser, eg.otherUser, eg.thirdUser],
users: [eg.selfUser, eg.otherUser, eg.thirdUser, eg.fourthUser],
unreadMessages: [
eg.streamMessage(stream: stream1, topic: 'specific topic', flags: []),
...generateStreamMessages(stream: stream1, count: 10, flags: []),
eg.streamMessage(stream: stream2, flags: []),
...generateStreamMessages(stream: stream2, count: 40, flags: []),
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []),
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser, eg.thirdUser], flags: []),
eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], flags: []),
eg.dmMessage(from: eg.fourthUser, to: [eg.selfUser], flags: []),
]);
sm-sayedi marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -310,6 +355,28 @@ void main() {
checkAppearsUncollapsed(tester, findSectionContent);
});

testWidgets('collapse all-DMs section when partially offscreen: '
'header remains sticky at top', (tester) async {
await setupVarious(tester);

final listFinder = find.byType(Scrollable);
final dmFinder = find.text(eg.otherUser.fullName).hitTestable();

// Scroll part of [_AllDmsSection] offscreen.
await dragUntilInvisible(
tester, dmFinder, listFinder, const Offset(0, -50));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still used the variables instead of directly populating the finders for better readability, but please let me know if you think the other way is better!

Copy link
Member

Choose a reason for hiding this comment

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

For this dragUntilInvisible call, it can go either way — there's some value in that variable name dmFinder in particular. So arranging it this way is fine.

For headerRow below, though, definitely better to inline. When the definition reads:

          final headerRow = findAllDmsHeaderRow(tester);

there's not really anything the variable name is expressing that wouldn't also be clear from seeing the expression findAllDmsHeaderRow(tester) directly. Similarly headerRowAfterTap.


// Check that the header is present (which must therefore
// be as a sticky header).
check(findAllDmsHeaderRow(tester)).isNotNull();

await tapCollapseIcon(tester);

// Check that the header is still visible even after
// collapsing the section.
check(findAllDmsHeaderRow(tester)).isNotNull();
});

// TODO check it remains collapsed even if you scroll far away and back

// TODO check that it's always uncollapsed when it appears after being
Expand Down Expand Up @@ -385,6 +452,52 @@ void main() {
checkAppearsUncollapsed(tester, 1, findSectionContent);
});

testWidgets('collapse stream section when partially offscreen: '
'header remains sticky at top', (tester) async {
await setupVarious(tester);

final topicFinder = find.text('stream 1 topic 4').hitTestable();
final listFinder = find.byType(Scrollable);

// Scroll part of [_StreamSection] offscreen.
await dragUntilInvisible(
tester, topicFinder, listFinder, const Offset(0, -50));

// Check that the header is present (which must therefore
// be as a sticky header).
check(findStreamHeaderRow(tester, 1)).isNotNull();

await tapCollapseIcon(tester, 1);

// Check that the header is still visible even after
// collapsing the section.
check(findStreamHeaderRow(tester, 1)).isNotNull();
});

testWidgets('collapse stream section in middle of screen: '
'header stays fixed', (tester) async {
await setupVarious(tester);

final headerRow = findStreamHeaderRow(tester, 1);
Copy link
Collaborator Author

@sm-sayedi sm-sayedi Mar 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The test for #560 comment was included in the previous revision, just added a few comments in this one.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, thanks.

// Check that the header is present.
check(headerRow).isNotNull();

final rectBeforeTap = tester.getRect(find.byWidget(headerRow!));
final scrollableTop = tester.getRect(find.byType(Scrollable)).top;
// Check that the header is somewhere in the middle of the screen.
check(rectBeforeTap.top).isGreaterThan(scrollableTop);

await tapCollapseIcon(tester, 1);

final headerRowAfterTap = findStreamHeaderRow(tester, 1);
final rectAfterTap =
tester.getRect(find.byWidget(headerRowAfterTap!));

// Check that the position of the header before and after
// collapsing is the same.
check(rectAfterTap).equals(rectBeforeTap);
});

// TODO check it remains collapsed even if you scroll far away and back

// TODO check that it's always uncollapsed when it appears after being
Expand Down
Loading