D17851: Add Android notification backend

[Volker Krause]

vkrause added inline comments.

INLINE COMMENTS

> vkrause wrote in notifybyandroid.cpp:125
> Possible, I'll investigate.

Addressed in D18526 .

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause, apol
Cc: broulik, apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Volker Krause]

vkrause added inline comments.

INLINE COMMENTS

> broulik wrote in notifybyandroid.cpp:79
> Can there be multiple instances of this `NotifyByAndroid`?

KNotificationManager prevents that IIUC.

> broulik wrote in notifybyandroid.cpp:125
> Does this need a `FreeLocalRef` call?

Possible, I'll investigate.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause, apol
Cc: broulik, apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Kai Uwe Broulik]

broulik added a comment.


  Pretty cool!

INLINE COMMENTS

> notifybyandroid.cpp:79
> +{
> +s_instance = this;
> +#if __ANDROID_API__ >= 23

Can there be multiple instances of this `NotifyByAndroid`?

> notifybyandroid.cpp:125
> +pixmap.save(, "PNG");
> +auto jIconData = env->NewByteArray(iconData.length());
> +env->SetByteArrayRegion(jIconData, 0, iconData.length(), 
> reinterpret_cast(iconData.constData()));

Does this need a `FreeLocalRef` call?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause, apol
Cc: broulik, apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Volker Krause]

vkrause added a comment.


  In D17851#386243 , @nicolasfella 
wrote:
  
  > Please note that all apps uploaded to the Play Store need to have a target 
API level >= 26 (Oreo). This means (among other things) that notifications need 
to have a channel (see 
https://developer.android.com/training/notify-user/channels). The current code 
will fail as soon as the app developer has a target API >= 26 specified. See 
https://developer.android.com/training/notify-user/channels for details. I 
would know how to do the required Java changes (I have done it for KDE Connect 
D9514 ) but I'm not sure if/how it's 
possible to do it without the support library/androidx and adding it to the 
build seems above my knowledge.
  
  
  Yep, I'm working on getting the support libs... I managed to get downloading 
of maven-based dependencies to work, but turns out all the Android support libs 
are AARs rather than JARs (which we can consume directly). Extracting the JAR 
from the AAR and linking against that builds, but that doesn't look like the 
right approach at all. I'd therefore like to explore building the entire Java 
side with Gradle (similar to what androiddeployqt does). This would solve the 
dependency problem for free and bring us much closer to how "normal" Android 
stuff is built, which should be much more sustainable.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause, apol
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Nicolas Fella]

nicolasfella added a comment.


  Please note that all apps uploaded to the Play Store need to have a target 
API level >= 26 (Oreo). This means (among other things) that notifications need 
to have a channel (see 
https://developer.android.com/training/notify-user/channels). The current code 
will fail as soon as the app developer has a target API >= 26 specified. See 
https://developer.android.com/training/notify-user/channels for details. I 
would know how to do the required Java changes (I have done it for KDE Connect 
D9514 ) but I'm not sure if/how it's 
possible to do it without the support library/androidx and adding it to the 
build seems above my knowledge.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause, apol
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:a54c7b17045a: Add Android notification backend (authored 
by vkrause).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17851?vs=48339=48609

REVISION DETAIL
  https://phabricator.kde.org/D17851

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/KF5Notifications-android-dependencies.xml
  src/knotificationmanager.cpp
  src/notifybyandroid.cpp
  src/notifybyandroid.h
  src/org/kde/knotifications/KNotification.java
  src/org/kde/knotifications/NotifyByAndroid.java

To: vkrause, apol
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.
This revision is now accepted and ready to land.


  These changes only will apply to Android, so let's get this in. The status 
quo is broken there anyway.
  
  As for the application, it could be in a separate repository too, no need to 
bloat the repository. For Kirigami we had to split it already because it was a 
bad idea.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause, apol
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Volker Krause]

vkrause added a comment.


  In D17851#383718 , @nicolasfella 
wrote:
  
  > By using AndroidX (formerly known as the support library) we could enable 
it for Android version older than 23 (which is about 29% of all active 
devices). That would require adding the appropriate jar to the build.
  
  
  This makes sense. The tricky part seems to be the CMake integration of a 
dependency only available via Maven. Doable it seems, but not straightforward. 
The changes needed on the Java side seem fairly minimal.
  
  In D17851#383826 , @apol wrote:
  
  > This looks really cool! I'd say let's get this in and we can iterate from 
here if needed.
  
  
  That would be much appreciated, juggling a stack of big interdependent 
patches with arc is not fun...
  
  > Maybe it would be useful to have builds of a test application?
  
  Yes. I have been testing with KDE Itinerary, but that isn't really elegant. 
Adding a dedicated test application here would add extra dependencies though.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Aleix Pol Gonzalez]

apol added a comment.


  This looks really cool! I'd say let's get this in and we can iterate from 
here if needed.
  
  Maybe it would be useful to have builds of a test application?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Nicolas Fella]

nicolasfella added a comment.


  By using AndroidX (formerly known as the support library) we could enable it 
for Android version older than 23 (which is about 29% of all active devices). 
That would require adding the appropriate jar to the build.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D17851

To: vkrause
Cc: nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Volker Krause]

vkrause created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vkrause requested review of this revision.

REVISION SUMMARY
  There are probably still many features that aren't properly mapped to
  Android, but at least the basics work:
  
  - text, title and icon
  - actions
  - dismissing the notification is propagated
  - taping the notification opens the corresponding app
  
  This is also the first time we have Java code for Android in a KF5
  framework, I tried to follow what Qt does for this.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17851

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/KF5Notifications-android-dependencies.xml
  src/knotificationmanager.cpp
  src/notifybyandroid.cpp
  src/notifybyandroid.h
  src/org/kde/knotifications/KNotification.java
  src/org/kde/knotifications/NotifyByAndroid.java

To: vkrause
Cc: kde-frameworks-devel, michaelh, ngraham, bruns