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

sm-sayedi · 2024-03-11T12:56:48Z

Collapsing a stream/all-DMs section through a sticky header behaves as expected without shifting other sections off the screen.

Fixes: #391.

lib/widgets/inbox.dart

gnprice

Very interesting, thanks @sm-sayedi!

The behavior here looks exactly right for stream sections. As #391 says, we'll also want the same behavior for the DMs section.

See other comments below about ways of organizing this logic and of writing the tests.

lib/widgets/inbox.dart

test/widgets/inbox_test.dart

lib/widgets/inbox.dart

sm-sayedi · 2024-03-13T20:16:00Z

I have updated my PR as per your review and with the same behavior added for the DMs section. I have also updated the tests for it. Please have a look. Thanks!

test/widgets/inbox_test.dart

gnprice

Thanks @sm-sayedi for the revision! The implementation now all looks good, modulo a few small comments. There's some more work still needed on the tests.

lib/widgets/inbox.dart

test/widgets/inbox_test.dart

lib/widgets/inbox.dart

test/widgets/inbox_test.dart

sm-sayedi · 2024-03-22T19:11:30Z

Thanks for the review! Revision pushed, with no changes for the tests yet. Following are some of the comments!

test/widgets/inbox_test.dart

sm-sayedi · 2024-03-23T00:05:03Z

I have pushed the latest revision. Please have a look!

gnprice

Thanks @sm-sayedi for the revision! These tests now generally look good too. Some small comments below.

We'll also want another kind of test as mentioned at: #560 (comment)

test/widgets/inbox_test.dart

sm-sayedi · 2024-03-23T07:21:03Z

Thanks for the review! Pushed the latest revision. Please have a look.

sm-sayedi · 2024-03-23T07:28:31Z

test/widgets/inbox_test.dart

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


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!

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.

sm-sayedi · 2024-03-23T07:43:14Z

test/widgets/inbox_test.dart

+        testWidgets('collapse stream section in the middle of screen', (tester) async {
+          await setupVarious(tester);
+
+          final headerRow = findStreamHeaderRow(tester, 1);


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

Ah, I see, thanks.

gnprice

Thanks for the revision! This is very close now. A couple of comments on that one test I didn't spot in the last revision, and some stylistic comments elsewhere.

gnprice · 2024-03-25T03:54:07Z

lib/widgets/inbox.dart

@@ -209,11 +209,19 @@ abstract class _HeaderItem extends StatelessWidget {
  final int count;
  final bool hasMention;

+  /// Context to access the [_StreamSection] or [_AllDmsSection].


Suggested change

/// Context to access the [_StreamSection] or [_AllDmsSection].

/// A build context within the [_StreamSection] or [_AllDmsSection].

This makes it easier to tell, when looking at code that constructs one of these widgets, what would be a valid value to pass for this field.

gnprice · 2024-03-25T03:54:59Z

lib/widgets/inbox.dart

+  };
  void Function() get onRowTap;


nit: blank line as separator, now that these aren't both just one line each:

Suggested change

};

void Function() get onRowTap;

};

void Function() get onRowTap;

gnprice · 2024-03-25T03:59:37Z

test/widgets/inbox_test.dart

