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

UnifiedPush #6228

Merged
merged 65 commits into from
Jun 16, 2022
Merged

UnifiedPush #6228

merged 65 commits into from
Jun 16, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Jun 2, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

This PR includes the content of #3448 and some rework.

Other changes:

  • Documentation: see file ./docs/unifiedpush.md
  • BackgroundSyncStarter and FcmHelper are now injected 634acbc
  • New Config object 03f05a3

Known bugs:

  • Sign in: this notification does not change the content when the init sync is over. This is not a regression, just check on a recent F-Droid build. So this will not be fixed in the scope of this PR.:

image

What can be done later: add a way to explain to the users how to install an external distributor.

Motivation and context

Closes #2743

It should highly reduce the battery usage of F-Droid version.

Screenshots / GIFs

New VectorFeature flag

image

New setting in notification (F-Droid)

image

The list of available distributors will be displayed here:

image

When Ntfy is selected, we got this notification:

image

And in the Ntfy app:

image

New setting in notification (GPlay)

image

image

Tests

Refer to #3448 (comment)

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@bmarty bmarty changed the title Feature/bma/unified push2 UnifiedPush - WIP Jun 2, 2022
@github-actions
Copy link

github-actions bot commented Jun 2, 2022

Unit Test Results

146 files  ±0  146 suites  ±0   2m 49s ⏱️ +42s
236 tests ±0  236 ✔️ ±0  0 💤 ±0  0 ±0 
788 runs  ±0  788 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit e4b3e07. ± Comparison against base commit e18146a.

♻️ This comment has been updated with latest results.

@genofire
Copy link

genofire commented Jun 2, 2022

This PR fixed #2743

Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small remarks. Didn't test but good work!

@@ -348,6 +348,7 @@ dependencies {
implementation project(":library:multipicker")
implementation 'androidx.multidex:multidex:2.0.1'

implementation libs.jetbrains.kotlinReflect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, coming from the original PR, removed.

* [4] https://github.com/p1gp1g/sygnal/blob/unifiedpush/sygnal/upfcmpushkin.py (Not tested for a while)
*/
fun parseData(message: String, firebaseFormat: Boolean): PushData? {
val moshi = MatrixJsonParser.getMoshi()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inject?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope of this PR, I think

)
}

private fun gRegister(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why gRegister?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's from the original PR. Will rename some fun here, to follow our naming convention.

@@ -113,37 +125,59 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
* when the InstanceID token is initially generated, so this is where
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comments ^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment deleted.

private val vectorFeatures: VectorFeatures,
private val fcmHelper: FcmHelper,
) {
private val up = UnifiedPush
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be injected too by adding a provides in a module

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an alias to UnifiedPush object. I will remove it, it does not bring anything.

@bmarty bmarty requested a review from ganfra June 16, 2022 11:54
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

1.9% 1.9% Coverage
0.0% 0.0% Duplication

@bmarty bmarty merged commit 16ca265 into develop Jun 16, 2022
@bmarty bmarty deleted the feature/bma/unifiedPush2 branch June 16, 2022 14:09
@mcg-matrix
Copy link

It should highly reduce the battery usage of F-Droid version.

Thank you for pursuing that goal! Element-FDroid is using way too much battery when idling...

However, the use of UnifiedPush will only reduce the total amount of energy spent if the use of energy by the UnifiedPush Distributor (Gotify-UP, ntfy, ...) is lower than the avoided use of energy by the applications using UnifiedPush.

In a case where Element is the only battery-expensive always-online app, the use of UnifiedPush currently does not reduce the total amount of energy spent (as far as I can tell); both Gotify-UP and ntfy actually use more energy than Element-FDroid in sync mode "real time".

Here's a screenshot showing the battery usage of

  • Element-FDroid in sync mode "real time" (with 179 seconds timeout and 0 seconds delay, talking to a homeserver that keeps TCP connections alive and allows reuse of HTTP connections),
  • ntfy (using its newer WebSockets connection protocol) serving SchildiChat.

Both Matrix clients are configured for the same account, so Element will probably have had to process more info via /sync than SchildiChat via UnifiedPush.

Screenshot_20220617-100554_Settings_30p

(Yes, Conversations also is always-online.)

@p1gp1g
Copy link
Contributor

p1gp1g commented Jun 17, 2022

Just a few things:

  • Even if element is the only app using the distributor, it can reduce battery drain because distributors are optimize to be battery efficient. For instance NextPush used 2% for 18h for me atm.
  • If you receive a lot of notifications, it increases the battery consumption. It is very probable people receiving less notifications will see a bigger difference.
  • Also, this comparison can be skewed : for what I've seen, if a process wake the phone, others won't have to do it. The first one will ends with a higher consumption in the stats. It is better to run one then the other to compare.
  • Finally, everything depends a lot on the vendor battery optimization. You may want to look at https://dontkillmyapp.com/

PS : have you disable battery optimization for ntfy ?

@genofire
Copy link

genofire commented Jun 17, 2022

sadly i have some experience with ntfy.sh on websocket, to save battery try json-stream over http.

I do not know, how ntfy.sh has implemented websocket so battery hungry.

@mcg-matrix
Copy link

  • [...] For instance NextPush used 2% for 18h for me atm.

Interesting. I wish there was a low-energy UnifiedPush Distributor app that I could recommend to my non-technical users, i.e. that can be run without them having to maintain a server-side thing.

  • Also, this comparison can be skewed : for what I've seen, if a process wake the phone, others won't have to do it. The first one will ends with a higher consumption in the stats. It is better to run one then the other to compare.

Well, the numbers look completely reasonable. Typical energy consumption of that reference device (running Element, not running ntfy, mostly idling) is ~1.5% of battery capacity per hour (you'll have to believe me that it's been like that for many months). So after 11 hours since charging, I'd expect ~84% of battery capacity to be left. As the screenshot shows, the device did only have 76% left and said that ntfy had used 8% – if that's a lucky coincidence, I should go and buy lottery tickets. ;-)

PS : have you disable battery optimization for ntfy ?

Yes, ntfy prompted me to "fix" that setting (if I remember the wording correctly), warning about potential issues with notifications. I'm not leaning towards ignoring ntfy's recommendation, as (A) I'd expect my users to also follow the recommendation, and (B) nothing is worse for them than missing notifications, not even enormous battery usage...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use UnifiedPush for push notifications
7 participants