Notifications #10294
Notifications #10294
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (25)
|
99% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (96)Click to expand |
|
Issue 1 Issue 2 Video of issue: https://drive.google.com/file/d/1z4F-Eb4YXKIGCmCtxUlMPxOLV9e4LJas/view |
|
A few things I was thinking: It's probably best not to use That way we can customize notifications (only for contacts, only for some chats and not necessarily only for messages, for example we could have mentions). In terms of enabling them only for contacts, I am not sure it will be clear enough for the user. Currently there's no incentive in adding someone to contacts (it's basically a noop), so I would be more inclined to have a separate menu (settings for a chat, we might have something similar already, which will come handy when we do disappearing messages, mute chat). Another thing is that we can always show an updated list of notifications, as status-go can instruct java to display the whole list. cc @errorists |
|
|
@cammellos @Serhy ok actually I was able to reproduce in the PR build but not in the debug build |
|
@Serhy ok looks like it is good after rebase I wasn't able to reproduce. I think we could go with the current settings of receiving notifications for all 1-1 chat, and next step is to do some work on status-go for filtering what gets a notifications, add settings in status-react and notifications for other chats and part of the app. @errorists @rachelhamlin wdyt? |
|
@yenda thank you. Issue3 This one is annoying in terms of reproduction because it was appearing all the time, but gone after I restarted Android devices. I anyway report it and trying to understand meanwhile when it appears. To reproduce:
Logcat: Issue4 Very often I see PN not destroyed (marked as read) after it was tapped. Instead PN received from contacts accumulates into huge list. And when there are PN from multiple contacts it becomes messy so I ended up on tapping wrong PN and opening 1-1 chat with false impression there were no messages
Issue5 Contact names in PN expose bold markdown ( tags) 
|
|
Issue6 I'd be very thankful if we fix the next (it a bug itself and needed to fixed to allow easier implementation of autotest)
|
yenda
force-pushed
the
notifications
branch
2 times, most recently
from
2b540a9
to
920a052
Compare
|
Issue 1 as discussed could be dealt with later, and is actually a feature TM Issue 2 Issue3 I think those should be fixed now, I fixed two things:
Issue4 Issue5 These issues were caused by the legacy implementation of groups, I removed it entirely because after testing on Android 7 it does some grouping already (albeit not as good as Android 8 and newer) Issue6 I am looking into this one I thought it would be fixed along with 2/3 but I reproduced it again in the PR build |
|
Ok 6 should be fixed in the next build. Android is actually calling onHostDestroy when you press the back button, which stops the service, so I just made sure the service isn't stopped then. That means even if you kill the app the service will keep running. edit: well I happened to have fixed another bug but there is still an issue with the universal link being empty when app was killed and opened from notification (which happens with back button press) |
|
@Serhy same bug already exists it's not related to PN but to universal links, when the activity is destroyed with the back button, the universal link isn't loaded on resume. So if you open a universal link with after the same steps you get the same result. I would suggest we create an issue for it and fix it separately, as it is not cause by this PR and is a broader issue that needs to be fixed |
|
@Serhy I added a fix for universal links |
99% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (90)Click to expand |
@yenda Indeed all issues fixed, push notifications are working fine right now! However, what happens is that even when I want to kill the app on Android with enabled So |
|
@Serhy you need to logout if you want to kill the app, it's not just an app now but also a service, and killing the app doesn't kill the service, otherwise it will be killed as well when you press the back button |
|
I don't know it's OK from security point of view. |
|
It's different because with notifications you always have a notification opened that says the app is running even after you kill it. |
|
Understood that this is a necessity of serverless notifications. What happens if I log out? Are notifications turned off? Do they have to be manually turned on again? |
|
@rachelhamlin they are turned off until you login again |
|
Yes, that's true
This notification keep staying if I close Status. Video of app behavior when notifications are ON: https://drive.google.com/file/d/1fRwmkYbB0nOBqP-KtTlDxO4gHHgBwG2T/view?usp=sharing
Notifications turned off (means you close the service), but once open Status and login -> Notifications enabled again. |
|
So, I understand this might be a bit too late to pull a change like this, but I'd like to suggest changing the "Don't kill the app" message to go somewhere else, maybe inside the app itself. We have a moment, when we offer to enable notifications, to educate the user and to explain why notifications are the way they are, and the shortcomings and advantages. Doing things like this, despite it acting as a reminder, will ultimately, IMHO, be a disservice to users in general. @hesterbruikman curious to hear your thoughts. |
|
I kind of like having the |
|
@hesterbruikman I don't have an opinion yet without having used it, I'm not familiar at all with Android notifications. Once this is out in the open and I and everyone else will get their feet wet, I'll be able to give informed advice. I definitely don't mind releasing it as it is here. |
|
Thank you @andremedeiros @hesterbruikman @errorists ! |
- only show notifications when app is in background - open the user chat upon tap on notification - remove chat from notifications on tap or dismiss notification - keep the service alive on host destroy - enable local notifications
fix for universal link not opening properly when app was closed with back button, returning nil instead of the url Signed-off-by: yenda <eric@status.im>
status: ready