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

Separate Android notification data by account #4633

Closed
gnprice opened this issue Apr 9, 2021 · 1 comment
Closed

Separate Android notification data by account #4633

gnprice opened this issue Apr 9, 2021 · 1 comment

Comments

@gnprice
Copy link
Member

gnprice commented Apr 9, 2021

Currently we maintain the following data structure in our Android-native code that handles notifications:

/**
 * The Zulip messages we're showing as a notification, grouped by conversation.
 *
 * Each key identifies a conversation; see [buildKeyString].
 *
 * Each value is the messages in the conversation, in the order we
 * received them.
 *
 * When we start showing a separate notification for each  [Identity],
 * this type will represent the messages for just one [Identity].
 * See also [ConversationMap].
 */
open class ByConversationMap : LinkedHashMap<String, MutableList<MessageFcmMessage>>()

/**
 * All Zulip messages we're showing in notifications.
 *
 * Currently an alias of [ByConversationMap].  When we start showing
 * a separate notification for each [Identity], this type will become
 * a collection of one [ByConversationMap] per [Identity].
 */
class ConversationMap : ByConversationMap()

As those comments foreshadow, we should separate out this data by which account/identity the user received it for. As is, because we don't, we end up showing conversations in different accounts jumbled together in a single summarized notification.

This will then be a building block for #2691.

We might also first use the separated data structure to start showing separate notifications per account within our current style of notification; that'll be worthwhile just if the work consists mostly of refactors we need anyway for #2691.

@gnprice gnprice self-assigned this Apr 9, 2021
AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue Jul 1, 2021
`ByConversationMap` was originally intended to store conversations
based on `Identity` as key, so that we would be able to group
conversations by realmId easily.

Here we are removing it, and introducing a new data class in the
following commit that will store realmId and messages along with
notificationId of that particular conversation.

A reason to do this is that structurally we are identifying unique
notification based on its title not by its `Identity`. So its easy
to create a direct mapping of UI notification with the
`ConversationMap` data structure this way. Extra information about
the conversation (eg: realmId) will be included in the value of
ConversationMap (see the next commit).

Skips: zulip#4633
AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue Aug 4, 2021
`ByConversationMap` was originally intended to store conversations
based on `Identity` as key, so that we would be able to group
conversations by realmId easily.

Here we are removing it, in favor of using
`NotificationManager#getActiveNotifications` in the following
commits.

Notifications will persist automatically if we are to do this, and
we don't have to be concerned with handling logic for storing
past notifications, Android SDK will abstract it away for us.

This will eventually be followed by removal of`ConversationMap`,
and other associated code.

Skips: zulip#4633
AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue Aug 4, 2021
`ByConversationMap` was originally intended to store conversations
based on `Identity` as key, so that we would be able to group
conversations by realmId easily.

Here we are removing it, in favor of using
`NotificationManager#getActiveNotifications` in the following
commits.

Notifications will persist automatically if we are to do this, and
we don't have to be concerned with handling logic for storing
past notifications, Android SDK will abstract it away for us.

This will eventually be followed by removal of`ConversationMap`,
and other associated code.

Skips: zulip#4633
AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue Aug 5, 2021
`ByConversationMap` was originally intended to store conversations
based on `Identity` as key, so that we would be able to group
conversations by realmId easily.

Here we are removing it, in favor of using
`NotificationManager#getActiveNotifications` in the following
commits.

Notifications will persist automatically if we are to do this, and
we don't have to be concerned with handling logic for storing
past notifications, Android SDK will abstract it away for us.

This will eventually be followed by removal of`ConversationMap`,
and other associated code.

Skips: zulip#4633
gnprice pushed a commit that referenced this issue Aug 10, 2021
`ByConversationMap` was originally intended to store conversations
based on `Identity` as key, so that we would be able to group
conversations by realmId easily.

Here we are removing it, in favor of using
`NotificationManager#getActiveNotifications` in the following
commits.

Notifications will persist automatically if we are to do this, and
we don't have to be concerned with handling logic for storing
past notifications, Android SDK will abstract it away for us.

This will eventually be followed by removal of`ConversationMap`,
and other associated code.

Skips: #4633
@gnprice
Copy link
Member Author

gnprice commented Nov 8, 2021

This was accomplished as part of #4842. We actually stopped keeping our own data structure here, leaving it to the OS, and we arranged there for the data to be appropriately separate.

@gnprice gnprice closed this as completed Nov 8, 2021
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