From 997d1c33621884578212bff32df3a1fa8a7da3e3 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Mon, 11 Mar 2024 15:50:26 +0430 Subject: [PATCH] inbox: Fix collapsing a section from its sticky header 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: zulip#391. --- lib/widgets/inbox.dart | 28 +++++++++++- test/widgets/inbox_test.dart | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 8aa5172efc7..f924dfe92f8 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -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; @@ -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) @@ -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? @@ -304,6 +319,7 @@ class _AllDmsSection extends StatelessWidget { hasMention: data.hasMention, collapsed: collapsed, pageState: pageState, + sectionContext: context, ); return StickyHeaderItem( header: header, @@ -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; @@ -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); } }; @@ -427,6 +450,7 @@ class _StreamSection extends StatelessWidget { hasMention: data.hasMention, collapsed: collapsed, pageState: pageState, + sectionContext: context, ); return StickyHeaderItem( header: header, diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index b3cda6613af..a46453ecee6 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -51,6 +51,19 @@ void main() { await tester.pumpAndSettle(); } + List 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 setupVarious(WidgetTester tester) async { final stream1 = eg.stream(streamId: 1, name: 'stream 1'); @@ -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 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( @@ -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 @@ -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