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

Make the notification a data notification #1

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ligi
Copy link
Member

ligi commented Nov 23, 2018

You have much more control over a data notification. E.g. this way you can localize the text for the user. Also this way you do not need to go through the launch activity but can directly launch the correct action.
It was this way before - but with the unfortunate migration to javascript it became a message notification.

Please see: https://firebase.google.com/docs/cloud-messaging/concept-options

Make the notification a data notification
You have much more control over a data notification. E.g. this way you can localize the text for the user. Also this way you do not need to go through the launch activity but can directly launch the correct action.
It was this way before - but with the unfortunate migration to javascript it became a message notification.

@ligi ligi requested a review from pedrouid Nov 23, 2018

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Nov 23, 2018

@pedrouid perhaps this was also the problem you had with the kotlin version? These messages have to be handled differently and remembering the problems you described this could explain it. So this means we can go back to kotlin here then?-)

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Nov 23, 2018

@pedrouid perhaps this was also the problem you had with the kotlin version? These messages have to be handled differently and remembering the problems you described this could explain it. So this means we can go back to kotlin here then?-)

Ahah maybe that was the issue. I need to detail the tech spec more thoroughly for client-side as well. Most specification revolves around the Bridge server which is not as relevant

Happy to move back to Kotlin!

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Nov 24, 2018

Wait a second, I just realised this was a PR and not an issue.

On this PR, you are removing the notification body. We need that to tell the user what this notification is about if the app shuts down. Otherwise we could've down simply through websockets.

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Nov 24, 2018

No - removing the notification body makes it to a data notification. Please see: https://firebase.google.com/docs/cloud-messaging/concept-options

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Nov 26, 2018

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Nov 26, 2018

Hey, I've followed up on the discourse about the dappName, being included in the push notification. This actually has been part of the design from the beginning. We can make it optional however.

Also about the data notifications, they assume the client app is actively running on the background which might not always be the case. Making it not possible for the operating system to push these notifications to re-open the app.

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Nov 26, 2018

Hey, I've followed up on the discourse about the dappName, being included in the push notification. This actually has been part of the design from the beginning. We can make it optional however.

IMHO it should be completely removed

Also about the data notifications, they assume the client app is actively running on the background which might not always be the case. Making it not possible for the operating system to push these notifications to re-open the app.

no - this is not true - please read the documentation I linked

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Dec 4, 2018

@pedrouid can we merge this?

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Dec 11, 2018

@pedrouid - weekly ping about this ;-)

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Dec 11, 2018

No worries, I will give it a try right now with the walletconnect-developer-app

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Dec 11, 2018

Confirmed! This was the same issue that I got from the Kotlin codebase before. However we need to figure out why does my app require a notification object.

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Dec 11, 2018

Also about the data notifications, they assume the client app is actively running on the background which might not always be the case. Making it not possible for the operating system to push these notifications to re-open the app.

no - this is not true - please read the documentation I linked

I've read the documentation again and I still get the same conclusion:
Notification Message - FCM automatically displays the message to end-user devices on behalf of the client app.
Data Message - Client app is responsible for processing data messages. Data messages have only custom key-value pairs.

The misunderstanding is solely because of the way the push notifications are handled in our own apps. I want the user receive notifications even in when the app is not open or even running on the background. This is a requirement for the Data Messages which completely removes the point of why we created the Push Server in the first place. We are implementing long-polling and websockets soon enough that would replace the whole purpose of Data Messages. However Notification Messages require the Push Server to exist and the idea of WalletConnect is to exist in the absence of the Push Server but provide this extra layer for improved UX

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Dec 11, 2018

I've read the documentation again and I still get the same conclusion:
Notification Message - FCM automatically displays the message to end-user devices on behalf of the client app.
Data Message - Client app is responsible for processing data messages. Data messages have only custom key-value pairs.

correct - and we want "Data Message" as you have more control over them (e.g. you can display them in the language of the user, specify the correct notification-channel, ...)

The misunderstanding is solely because of the way the push notifications are handled in our own apps. I want the user receive notifications even in when the app is not open or even running on the background. This is a requirement for the Data Messages which completely removes the point of why we created the Push Server in the first place. We are implementing long-polling and websockets soon enough that would replace the whole purpose of Data Messages. However Notification Messages require the Push Server to exist and the idea of WalletConnect is to exist in the absence of the Push Server but provide this extra layer for improved UX

I just tried the following to confirm that I was reading the documentation correctly: I restarted the phone and received a Data Notification afterwards without starting the app before this. Worked perfectly - and this app is not using any ACTION_BOOT_COMPLETED or things like this.

So can we merge this now?

@pedrouid

This comment has been minimized.

Copy link
Member

pedrouid commented Dec 12, 2018

I'm sorry Ligi but there is still a misunderstanding. For iPhone this won't work. I requires Notification Messages to display to the user when the app is on the background or closed. Merging this PR would just make it work for Android.

I will push the development of WalletConnect long-polling / websockets faster and then we can re-visit this issue. There won't be any use for Data Messages then and we can leave the Notification Messages for only Wallet providers that want it to add to their apps.

@ligi

This comment has been minimized.

Copy link
Member Author

ligi commented Dec 14, 2018

There won't be any use for Data Messages

I see a lot of use for data messages ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.