Rename "All messages" to "Combined feed" in the codebase. #688
Conversation
| /// This does not literally mean all messages, or even all messages | ||
| /// that the user has access to: in particular it excludes muted streams | ||
| /// and topics. |
There was a problem hiding this comment.
I am not sure whether I should remove this comment now or not?
There was a problem hiding this comment.
Good question. I think we can replace it with something like:
/// All messages the user has access to, excluding unsubscribed streams
/// and muted streams and topics. See [PerAccountStore.isTopicVisible].There was a problem hiding this comment.
Sounds good done!
There was a problem hiding this comment.
Thanks! This is close to being ready.
I noticed that you included Fixes: #676 in both commits. Please only include the Fixes: #676 line in the one commit that completely fixes the issue; that is, the commit at which the "all messages" name no longer appears in the codebase. (If you want, you could include a Fixes-partly: #676 in the first commit, but I think we don't have a consistent pattern of doing that.)
Also, I find one remaining reference to AllMessagesNarrow, in test/widgets/message_list_test.dart.
| /// This does not literally mean all messages, or even all messages | ||
| /// that the user has access to: in particular it excludes muted streams | ||
| /// and topics. |
There was a problem hiding this comment.
Good question. I think we can replace it with something like:
/// All messages the user has access to, excluding unsubscribed streams
/// and muted streams and topics. See [PerAccountStore.isTopicVisible].
|
It looks like some conflicts have appeared. Please resolve those and comment when this is ready for another review. |
Lalit3716
force-pushed
the
dev
branch
2 times, most recently
from
724b48a
to
c3dcb57
Compare
|
Resolved the conflicts and removed the |
lib/model/narrow.dart
Outdated
| /// The narrow called "Combined feed" in the UI. | ||
| /// All messages the user has access to, excluding unsubscribed streams | ||
| /// and muted streams and topics. See [PerAccountStore.isTopicVisible]. |
There was a problem hiding this comment.
nit: empty line between dartdoc's summary line and body
chrisbobbe
added
the
integration review
There was a problem hiding this comment.
Thanks @Lalit3716 and @chrisbobbe!
Apart from the comment Chris made above, I reran the search from the issue, and it looks like there's one match remaining that should be updated:
$ git grep -i 'all.?messages'
…
lib/widgets/message_list.dart: // widgets. As a simple test, flinging through All Messages in
|
Updated ready for another look now! |
|
Thanks for the revision! Looks good — merging. |
Fixes: #676