+/// Throws a [StateError] if `finder` is still visible after `maxIteration`
+/// drags.
+Future<void> dragUntilInvisible(


Suggested change

/// Throws a [StateError] if `finder` is still visible after `maxIteration`

/// drags.

Future<void> dragUntilInvisible(

/// Throws a [StateError] if `finder` is still visible after `maxIteration`

/// drags.

///

/// See also:

/// * [WidgetController.dragUntilVisible], which does the inverse.

Future<void> dragUntilInvisible(

This helps draw the connection to the method it's based on. If sending this upstream, we'd want a similar "see also" link here and also one pointing in the other direction, to help make both of them more discoverable.

gnprice · 2024-03-25T04:04:15Z

test/widgets/inbox_test.dart

+        testWidgets('collapse stream section in the middle of screen', (tester) async {
+          await setupVarious(tester);
+
+          final headerRow = findStreamHeaderRow(tester, 1);


Ah, I see, thanks.

gnprice · 2024-03-25T04:04:52Z

test/widgets/inbox_test.dart

+          final rectBeforeTap = tester.getRect(find.byWidget(headerRow!));
+
+          // Check that the header is present (which must therefore
+          // be somewhere in the middle of screen)
+          check(headerRow).isNotNull();


The isNotNull check should come before the ! — otherwise it's impossible for the check to fail, because if headerRow were null we'd have already thrown at headerRow!.

gnprice · 2024-03-25T04:16:50Z

test/widgets/inbox_test.dart

+          // Check that the header is present (which must therefore
+          // be somewhere in the middle of screen)


Hmm, but why does this mean it's in the middle of the screen? As far as one can see from reading this test's own code, it seems like it could just as well be the sticky header at the top of the viewport.

The reason it's not at the top is because there are unread DMs too, and so there's a DMs section above it. But that's a detail of setupVarious, and it seems quite possible that someone in the future could change that, causing this test to just always pass even if the functionality it's meant to test subsequently gets broken. After all, when they do make that change, nothing in this test will break.

There are two basic ways to solve that sort of problem:

Add a check within this test that verifies that the setup meets the conditions that this test specifically needs from it.

Specify the relevant details directly in the test, rather than far away in a place where it's not obvious that there are tests that rely on them.

For example here:

For the first approach, check that the top of rectBeforeTap is somewhere below the top of the scrollable viewport.

Or for the second approach, instead of using setupVarious, call setupPage directly. That setup can then be simpler than what setupVarious does, focusing only on the details this test needs. For example: one DM conversation, then a stream with just one conversation, then a second stream with lots of conversations (enough so that it's clearly more than a screenful, so that scrolling is possible).

Here I think either approach would work fine. (Only one is needed, not both.)

gnprice · 2024-03-25T04:20:28Z

test/widgets/inbox_test.dart

+          check(headerRowAfterTap).isNotNull();
+        });
+
+        testWidgets('collapse stream section in the middle of screen', (tester) async {


I think mentioning here the intended outcome (though very concisely) would help the reader understand the test. I suspect it'd have helped me when I overlooked this test previously: #560 (comment)

So:

Suggested change

testWidgets('collapse stream section in the middle of screen', (tester) async {

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

gnprice · 2024-03-25T04:23:47Z

test/widgets/inbox_test.dart

@@ -385,6 +450,50 @@ void main() {
          checkAppearsUncollapsed(tester, 1, findSectionContent);
        });

+        testWidgets('collapse stream section after scroll', (tester) async {


Similarly to the other test description, let's mention briefly the expected outcome:

Suggested change

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

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

That version also gets a bit more precise than "after scroll", which could mean a lot of different things — for example if we had some bug that was triggered by scrolling down, then scrolling back up to the same spot, then doing something, then a test for that bug's fix could equally well be described with "after scroll".

gnprice · 2024-03-25T04:24:38Z

test/widgets/inbox_test.dart

+        testWidgets('collapse stream section after scroll', (tester) async {
+          await setupVarious(tester);
+
+          final topicFinder = find.text('stream 1 topic 4' ).hitTestable();


nit:

Suggested change

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

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

gnprice · 2024-03-25T04:33:10Z

test/widgets/inbox_test.dart

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


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.

sm-sayedi · 2024-03-25T05:40:14Z

Thanks for the review. On it!

Just opened a draft PR as the tests are remaining. Would love to get a review on it.

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

sm-sayedi · 2024-03-25T06:59:31Z

Revisions pushed! Ready for another review!

gnprice · 2024-03-25T19:49:41Z

Looks good — merging! Thanks @sm-sayedi for all your work on this.

sm-sayedi · 2024-03-25T19:58:39Z

Thank you so much @gnprice !! I appreciate your patience and excellent guidance throughout!!

chrisbobbe · 2024-03-25T22:15:13Z

Yay! I remember feeling disappointed that this fix wasn't in my original implementation of the inbox page. I'm glad it's done now.

sm-sayedi · 2024-03-26T05:01:31Z

Happy we got it done together!!

sm-sayedi commented Mar 11, 2024

View reviewed changes

lib/widgets/inbox.dart Outdated Show resolved Hide resolved

sm-sayedi commented Mar 11, 2024

View reviewed changes

lib/widgets/inbox.dart Show resolved Hide resolved

gnprice reviewed Mar 11, 2024

View reviewed changes

sm-sayedi force-pushed the issue-391-autoscroll-stream-header branch 4 times, most recently from 997d1c3 to 6e5a272 Compare March 13, 2024 19:59

sm-sayedi commented Mar 13, 2024

View reviewed changes

test/widgets/inbox_test.dart Show resolved Hide resolved

gnprice reviewed Mar 20, 2024

View reviewed changes

gnprice mentioned this pull request Mar 20, 2024

autocomplete: Add user avatars to user-mention autocompletes #227

Closed

sm-sayedi force-pushed the issue-391-autoscroll-stream-header branch 2 times, most recently from 97c8a93 to ca7d5bc Compare March 22, 2024 18:54

sm-sayedi commented Mar 22, 2024

View reviewed changes

test/widgets/inbox_test.dart Outdated Show resolved Hide resolved

sm-sayedi force-pushed the issue-391-autoscroll-stream-header branch 3 times, most recently from c3bce0e to a0483bd Compare March 23, 2024 00:01

gnprice reviewed Mar 23, 2024

View reviewed changes

sm-sayedi force-pushed the issue-391-autoscroll-stream-header branch from a0483bd to 6b956dc Compare March 23, 2024 07:15

sm-sayedi commented Mar 23, 2024

View reviewed changes

gnprice reviewed Mar 25, 2024

View reviewed changes

sm-sayedi force-pushed the issue-391-autoscroll-stream-header branch from 6b956dc to 13e890c Compare March 25, 2024 06:49

gnprice merged commit 13e890c into zulip:main Mar 25, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sm-sayedi commented Mar 11, 2024 •

edited

Loading

gnprice left a comment

sm-sayedi commented Mar 13, 2024 •

edited

Loading

gnprice left a comment

sm-sayedi commented Mar 22, 2024

sm-sayedi commented Mar 23, 2024

gnprice left a comment

sm-sayedi commented Mar 23, 2024

sm-sayedi Mar 23, 2024

gnprice Mar 25, 2024

sm-sayedi Mar 23, 2024 •

edited

Loading

gnprice Mar 25, 2024

gnprice left a comment

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

gnprice Mar 25, 2024

sm-sayedi commented Mar 25, 2024

sm-sayedi commented Mar 25, 2024

gnprice commented Mar 25, 2024

sm-sayedi commented Mar 25, 2024

chrisbobbe commented Mar 25, 2024

sm-sayedi commented Mar 26, 2024

	/// Context to access the [_StreamSection] or [_AllDmsSection].
	/// A build context within the [_StreamSection] or [_AllDmsSection].

		// Check that the header is present (which must therefore
		// be somewhere in the middle of screen)

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

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

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

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

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

Conversation

sm-sayedi commented Mar 11, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

sm-sayedi commented Mar 13, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

sm-sayedi commented Mar 22, 2024

sm-sayedi commented Mar 23, 2024

gnprice left a comment

Choose a reason for hiding this comment

sm-sayedi commented Mar 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi commented Mar 25, 2024

sm-sayedi commented Mar 25, 2024

gnprice commented Mar 25, 2024

sm-sayedi commented Mar 25, 2024

chrisbobbe commented Mar 25, 2024

sm-sayedi commented Mar 26, 2024

sm-sayedi commented Mar 11, 2024 •

edited

Loading

sm-sayedi commented Mar 13, 2024 •

edited

Loading

sm-sayedi Mar 23, 2024 •

edited

Loading