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.

Sign up for GitHub

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

Jump to bottom

Click on notification should expand it #5242

Open
alya opened this issue Feb 17, 2022 · 9 comments · May be fixed by #5472
Open

Click on notification should expand it #5242

alya opened this issue Feb 17, 2022 · 9 comments · May be fixed by #5472
Assignees
@rachelhyman
Labels
a-Android a-notifications help wanted P1 high-priority

Comments

@alya
Copy link
Collaborator

alya commented Feb 17, 2022

On Android, clicking on a top-level notification currently does not do anything. The user needs to click on the caret menu to expand and show the individual topics, which can then be clicked to open in the app.

To make it easier for the user to open up the notification, clicking on a top-level notification should make it expand to show individual topics.

CZO discussion
@Tusharkhati
Copy link

Hello. can I work on it? can you please guide me a bit?
Thank you.

@alya
Copy link
Collaborator Author

alya commented Feb 22, 2022

@Tusharkhati please refer to the Zulip contributor guide and linked resources for some advice no how to ask for help. As mentioned in the guide:

It’s very hard to answer a general question like, “How do I do this issue?”

Thanks!

@omkarg01
Copy link

@gnprice would I get help on how to go about solving this issue? I had checked NotificationListener.js, notifOpen.js, notifToken.js files getting no idea where to look for.

@gnprice
Copy link
Member

gnprice commented Mar 24, 2022

This part of the UI is one of the small number of places controlled by our Android-native code. The relevant source file is:
android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt

For background on the Android-native code, see:
https://github.com/zulip/zulip-mobile/blob/main/docs/architecture/android.md

@rachelhyman
Copy link

rachelhyman commented Aug 3, 2022 •
edited
Loading

@zulipbot claim
Ope, I see we don't use zulipbot in this repo. I'd like to start working on this!

@chrisbobbe
Copy link
Contributor

Ah, yeah, perhaps we'll get back to doing that (or something like it) someday. 🙂 I've just assigned the issue to you; please let me know if you have any questions!

@rachelhyman
Copy link

@chrisbobbe Is there a good way to trigger several notifications (apart from just waiting for them to roll in organically)?

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Aug 5, 2022 •
edited
Loading

Hmm, good question. We've thought about adding a feature for requesting a "test notification" from the server when you press a button in the mobile app, so users can confirm on-demand that notifications are working. So far, we haven't picked up that work, though.

My go-to solution so far has been to send PMs between my main account on CZO (chat.zulip.org) and a test account I created there. The "test here" stream or a new private stream would also work as an alternative to PMs, as long as notifications are enabled for the stream.

rachelhyman added a commit to rachelhyman/zulip-mobile that referenced this issue Aug 7, 2022
@rachelhyman
notifications: Remove Android summary notification.
fe7d113 
Removes the summary notification from the group in
NotificationManager.kt. This allows the user to tap on the body of the
notification to open the app, which was not possible with the summary
notification (only the chevron would expand the
notification, which would then allow the user to interact with the
individual notifications below it.

Please note that removing the summary notification breaks proper
grouping: multiple private messages from the same user will correctly
be grouped together, but stream notifications appear in a separate
notification. This was tested manually.

Fixes part of zulip#5242.
@rachelhyman rachelhyman mentioned this issue Aug 7, 2022
Closed
rachelhyman added a commit to rachelhyman/zulip-mobile that referenced this issue Aug 10, 2022
@rachelhyman
notifications: Remove Android summary notification.
5a951a2 
Removes the summary notification from the notification group in
NotificationManager.kt, and removes logic around removing the
summary notification when Zulip messages are read.

This change allows the user to tap on the body of the
notification to open the app, which was not possible with the summary
notification (only the chevron would expand the
notification, which would then allow the user to interact with the
individual notifications below it.

Please note that removing the summary notification breaks proper
grouping: multiple private messages from the same user will correctly
be grouped together, but notifications for different stream
conversations appear in separate notifications.
This was tested manually.

Fixes part of zulip#5242.
rachelhyman added a commit to rachelhyman/zulip-mobile that referenced this issue Aug 10, 2022
@rachelhyman
notifications: Remove Android summary notification.
3e1d54d 
Removes the summary notification from the notification group in
NotificationManager.kt, and removes logic around removing the
summary notification when Zulip messages are read.

This change allows the user to tap on the body of the
notification to open the app, which was not possible with the summary
notification (only the chevron would expand the
notification, which would then allow the user to interact with the
individual notifications below it.

Please note that removing the summary notification breaks proper
grouping: multiple private messages from the same user will correctly
be grouped together, but notifications for different stream
conversations appear separately.
This was tested manually.

Fixes part of zulip#5242.
@gnprice
Copy link
Member

gnprice commented Aug 18, 2022

Updating this with what we've learned from @rachelhyman's investigation (in chat thread and at #5459):

  • It doesn't seem to be possible to get the behavior we originally hoped for: that tapping the summary notification -- outside the expando chevron in the corner -- would expand the notification group.
  • A pretty reasonable behavior we could do instead would be that doing so would open the app.
    • One nuance to take care of in this behavior: we'd want to switch to the appropriate account (i.e. the one the notification was for), if that isn't the account that's already active. That's something we currently do when opening an individual notification.

That latter behavior is what one sees with Gmail, or with the Messages app that on a Pixel device handles SMS by default.

rachelhyman added a commit to rachelhyman/zulip-mobile that referenced this issue Aug 22, 2022
@rachelhyman
notifications: Open to unreads from summary notification
273fae8 
Adds an intent to the summary notification. When the summary
notification is clicked in the collapsed state, the app will open and
reset itself to the unreads screen. It will also switch to the
appropriate account if needed.

Because the intent flags (Intent.FLAG_ACTIVITY_NEW_TASK
and Intent.FLAG_ACTIVITY_CLEAR_TOP) apply to both the normal
notification and the summary one (and are prefaced with explanatory
comments), I moved them to a variable before where both notifications
are created.

This was manually tested for the following cases, with both the app
already being on the correct account and being on a different account:
- App open in background on non-unreads screen
- App open in background on unreads screen
- App not open in background

I also made sure that tapping on a notification in the expanded state
still routes the user to the correct screen.

Fixes zulip#5242.
@rachelhyman rachelhyman linked a pull request Aug 22, 2022 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rachelhyman rachelhyman

Labels
a-Android a-notifications help wanted P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

notifications: Open to unreads from summary notification. rachelhyman/zulip-mobile
6 participants
@gnprice @alya @rachelhyman @chrisbobbe @omkarg01 @Tusharkhati