-
Notifications
You must be signed in to change notification settings - Fork 340
Rely on Zulip Server 6 APIs #1869
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
Conversation
f29237b
to
4a8cebc
Compare
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 @chrisbobbe! LGTM, moving over to Greg's review.
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 cleaning these up! Generally all looks good. A few comments below.
In addition to the TODO-server comments in our code, I also took this as a prompt to read through the API changelog https://zulip.com/api/changelog for Server 6. Items I found there which should also be part of relying on Server 6:
- FL-139: no more
in_home_view
insubscription/update
events - FL-148: no more
away
in user status, which we had commented out
Also grepped -i
for server.?6
at the tip of the branch; the only match was that FL-148 removal of away
.
final UserSettings userSettings; | ||
|
||
final List<UserTopicItem>? userTopics; // TODO(server-6) | ||
final List<UserTopicItem> userTopics; |
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.
This replaced muted_topics
. Grepping (with git grep -i
muted.?topic`), we have a commented-out reference to that, above, which we can delete.
Also a comment in the list of event types.
test/example_data.dart
Outdated
GetMessagesResult olderGetMessagesResult({ | ||
required int anchor, | ||
bool foundAnchor = false, // the value if the server understood includeAnchor false | ||
bool foundAnchor = false, |
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.
This should mean we can drop this parameter, right? And just hard-code the value in this test helper's body. (Because we no longer need to exercise scenarios where it has any other value.)
topics.update(topic, | ||
ifAbsent: () => messageIds, | ||
// setUnion dedupes existing and incoming unread IDs, | ||
// so we tolerate zulip/zulip#22164, fixed in 6.0 | ||
// TODO(server-6) remove 6.0 comment | ||
(existing) => setUnion(existing, messageIds), | ||
); | ||
(existing) => setUnion(existing, messageIds)); | ||
} | ||
|
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.
This comment appears to have been explaining why we had this setUnion
. Do we no longer need it?
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.
We discussed this in the office and decided to keep it, because it ensures the result is sorted when existing
and messageIds
are disjoint.
4a8cebc
to
0d63468
Compare
Thanks for the review! Revision pushed. |
Thanks!
This small piece is still open. One nit here:
This change doesn't ignore Otherwise all LGTM. |
0d63468
to
04595cd
Compare
Ah yep, thanks for those catches. Revision pushed! |
Issue zulip#992 was closed as a duplicate of zulip#1837, and zulip#1838 exists as the issue for TODO(server-7) comments.
The doc for this says > This field is only included when its value is `true`. so the TODO(server-6) isn't reason enough to make it a required field. Fixes-partly: zulip#1837
This isn't a case of special backward-compat code to be trimmed out; the backwards compatibility was simply that older servers wouldn't exercise this code. Fixes-partly: zulip#1837
…read" Fixes-partly: zulip#1837
…value Fixes-partly: zulip#1837
… bugs Fixes-partly: zulip#1837
Our event poll shouldn't crash on subscription/update events about data that we don't store and act on. This bugfix allows servers to add new Subscription fields and send corresponding events without causing the app to discard its data and reregister a new event queue. Soon, for zulip#1837, we'd like to remove SubscriptionProperty.inHomeView, which was deprecated in FL 139. Events with in_home_view are still sent by modern servers (as of CZO 2025-10-03), but we never need them because they always come with an is_muted event. Now, when we remove SubscriptionProperty.inHomeView, the in_home_view event will parse instead of not parsing, and it'll have SubscriptionProperty.unknown for `property`, which the store already ignores.
See changelog for FL 139: `GET /get-events`: When a user mutes or unmutes their subscription to a stream, a `subscription` update event is now sent for the `is_muted` property and for the deprecated `in_home_view` property to support clients fully migrating to use the `is_muted` property. Prior to this feature level, only one event was sent to clients with the deprecated `in_home_view` property. Fixes-partly: zulip#1837
04595cd
to
6adc9c3
Compare
Thanks, revision pushed! |
Thanks! Looks good; merging. (The last revision was adjusting commit-message summary lines to use the prefix "api:", following a conversation in the office.) |
Fixes #1837.
This isn't our most urgent issue, but I was looking for some low-hanging fruit I could clear during yesterday's heat wave when it was really hot and uncomfortable at home. :)