feat(android): Remove all gms usages and use guava instead #510
Conversation
|
|
meypod
changed the title
meypod
changed the title
meypod
force-pushed
the
migrate_to_guava
branch
from
e54cb67
to
c5083e3
Compare
android/src/main/java/app/notifee/core/utility/ExtendedListenableFuture.java
Show resolved
Hide resolved
android/src/main/java/app/notifee/core/utility/ExtendedListenableFuture.java
Show resolved
Hide resolved
|
wow! pretty quick from idea to a PR, very cool. This isn't the sort of change I can (or should or would) ingest unilaterally so I need to find some time to sync up with @helenaford on it, but the general idea of "replacing async programming library with something that works the same but is 'free software' compatible is something I support. And the churn in the diff isn't too crazy, though it needs review of course left just a couple specific comments which you've likely already seen... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #510 +/- ##
=======================================
Coverage 77.25% 77.25%
=======================================
Files 34 34
Lines 1701 1701
Branches 566 566
=======================================
Hits 1314 1314
Misses 337 337
Partials 50 50 |
|
ah nice, i like this idea of having everything fully open source :) happy to release this as an alpha from your p/r if that helps? but won't be able to fully test this before merging to master for a couple of weeks as I'm going away. |
|
It would be very helpful, edit: I probably will update this branch tomorrow around the same time, are you available till then ? |
|
@meypod nope unfortunately not... but there's a way to use patch-package i believe. I'm going on vacation for two weeks from tomorrow |
|
I'm not sure this one works well with patch-package, however I know from previous collaboration on the idea that you a build script that pulls notifee core from git and references it from source when building the react-native module - no reason you can't switch both of those to point at your local branch. More than anything, I think it was important just to see what Helena's initial reaction was - that was important for me anyway. Seeing it start with "ah nice" makes me think this will merge over time so it shouldn't be too dangerous to go off your branches for a bit while we work through it |
|
Have a good vacation ! |
|
Thank you. I had a great vacation. I am always open to moving to open source libraries, I'm not going to lie though that this change does make me feel a bit nervous as it's a big change 😅 Just might need some more testing than smaller p/rs |
|
Nice |
There was a problem hiding this comment.
This looks good to me, and honestly, there is nothing better for making sure something works than an actual deployed app with users (and you have 8 stars on github - no joke, just having the first few people watch/follow/use an app is where almost all the issues are discovered...)
Nice one
|
I'm getting few reports from my app users that it's not getting triggered reliably despite disabling battery optimizations Since it's started happening after this update, I'd say it's possible that I have broken sth that I'm not aware The app been triggering as expected for myself so I have my doubts if it's the update causing this problem alone |
|
Is it possible that it has caused a memory leak and os terminating it due to leak after sometime on low end devices? |
|
@meypod anything is possible? Using the Android Studio profiler can say |
|
well, profiler did not show any sign of leak, memory usage was consistent edit: it seems my app was crashing sometimes due to service trying to unbind under some condition that would throw an exception, so that maybe was why app was not working properly sometimes |
|
After hours of debugging I figured there was indeed some issue with the PR I hope it was the only fixes needed |
|
@mikehardy any idea when and if this will be merged? |
ignore result of NotifeeAlarmManager::cancelAllNotifications call
also cleaned up some code
|
I'm still paying attention here for what it's worth, and I've got some time budgeted for notifee maintenance now and for the next few weeks. Hoping to square this away + get it merged. You've been extraordinarily patient |
|
Ahaha, thank you 😸 |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
Hi @mikehardy , is this still planned ? |
|
Yes! |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
🤷🏻♂️ 😞 |
|
No stale bot, no. This PR is gold, it will go in |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
not going to let it close without merging |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
bad bot 😠 |
|
@meypod apologies and no rush but if you get a chance would you mind fixing the conflicts/merging the upstream changes again? i'd love to switch to this branch for my app (until it gets merged) but i'd rather not lose any upstream fixes |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
not stale |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
not stale, although it'd be good to get an idea of when y'all will be able to review this/get it closer to merging? i get the impression y'all are busy with the Firebase libraries though, so no pressure/judgement :3 |
|
Would be nice before it hits the 2 year mark 🤔 |
hi @mikehardy , sorry about mentioning, please take a look at this PR if you can merge this
this is related to #493
I tried running tests and all passed
the code may look inconsistent in indentations, this is intentional, since there were many changes, I want to keep the noise down
you can format the code later using the formatter you use to fix the formatting issues.
thanks !