Skip to content
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

Better title group PMs in Android notifications #5116

Open
gnprice opened this issue Nov 9, 2021 · 0 comments
Open

Better title group PMs in Android notifications #5116

gnprice opened this issue Nov 9, 2021 · 0 comments

Comments

@gnprice
Copy link
Member

gnprice commented Nov 9, 2021

From #4842 (comment) , a followup after #4842 which gave us nice grouped notifications:

For group PMs, the conversation heading is like "from Greg Price with 1 other"; it names the sender (of the latest message) but not any of the other people in the thread.

The "and N others" form is unavoidable at some point when there are lots of people involved, because a list of like 20 names (or even 4 or 5, really) is not going to be comfortable in this UI. But it's pretty silly when it's a 3-way group-PM thread so that there are just 2 other names. It's likely to be frustrating for users in some circumstances.

This was a known issue, discussed at #4842 (comment). The difficulty is that while for the message's sender, the server includes their display name (and avatar URL), for the other recipients we have just their user IDs but no details like display names.

The options here are very much like in #5115, for the name of the org:

  • We could have the server start including it in notification payloads. That'd start working promptly for users on Zulip Cloud and other servers regularly updated from Git. We'd fall back to the current text when it's not available.
  • This information is present in the Redux store… when this account is the active account, anyway. We could have the notifications code go read it from the persisted state.
    • After Store server data for multiple accounts at once #5005, we'll have this data for all accounts, not just the active account. But even before then, if we had the plumbing for this then we could use this data when the notification does happen to be for the active account (so in particular for everyone who only has one account they use), and otherwise fall back to the current text.
    • This is probably the ideal solution, because this shape of solution avoids denormalizing more and more data into the notification payloads. But it'll require some infrastructural work: to date we have not had any code on the Kotlin / native-Android side reading the state we persist from Redux.
    • Compared with the similar option for Use org name rather than URL for heading of Android notif group #5115, this has another obstacle which is that the place this information lives is the state.users subtree, which is all stored as one JSON blob in one SQLite row, and may be several megabytes. (The information needed for Use org name rather than URL for heading of Android notif group #5115 would live in state.accounts, which should rarely reach a kilobyte.)
      • That means parsing several MB of JSON from the Kotlin side. With a good fast parser on a desktop machine, that might take 5-10ms (e.g. these results for orjson)… but a less-fast parser can take much longer (40ms for some libraries in those same benchmark results, and even those were the new generation of highly-optimized JSON parsers for Python a decade ago), plus the device may not be as fast as a laptop. This is probably fine but calls for at least some rough measurement before we ship it.
      • Alternatively, we've discussed (Store persistent data in a sound way #4841 (comment)) splitting up state.users into one row per user, a more natural style of database schema. If we do that, it will resolve this concern entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant