Skip to content

Fetch server emoji data (about Unicode emoji) #976

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

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Oct 2, 2024

Stacked atop #975.

This completes the data we'll need for use in #669.

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Oct 2, 2024
@chrisbobbe chrisbobbe self-requested a review October 5, 2024 01:30
@chrisbobbe chrisbobbe self-assigned this Oct 5, 2024
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One comment below.

@@ -68,6 +68,8 @@ class InitialSnapshot {

final int maxFileUploadSizeMib;

final Uri serverEmojiDataUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README still has our support threshold at server 4.0, so what we'd normally do is make this optional with a server-6 / FL 140 TODO, right, even if it only stays that way for a short time before we bump the threshold high enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably best to handle that explicitly. We can skip actually trying to provide data for those servers (since they're already out of the 18-month support window), though.

This is NFC because all existing call sites (all of which are in
the other methods on this class) do always pass URLs that are
based on realmUrl and preserve its origin.
This will let us reuse ApiConnection for some specialty endpoints that
aren't authenticated (or don't use the normal Zulip auth mechanism).
We'll need another of these for fetching emoji data.  Let's move them
out from amidst all the main logic, to keep down the clutter.
This completes the data we'll need for use in zulip#669, which in turn
will let us offer an emoji picker for reactions and for use
inside message content.

The doc on fetchEmojiData is based on a comment at the corresponding
code in the legacy zulip-mobile app, in src/events/eventActions.js .
@gnprice
Copy link
Member Author

gnprice commented Oct 9, 2024

Thanks for the review! Pushed a revision.

@chrisbobbe
Copy link
Collaborator

When you say "handle explicitly" in #976 (comment), do you mean making InitialSnapshot.serverEmojiDataUrl optional with a TODO? I noticed that isn't done in this revision. 🙂

@gnprice
Copy link
Member Author

gnprice commented Oct 9, 2024

Oops, I didn't push my latest revision. 🙂 Pushed now.

@chrisbobbe
Copy link
Collaborator

Thanks! LGTM; please merge at will once CI has passed.

@gnprice gnprice merged commit f5d09c4 into zulip:main Oct 10, 2024
1 check passed
@gnprice gnprice deleted the pr-emoji branch October 12, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants