Skip to content

Commit

Permalink
inbox: Fix collapsing a section from its sticky header
Browse files Browse the repository at this point in the history 
Before this fix, when scrolled down through the inbox page, collapsing a
section (either the all-DMs section or a stream section) through its
sticky header would work but cause an abnormal behavior, pushing the
current section header and some of the following sections off the screen
by the amount of space the current section occupied.

After this fix, collapsing a section through a sticky header would work
as expected, without pushing the header and the following sections off
the screen.

Fixes: #391.
  • Loading branch information
@sm-sayedi
sm-sayedi committed Mar 13, 2024
1 parent 46b4577 commit 997d1c3
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 2 deletions.
28 changes: 26 additions & 2 deletions lib/widgets/inbox.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,20 @@ abstract class _HeaderItem extends StatelessWidget {
final _InboxPageState pageState;
final int count;
final bool hasMention;
/// This context is used to ensure the [_StreamSection] or [_AllDmsSection]
/// that encloses the current [_HeaderItem] is visible after being collapsed
/// through this [_HeaderItem] being used as a sticky header.
///
/// A non-null value should be passed if this [_HeaderItem]
/// is used as a sticky header.
final BuildContext sectionContext;

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

String get title;
Expand Down Expand Up @@ -271,6 +279,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 +289,13 @@ class _AllDmsHeaderItem extends _HeaderItem {
@override get uncollapsedBackgroundColor => const Color(0xFFF3F0E7);
@override get unreadCountBadgeBackgroundColor => null;

@override get onCollapseButtonTap => () {
@override get onCollapseButtonTap => () async{
if (!collapsed) {
await Scrollable.ensureVisible(
sectionContext,
alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
);
}
pageState.allDmsCollapsed = !collapsed;
};
@override get onRowTap => onCollapseButtonTap; // TODO open all-DMs narrow?
Expand All @@ -304,6 +319,7 @@ class _AllDmsSection extends StatelessWidget {
hasMention: data.hasMention,
collapsed: collapsed,
pageState: pageState,
sectionContext: context,
);
return StickyHeaderItem(
header: header,
Expand Down Expand Up @@ -386,6 +402,7 @@ class _StreamHeaderItem extends _HeaderItem {
required super.pageState,
required super.count,
required super.hasMention,
required super.sectionContext,
});

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

@override get onCollapseButtonTap => () {
@override get onCollapseButtonTap => () async {
if (collapsed) {
pageState.uncollapseStream(subscription.streamId);
} else {
if (sectionContext != null) {
await Scrollable.ensureVisible(
sectionContext!,
alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
);
}
pageState.collapseStream(subscription.streamId);
}
};
Expand All @@ -427,6 +450,7 @@ class _StreamSection extends StatelessWidget {
hasMention: data.hasMention,
collapsed: collapsed,
pageState: pageState,
sectionContext: context,
);
return StickyHeaderItem(
header: header,
Expand Down
83 changes: 83 additions & 0 deletions test/widgets/inbox_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,19 @@ void main() {
await tester.pumpAndSettle();
}

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

/// 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 @@ -64,12 +77,34 @@ void main() {
users: [eg.selfUser, eg.otherUser, eg.thirdUser],
unreadMessages: [
eg.streamMessage(stream: stream1, topic: 'specific topic', flags: []),
...generateStreamMessages(stream1, 10),
eg.streamMessage(stream: stream2, flags: []),
...generateStreamMessages(stream2, 40),
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []),
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser, eg.thirdUser], flags: []),
]);
}

/// Set up an inbox view with more content for scrolling.
Future<void> setupForScrolling(WidgetTester tester) async {
final stream1 = eg.stream(streamId: 1, name: 'stream 1');
final sub1 = eg.subscription(stream1);
final stream2 = eg.stream(streamId: 2, name: 'stream 2');
final sub2 = eg.subscription(stream2);
final stream3 = eg.stream(streamId: 3, name: 'stream 3');
final sub3 = eg.subscription(stream3);

await setupPage(tester,
streams: [stream1, stream2, stream3],
subscriptions: [sub1, sub2, sub3],
users: [eg.selfUser, eg.otherUser, eg.thirdUser],
unreadMessages: [
...generateStreamMessages(stream1, 10),
...generateStreamMessages(stream2, 10),
...generateStreamMessages(stream3, 40),
]);
}

/// Find a row with the given label.
Widget? findRowByLabel(WidgetTester tester, String label) {
final rowLabel = tester.widgetList(
Expand Down Expand Up @@ -310,6 +345,30 @@ void main() {
checkAppearsUncollapsed(tester, findSectionContent);
});

testWidgets('collapse all-DMs section after scroll', (tester) async {
await setupVarious(tester);

final listFinder = find.byType(Scrollable);
// Scroll the [StickyHeaderListView] enough so that
// the [_AllDmsSection] shows a sticky header
await tester.drag(listFinder, const Offset(0, -50));

final headerRow = findAllDmsHeaderRow(tester);
// Check that the sticky header is present
check(headerRow).isNotNull();

final findSectionContent = find.text(eg.otherUser.fullName);

checkAppearsUncollapsed(tester, findSectionContent);
await tapCollapseIcon(tester);
// Check that the header is still visible even after
// collapsing the section
check(headerRow).isNotNull();
checkAppearsCollapsed(tester, findSectionContent);
await tapCollapseIcon(tester);
checkAppearsUncollapsed(tester, findSectionContent);
});

// 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 +444,30 @@ void main() {
checkAppearsUncollapsed(tester, 1, findSectionContent);
});

testWidgets('collapse stream section after scroll', (tester) async {
await setupVarious(tester);

final listFinder = find.byType(Scrollable);
// Scroll the [StickyHeaderListView] enough so that
// the [_StreamSection] shows a sticky header
await tester.drag(listFinder, const Offset(0, -200));

final headerRow = findStreamHeaderRow(tester, 1);
// Check that the sticky header is present
check(headerRow).isNotNull();

final findSectionContent = find.text('specific topic');

checkAppearsUncollapsed(tester, 1, findSectionContent);
await tapCollapseIcon(tester, 1);
// Check that the header is still visible even after
// collapsing the section
check(headerRow).isNotNull();
checkAppearsCollapsed(tester, 1, findSectionContent);
await tapCollapseIcon(tester, 1);
checkAppearsUncollapsed(tester, 1, findSectionContent);
});

// 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

0 comments on commit 997d1c3

Please sign in to comment.