Normalize topic name when opening message list #1718
Conversation
lib/notifications/open.dart
Outdated
| return MessageListPage.buildRoute( | ||
| return MaterialAccountWidgetRoute( | ||
| accountId: account.id, | ||
| // TODO(#1565): Open at specific message, not just conversation | ||
| narrow: data.narrow); | ||
| page: Builder(builder: (context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| var narrow = data.narrow; | ||
| if (narrow is TopicNarrow) { | ||
| narrow = narrow.processTopicLikeServer( | ||
| zulipFeatureLevel: store.zulipFeatureLevel, | ||
| realmEmptyTopicDisplayName: store.realmEmptyTopicDisplayName); |
There was a problem hiding this comment.
Hmm, I see. The normalization is a bit sticky to do at this spot, because when this method is running we don't yet have a per-account store. (In particular the app may have really not yet loaded the data from the server.) So we don't know realmEmptyTopicDisplayName, and have to make a route before we've done the normalization.
I think it'd be best to let this continue to use a proper MessageListPage route, though, and let MessageListPage or MessageList take the responsibility for normalizing the narrow. They already have the capability to do something similar, because the initial fetch might discover the topic has been moved. That's even later than the point where we have the store and can do this normalization.
Specifically I think _MessageListState.onNewStore looks like a good place to handle this.
rajveermalviya
force-pushed
the
pr-msglist-general-chat
branch
from
7f63f71 to
0893924
Compare
| // Normalize topic name if this is a TopicNarrow. See #1717. | ||
| var narrow = widget.narrow; | ||
| if (narrow is TopicNarrow) { | ||
| narrow = narrow.processTopicLikeServer( |
There was a problem hiding this comment.
This should also notify the MessageListPage about the change.
In particular that will cause the app bar to put "general chat" in italics.
There was a problem hiding this comment.
We already notify (albeit a bit later) as a side effect of model.addListener(_modelChanged) below.
If we want to do it here we would need to use something like, because ancestor calls setState in onNarrowChanged:
SchedulerBinding.instance.scheduleFrameCallback((_) {
widget.onNarrowChanged(narrow);
});There was a problem hiding this comment.
I see. Yeah, I think that would be better — that will make sure it happens for the next frame, rather than waiting until the model updates for some other reason.
That explicit approach is also good because it means a reader of this code doesn't have to wonder about how and when the change makes it to the page widget.
There was a problem hiding this comment.
(In manual testing, I tried commenting that call back out to see the effect — the page ends up continuing to show the old value until the message list finishes fetching the initial messages from the server. That means "general chat" isn't in italics; and a bit more substantively, any applicable topic-visibility icon is missing.
So indeed calling onNarrowChanged explicitly here prevents a user-visible glitch, in addition to making it easier for a reader to become confident that things work.)
rajveermalviya
force-pushed
the
pr-msglist-general-chat
branch
2 times, most recently
from
26f5b51 to
1e90bc5
Compare
rajveermalviya
added
the
integration review
rajveermalviya
force-pushed
the
pr-msglist-general-chat
branch
from
1e90bc5 to
a0e528c
Compare
lib/widgets/message_list.dart
Outdated
| SchedulerBinding.instance.scheduleFrameCallback((_) { | ||
| widget.onNarrowChanged(narrow); |
There was a problem hiding this comment.
This is only needed if the narrow is different from its original value — let's avoid calling it when nothing changed.
rajveermalviya
force-pushed
the
pr-msglist-general-chat
branch
from
a0e528c to
c1a8f44
Compare
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks for fixing this! The logic looks good now. Two comments below.
I'll add that commit clarifying the test, and merge.
| // Normalize topic name if this is a TopicNarrow. See #1717. | ||
| var narrow = widget.narrow; | ||
| if (narrow is TopicNarrow) { | ||
| narrow = narrow.processTopicLikeServer( |
There was a problem hiding this comment.
(In manual testing, I tried commenting that call back out to see the effect — the page ends up continuing to show the old value until the message list finishes fetching the initial messages from the server. That means "general chat" isn't in italics; and a bit more substantively, any applicable topic-visibility icon is missing.
So indeed calling onNarrowChanged explicitly here prevents a user-visible glitch, in addition to making it easier for a reader to become confident that things work.)
test/widgets/message_list_test.dart
Outdated
| testWidgets('MessageListPageState.narrow (general chat)', (tester) async { | ||
| final stream = eg.stream(); | ||
| final topic = eg.defaultRealmEmptyTopicDisplayName; | ||
| final topicNarrow = eg.topicNarrow(stream.streamId, topic); | ||
| await setupMessageListPage(tester, narrow: topicNarrow, | ||
| streams: [stream], | ||
| messages: [eg.streamMessage(stream: stream, topic: topic, content: "<p>a message</p>")]); | ||
| final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); | ||
| check(state.narrow).equals(eg.topicNarrow(stream.streamId, '')); |
There was a problem hiding this comment.
I think this test works, at the level that if we were to re-introduce this bug, the test would fail.
There's another layer of what a test needs to do to be fully successful at its job: when the test does fail in that situation, it needs to tell its story. (I go into more detail on this in the talk I gave last month at Fluttercon; hoping for the talk video to get posted soon so I can share it.) The person who's made the change needs to be able to look at the test and figure out why the test's author thought it was important for things to behave this way; they need to be able to determine whether the breakage is pointing at a real bug in their change, or just means the test needs to be updated to match their change. If they can't tell that, then it's likely they'll just edit the test to match the new (broken) behavior, and the test won't have done its job of preventing the regression.
I think this version doesn't really tell the story. It looks very much like the test above it — but that test is just testing some boring plumbing. In this test, there's a critical contrast where a piece of data (the topic name) is different at one stage from an earlier stage; and there's a reason why that particular change should happen and why it's important. The contrast is easy to miss when scanning this code; and once spotted, without the reason, it almost looks like a bug instead of an important feature.
I'll add a commit to fix this aspect up and demonstrate telling the test's story.
This is a helper function around `TopicName.processLikeServer`, where it creates a new `TopicNarrow` with the processed topic name.
gnprice
force-pushed
the
pr-msglist-general-chat
branch
from
c1a8f44 to
09203d1
Compare
2 participants
Fixes: #1717