Fix all remaining stream names to be IDs, modulo server support#5268
Merged
gnprice merged 13 commits intozulip:mainfrom Mar 2, 2022
Merged
Fix all remaining stream names to be IDs, modulo server support#5268gnprice merged 13 commits intozulip:mainfrom
gnprice merged 13 commits intozulip:mainfrom
Conversation
Member
Author
|
(Just pushed a rebase, since there were conflicts.) |
This includes a variety of conveniences; the one that brought it to my attention today is `bundleOf`, which we'll use next. Use the latest stable version, 1.7.0: https://developer.android.com/jetpack/androidx/releases/core Include a tweak which becomes needed in order to keep the build working.
This sets us up to push the Bundle out of this method entirely. That in turn will allow us to write unit tests for it -- Bundle is an Android framework class that isn't available in unit tests.
This will let us write unit tests for it.
A small code cleanup, since we now have this and were just looking at it.
chrisbobbe
approved these changes
Mar 2, 2022
| null -> "stream:${fcmMessage.recipient.streamName}\u0000${fcmMessage.recipient.topic}" | ||
|
|
||
| // The conditional use of streamId means a slight glitch: when upgrading either the | ||
| // client or server (whichever comes later) so that we start using stream names, any |
Contributor
There was a problem hiding this comment.
when upgrading […] so that we start using stream names
I think you might mean IDs instead of names?
This doesn't yet do anything with this information -- that's coming next -- so it's NFC as long as the server is behaving properly. It's not NFC overall, though, because if the payload includes a `stream_id` property that isn't a number, we'll now reject the notification as malformed.
This gives correct behavior if the stream is renamed while a conversation is ongoing.
For each of these components, the default behavior is already to look at the item's `key` property.
These `key` properties just get used as section keys in the SectionList we create in UnreadCards. SectionList docs: https://reactnative.dev/docs/sectionlist#section don't quite say that they get used as the `key` pseudo-prop of some React components internally; but they do say that they have basically the same effect as such React keys would. This change is arguably not *quite* NFC, because it changes how these keys behave if a stream is renamed. Previously it'd make a fresh React component -- or if one stream was renamed to another's old name between rerenders, it'd reuse the other stream's old component. Now it'll reuse the same stream's old component. I believe it still ends up being NFC -- apart from a possible perf improvement, which is negligible because it's such an uncommon occasion and the old behavior wasn't having any catastrophic perf impact -- because I don't think there's any local state on the components we have in the subtrees these act as the keys of.
… an ID The example was still valid for the point this section is making. But there are lots of other examples to choose from, so pick one that doesn't serve as a bad example in another respect.
This is (as the comment already says) blocked behind a minimum-server upgrade, so it's not really actionable on its own. Instead, connect the TODO to that upgrade.
Member
Author
|
Thanks for the review! Merging, with that fix. I also auto-formatted the files, having rebased past #5267 which did that more broadly. That caused one formatting change in FcmMessageTest.kt. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows #5246, #5223, #5205, #5183, #5069, #5056, #5000, and #4635 in switching us over from identifying streams by their names to doing so with their stable numeric IDs: #3918. This prevents buggy behavior when a stream gets renamed.
At this point the bulk of the work has been done already. The previous PR in this series, #5246, marked with
TODO(#3918):all the remaining places we needed to fix. This PR then eliminates each of those in turn. Mostly this means parsing and using stream IDs when present in notifications; that's followed by a handful of trivial fixes.As forecast at #3918 (comment) , there remain some places where we use stream names that should be IDs, but these are limited to interacting with the server in places where it doesn't support IDs. For those, moreover:
TODO(server-2.0):. We'll take care of those sometime soon in a followup to api: Start relying more on non-ancient servers #5100, as we concretely drop support for ancient server versions we already don't support.TODO(server-5.0):. We'll take care of those circa late 2023, when we drop support for all server releases that exist today. In the meantime, we make sure to take advantage of stream IDs when the server is new enough to provide them.TODO(server-future):comments.Fixes: #3918