-
Notifications
You must be signed in to change notification settings - Fork 221
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.
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
inbox: Fix collapsing a section from its sticky header #560
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
997d1c3
to
6e5a272
Compare
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
97c8a93
to
ca7d5bc
Compare
Thanks for the review! Revision pushed, with no changes for the tests yet. Following are some of the comments! |
c3bce0e
to
a0483bd
Compare
I have pushed the latest revision. Please have a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
a0483bd
to
6b956dc
Compare
Thanks for the review! Pushed the latest revision. Please have a look. |
|
||
// Scroll part of [_AllDmsSection] offscreen. | ||
await dragUntilInvisible( | ||
tester, dmFinder, listFinder, const Offset(0, -50)); |
There was a problem hiding this comment.
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 finder
s for better readability, but please let me know if you think the other way is better!
There was a problem hiding this comment.
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
.
testWidgets('collapse stream section in the middle of screen', (tester) async { | ||
await setupVarious(tester); | ||
|
||
final headerRow = findStreamHeaderRow(tester, 1); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/widgets/inbox.dart
Outdated
@@ -209,11 +209,19 @@ abstract class _HeaderItem extends StatelessWidget { | |||
final int count; | |||
final bool hasMention; | |||
|
|||
/// Context to access the [_StreamSection] or [_AllDmsSection]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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.
lib/widgets/inbox.dart
Outdated
}; | ||
void Function() get onRowTap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: blank line as separator, now that these aren't both just one line each:
}; | |
void Function() get onRowTap; | |
}; | |
void Function() get onRowTap; |
test/widgets/inbox_test.dart
Outdated
/// Throws a [StateError] if `finder` is still visible after `maxIteration` | ||
/// drags. | ||
Future<void> dragUntilInvisible( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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.
testWidgets('collapse stream section in the middle of screen', (tester) async { | ||
await setupVarious(tester); | ||
|
||
final headerRow = findStreamHeaderRow(tester, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, thanks.
test/widgets/inbox_test.dart
Outdated
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
.
test/widgets/inbox_test.dart
Outdated
// Check that the header is present (which must therefore | ||
// be somewhere in the middle of screen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, callsetupPage
directly. That setup can then be simpler than whatsetupVarious
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.)
test/widgets/inbox_test.dart
Outdated
check(headerRowAfterTap).isNotNull(); | ||
}); | ||
|
||
testWidgets('collapse stream section in the middle of screen', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
testWidgets('collapse stream section in the middle of screen', (tester) async { | |
testWidgets('collapse stream section in middle of screen: header stays fixed', (tester) async { |
test/widgets/inbox_test.dart
Outdated
@@ -385,6 +450,50 @@ void main() { | |||
checkAppearsUncollapsed(tester, 1, findSectionContent); | |||
}); | |||
|
|||
testWidgets('collapse stream section after scroll', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to the other test description, let's mention briefly the expected outcome:
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".
test/widgets/inbox_test.dart
Outdated
testWidgets('collapse stream section after scroll', (tester) async { | ||
await setupVarious(tester); | ||
|
||
final topicFinder = find.text('stream 1 topic 4' ).hitTestable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
final topicFinder = find.text('stream 1 topic 4' ).hitTestable(); | |
final topicFinder = find.text('stream 1 topic 4').hitTestable(); |
|
||
// Scroll part of [_AllDmsSection] offscreen. | ||
await dragUntilInvisible( | ||
tester, dmFinder, listFinder, const Offset(0, -50)); |
There was a problem hiding this comment.
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
.
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
6b956dc
to
13e890c
Compare
Revisions pushed! Ready for another review! |
Looks good — merging! Thanks @sm-sayedi for all your work on this. |
Thank you so much @gnprice !! I appreciate your patience and excellent guidance throughout!! |
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. |
Happy we got it done together!! |
Collapsing a stream/all-DMs section through a sticky header behaves as expected without shifting other sections off the screen.
Fixes: #391.