changes to enable unified push notifications in generic build. Signed… #4169
Conversation
app/build.gradle
Outdated
| implementation 'com.github.nextcloud-deps:android-talk-webrtc:121.6167.0' | ||
|
|
||
| // unified push library for generic flavour | ||
| genericImplementation 'org.codeberg.UnifiedPush:android-connector:2.4.0' |
There was a problem hiding this comment.
| genericImplementation 'org.codeberg.UnifiedPush:android-connector:2.4.0' | |
| genericImplementation 'org.unifiedpush.android:connector:2.5.0' |
|
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.) |
|
@gavine99 you may want checkout the DCO error message, if you click on "details" it will explain what you need to do |
|
@Fuseteam i'm not sure what i'm supposed to do to fix ?? dco results say 8 commits not signed off my 4 commit messages all have sign-off string; the other 4 commits listed dco results are nextcloud android bot and i can't sign them off;
|
|
@gavine99 then i think you need to drop the commits that are not yours, i assume those should be in the main tree already |
|
@gavine99 perhaps try rebasing your branch and only picking your own commits |
gavine99
force-pushed
the
unified_push_nextcloud_notifications
branch
from
3a39598 to
b190409
Compare
|
ayyy nice DCO looks good now :D |
|
looks like remaining issues are listed here: https://app.codacy.com/gh/nextcloud/talk-android/pull-requests/4169/issues |
gavine99
force-pushed
the
unified_push_nextcloud_notifications
branch
3 times, most recently
from
b8f144c to
2d54efb
Compare
|
nice i see no failing checks anymore, i suppose now the other 3 PRs likely also need the same treatment, 2 of them have review comments from a member of nextcloud that may need to be addressed p.s other than for the rebase(s), i believe you shouldn't force push, the maintainer can always squash later. But maybe that's just me |
|
As I said on the NextPush PR, a better approach is to add support for webpush, so we can have a proper UnifiedPush support on the Nextcloud applications: it won't depend on a pre-defined distributor (which goes against the whole UnifiedPush idea) I will work on adding this support in not so long. Your different PR on the clients may be used as a base for it |
@p1gp1g maybe i don't understand enough but this pr uses the generic unified push connector so it doesn't depend on a predefined distributor (ie the distributor can use webpush or any other push tech). this pr also uses the same message format as the existing fcm push messages. it's all very generic and doesn't commit anyone to using anything. |
@Fuseteam i rebased and squashed because all my commits had the wrong signed-off-by info |
I didn't look at the details, this is what I understood based on your requirements for nextpush. Does it work with another distributor right now ? |
|
there is nothing to make it rely on nextpush. the original pr description does say; ... this pr is related to, but not dependent on,... i will edit the pr description to make it more clear. if one has multiple distributors installed (i have nextpush and ntfy) it even allows you to choose which distributor to use using the connector-ui library dialog.
|
|
Well, that's great then. It would be nice to edit the different PR to know what is required, what is not, and what has changed! If you want, I'm OK to review your different client-side PRs. From uppush PR:
It isn't true anymore then ? Or it has never been a requirement ? I will still work to add webpush on notifications app, it is needed for different reasons. And it can then be used by the different clients then. Do you use the same trick than neon at this moment ? It adds a hash |
it was never a hard requirement. that uppush pr can be abandoned because it is no longer required since i have developed the stand-alone nextcloud server app that bridges notifications and uppush. i have been using it for months and it works great. there are no dependencies for my nextcloud talk and nextcloud app unified push connector pull requests. they are totally independent.
that sounds good. do you intend webpush to be used as transport for unified push - ie webpush used between server and unified push distributor? or totally separate from unified push?
i don't know what neon is, what the hard-coded path is, and what nextcloud proxy you mean :) but i can say i didn't do any tricks or work-arounds. i just added the unified push libraries and plumbed them into the existing fcm push receiver code with the least amount of changes to existing code as possible. |
The specs have been clarified to explicitly define webpush as the server to server protocol. It generally work even if it misses some parts. Can you reach me on matrix (from the UnifiedPush room), so we can clarify the different PR ? I think I wasn't the only one confused with what the different PRs do. And I think it is necessary if we want these PR to be merged |
if understand correctly does this mean that the nextcloud notifications app will talk to ntfy, uppush or ubports push over webpush? To me it sounds like this pr and nextcloud/android#13516 simply enable the android applications to use an unified push distributor that is installed on the device, while adding webpush to the notifications app will able to supercede nextcloud/server#47763, nextcloud/notifications#2027 and the bridge app |
It means UnifiedPush servers are webpush servers
Yeah, there is still one brick that will be changed: getting the VAPID key, the API to send new endpoint & webpush keys
This is the most confusing part because @gavine99 said these PR aren't necessary anymore. But "webpush to the notifications app will able to supercede" was exactly what I had in mind |
I believe this refers to the this PR: https://codeberg.org/NextPush/uppush/pulls/17#issuecomment-2282854 as the server app seems to say
well that and i think we may be confusing nextpush the distributor and uppush the server app in this discussion (i almost did too) So i think @gavine99 meant to say the nextpush PR is no longer needed |
|
then again that nextpush pr is for the uppush the server app— is there is even a nextpush pr for the distributor? looks like i'm still confusing the two 😂 |
|
i'll try to clear things up with the pushes; mobile apps uppush server app nextcloud server and the nextcloud server notifications app i hope this is helpful. |
there is a pr for the distributor, but it's completely separate bug fix and has no relationship to nextcloud or uppush |
for me, i am not a fan of this idea. i think unified push should be transport agnostic. a reason why is that, in the future when i had time available, i wanted to develop a unified push distributor based on mqtt back-end so that apps can 'push messages back.' (and other benefits that mqtt provides). yes, i realise this would be a non-standard extension of unified push but it would be unified push compliant for messages from server to client so ok for 100% unified push compliance. it would just do extra stuff. (maybe name it "unified pushback" ?? :) ) a nice example of what is good about apps being able to 'push messages back' would be that sip protocol could be 100% performed over this. |
Note @p1gp1g said "server to server protocol" So it is distributor independent. it just how web services talk to push servers. So that more services can make use of webpush. I can imagine that webpush may also make it possible to have pwa push notifications 🤔 |
sounds great |
|
@gavine99 I've sent you a message on matrix |
Signed-off-by: gavine99 <[email protected]>
gavine99
force-pushed
the
unified_push_nextcloud_notifications
branch
from
2d54efb to
875663c
Compare
changes to allow any UnifiedPush distributor to deliver push messages in generic/f-droid build.
added a setting item to reset push notification registrations in case of issues with push (in case UnifiedPush distributor gets out of sync). this same setting item can also be used to reset firebase push in case of issues with it too.
modified settings->diagnosis to cater for both unified push and fcm push messaging.
a similar pr has been submitted for the nextcloud android client app - nextcloud/android#13516