-
-
Notifications
You must be signed in to change notification settings - Fork 292
Adding bubbles support #5554
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
base: master
Are you sure you want to change the base?
Adding bubbles support #5554
Conversation
|
|
||
| // Use the same notification ID calculation as NotificationWorker | ||
| // Show notification with bubble | ||
| notificationManager.notify(notificationId, notification) |
Check failure
Code scanning / CodeQL
Use of implicit PendingIntents
|
thank you @arlexTech |
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.
Thank you @arlexTech ! 👍
The bubbles itself work for me, however there are some things that must be changed:
Don't show conversation list and modify back button handling
I strongly suggest to not open the conversation list when pressing back button in bubbled mode.
So the bubbled view should not show the conversation list at all but only single conversations. Otherwise i'm concerned of bugs when having multiple instance of the ConversationListActivity.
For the back button in bubbled mode, either:
- just close the bubble (This is how Telegram does it)
- or replace the back button with a the talk icon which will open the app in normal mode and the conversation list is shown (This is how signal does it and it'S also my preferred solution)
Avoid multiple instance of ChatActivity for same conversation
Also, we should make sure that there are not multiple instances of ChatActivity for the same conversation.
Right now, the PR has a bug that multiple instances are shown when "open in app" is used.
https://developer.android.com/develop/ui/views/notifications/bubbles#launching-activities
But also we should not allow to have 2 instances for the same conversation when one is opened in normal mode and one in bubble mode.
This is very important to avoid bugs in the chat repository (OfflineFirstChatRepository)!
Signal solved it this way: Whenever there is a bubble for a chat and the chat is also opened in the normal mode, the bubble closes.
We should solve it the same way and try to use only one instance of ChatActivity for a conversation.
app/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.kt
Outdated
Show resolved
Hide resolved
| // Use a consistent ID based on the room token to avoid duplicate bubbles | ||
| val systemNotificationId: Int = | ||
| activeStatusBarNotification?.id ?: calculateCRC32(System.currentTimeMillis().toString()).toInt() | ||
| activeStatusBarNotification?.id ?: NotificationUtils.calculateCRC32(pushMessage.id!!).toInt() |
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.
i am not 100% if using the room token could cause unwanted side effects. will have to think about and test when i have better internet connection ;)
|
@rapterjet2004 @sowjanyakch please also do a deep review. E.g. specially check if former notification handling is affected by the changes. |
app/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.kt
Outdated
Show resolved
Hide resolved
|
I'll work on it this week (don't mind the review request i miss clicked) |
Sure, thanks. Let us know if there are any questions or help is needed! |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
For the back button in bubbled mode I replaced it by the talk icon in bubble mode which open the app in normal mode on the conversation list
fixed
should be fixed, I couldn't open multiple anymore
done
because it was preventing bubbles to show up but it should be fixed now I wanted the handleOnBackPressed function to minimize the bubble but I tried for hours and never managed to make it work correctly, if anyone has an idea of how to do it, I'm open to try anything at this point Tell me if i can do anything more to merge it |
I didn't see this part, I wonder how Messenger deal with that because it can have the bubble and the chat open at the same time |
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 to implement the suggestions @arlexTech 👍 We appreciate your efforts!
I commented on some things that must be changed.
There were changes on master in NotificationWorker that conflict with your branch. Could you resolve it?
Please also see the codacy warnings at
https://app.codacy.com/gh/nextcloud/talk-android/pull-requests/5554/issues
and sign your commits like described in
https://github.com/nextcloud/talk-android/pull/5554/checks?check_run_id=57331453438
If you need any help or nextcloud employees should take over, just let us know.
app/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.kt
Outdated
Show resolved
Hide resolved
sowjanyakch
left a comment
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.
Thank you for PR @arlexTech
The inbuilt emoji picker does not appear when tapping the emoji icon in the bubble notification chat view.
|
Thank you for your feedback, i try to fix all of this and if I'm hard stuck I'll ask for help |
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
…when enabled Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
Signed-off-by: alex <[email protected]>
b33937a to
d11c761
Compare
|
I think I fixed everything you said but the emoji picker, for the emoji picker I tried but didn't managed to make it work, it simply never shows up |
|
@arlexTech i'm currently taking a look and modify some things. Is it okay for you if i push to the branch? |
Yes no problem |
Signed-off-by: Marcel Hibbe <[email protected]>
instead to use KEY_NOTIFICATION_RESTRICT_DELETION, always check if notification is actually bubbled in cancelNotification Without this commit, bubbled notifications would remain open when the chat was opened in normal mode. With this commit, bubbled notifications will be removed when opening the chat in normal mode. Signed-off-by: Marcel Hibbe <[email protected]>
…" is chosen for a conversation before this commit, only systemAllowsAllConversations was checked. This was not enough but also system wide enabling of bubbles must be checked + minor refactoring Signed-off-by: Marcel Hibbe <[email protected]>
3e25a68 to
0341869
Compare
|
I took a closer look and changed/fixed some things. Please see the commit messages and let me know if you have questions/comments. Could you please sync the fork and rebase on master to have the newest upstream changes? I think we are getting there to merge it soon.
Did not have a look but it's not a blocker to merge. I would like to replace the emojipicker (#2735), maybe this would even fix it. @sowjanyakch and @rapterjet2004 could you please review/test this PR a lot? At best also with different android versions. With bubbles enabled and also disabled (so, normal notifications). @arlexTech if you are willing to (so not a blocker for merging), it would be great if you could move some bubbles logic out of the ChatActivity, e.g. to NotificationsUtils. For example createConversationBubble.. |
|
Regarding:
@arlexTech let me know if you would like to have rights to the original repo. So you could create branches inside it if you would like to further contribute in the future and reviewing gets easier for nc devs.. |
| activeStatusBarNotification?.id ?: calculateCRC32(System.currentTimeMillis().toString()).toInt() | ||
|
|
||
| if (TYPE_CHAT == pushMessage.type || TYPE_REMINDER == pushMessage.type) { | ||
| // Use a consistent ID based on the room token to avoid duplicate bubbles |
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.
- please remove the last comment line as it's outdated
Issue: #3384
🖼️ Screenshots
VID_20251114124529.mp4
Record_2025-11-14-12-44-40.mp4
🚧 TODO
🏁 Checklist
/backport to stable-xx.x