app, cmd/gogio: add android foreground permission and service #67

mixmasala · 2021-11-18T10:58:03Z

This adds the permission android.permission.FOREGROUND_SERVICE and adds
GioForegroundService which creates the tray Notification necessary to
implement the Foreground Service. The package foreground includes the method
StartForeground, which on android, notifies the system that the program
will perform background work and that it shouldn't be killed. It returns
a channel that should be closed when the background work is complete.

See https://developer.android.com/guide/components/foreground-services
and https://developer.android.com/training/notify-user/build-notification

mixmasala · 2021-11-18T11:07:41Z

I've tested this with my application; but feel that there could be better integration with the notification library gioui.org/x/notify, so that the foreground service can share the same notification channels, in particular so that the text of the background notification can be updated. I'm not sure that this change should depend on that package though, but I think that the Service class needs to be declared in the application manifest. Any help resolving/improving the above details is very much appreciated.

eliasnaur · 2021-11-18T16:27:59Z

Nice work, thank you.

I've tested this with my application; but feel that there could be better integration with the notification library gioui.org/x/notify, so that the foreground service can share the same notification channels, in particular so that the text of the background notification can be updated. I'm not sure that this change should depend on that package though, but I think that the Service class needs to be declared in the application manifest. Any help resolving/improving the above details is very much appreciated.

Why share notifications with niotify? This change is for foreground work, niotify is for notifications. However, see my review comments for a suggestion to customize foreground notification content text.

app/GioForegroundService.java

app/permission/foreground/android.go

app/permission/foreground/main.go

app/permission/foreground/android.go

eliasnaur · 2021-11-18T16:29:24Z

Oh, and remember to sign-off your change and squash commits.

mixmasala · 2021-11-18T17:25:33Z

Nice work, thank you.

I've tested this with my application; but feel that there could be better integration with the notification library gioui.org/x/notify, so that the foreground service can share the same notification channels, in particular so that the text of the background notification can be updated. I'm not sure that this change should depend on that package though, but I think that the Service class needs to be declared in the application manifest. Any help resolving/improving the above details is very much appreciated.

Why share notifications with niotify? This change is for foreground work, niotify is for notifications. However, see my review comments for a suggestion to customize foreground notification content text.

well, the foreground service requires a notification - the code here just sets a static string which isn't very appealing. Android applications using foreground services tend to have some kind of content displayed in the service notification and it would be useful to have a clean way to interact with the notification - possibly the StartForeground method should return or accept this as an argument - and this is where it starts to overlap with the functionality provided by niotify.

mixmasala · 2021-11-19T12:46:37Z

edit: fixed

mixmasala · 2021-11-26T11:50:08Z

I've refactored this PR to address issues raised above and changed the API to include setting the notification ID, title, and text.

app/os_android.go

app/permission/foreground/main.go

cmd/gogio/androidbuild.go

mixmasala · 2021-11-29T16:25:44Z

edit: stopped using startForegroundService because I saw some crash with resuming the application from background with same error as: transistorsoft/nativescript-background-geolocation-lt#121 (comment) . Instead startService is used, which fixes this crash.

app/GioForegroundService.java

app/os_android.go

app/permission/foreground/main.go

cmd/gogio/permission.go

cmd/gogio/androidbuild.go

eliasnaur

Leaving a few review comments for your follow-ups.

app/GioForegroundService.java

cmd/gogio/androidbuild.go

app/os_android.go

app/GioForegroundService.java

app/Gio.java

eliasnaur

Looks pretty good! I have a handful of comments, but it seems we can soon merge this.

app/permission/foreground/main.go

app/permission/foreground/android.go

eliasnaur · 2022-11-07T13:48:42Z

app/permission/foreground/main.go

+
+// Start is a no-op on Linux, Windows, macOS; Android will
+// display a notification during background work; iOS isn't supported.
+func Start(title, text string) (func(), error) {


Let's just delete this function, leaving just app.Start and this package only for the permission. Note in the documentation for app.Start that this package must be imported for Start to work. Also, make sure Start returns an error if the user forgets the import.

Ok, I moved code out of the app/permission path and added os_nandroid.go to add a no-op Start() method for platforms other than android so that calls to Start() and its returned stop method have no effect.

eliasnaur · 2022-11-07T13:49:46Z

app/Gio.java

+
+	static Intent startForegroundService(Context ctx, String title, String text) throws ClassNotFoundException {
+		Intent intent = new Intent();
+		intent.setClass(ctx, ctx.getClassLoader().loadClass("org/gioui/GioForegroundService"));


Why not just GioForegroundService.class instead of loadClass...?

I think that I tried doing that but the class doesn't exist unless it was first loaded.

Please post the error message you get. It doesn't make sense to me that org.gioui.Gio can be loaded but org.gioui.GioForegroundService can't.

ActivityManager
Unable to start service Intent
{ cmp=org.mixnetworks.katzen/
org.gioui.GioForegroundServie (has extras) } U=0:
not found

https://github.com/mixmasala/gio/blob/4c7028ff5fe3e9a562dbd20f5bb4a24aab4d2ff1/app/Gio.java#L73

There must be more to the error than what you pasted. A stack trace or similar that can tell us why the class cannot be loaded.

do you know how to get android to produce a stack trace here?

app/GioForegroundService.java

app/os_android.go

eliasnaur · 2022-11-07T14:04:49Z

app/os_android.go

+var foregroundService struct {
+	intent C.jobject
+	mu     sync.Mutex
+	count  int


This is much better than before, but users can still call their stop functions more than once. I suggest a map[*int]bool where each call to app.Start registers a new(int) token in the map. The stop function then returns eearly if the map no longer contains the token.

I'm afraid I don't see how users can call their stop functions more than once as the returned function checks count while ohlding the lock in order to prevent stopService from being called more than once.

There should be only one instance of the GioForegroundService, so multiple calls to Start() only indicate that some part of the application is doing something that needs to continue running, and when all of the associated stop func() have been called should the service be actually stopped.

I don't see why a map is useful here, where a counter suffices.

A program may have two unrelated background jobs, say (a) uploading an image and (b) checking for new messages. If they run concurrently, the job count is incremented to 2. A programming mistake causes (a) to calls its stop function twice, decrementing the count to 0, even though (b) is still in progress. The resulting missing notification is very hard to notice.

A map allows us to detect and ignore multiple calls to any particular stop function. In other words, it is no longer a (subtle) mistake to call your stop function twice.

ok, this makes sense to me. Thank you for the explanation.

(changes made)

mixmasala · 2022-11-10T16:31:48Z

For some reason github wont let me comment on the change requests directly any more.

I moved code out of gioui.org/app/permissions, but I was not able to find a solution to check that gioui.org/app/permissions/foreground was imported when built. For example, I thought that BuildInfo might have package-level resolution, but it does not:

+func hasPermission(permission string) bool {
+       permissionPath := filepath.Join("gioui.org/app/permission/", permission)
+       if info, ok := debug.ReadBuildInfo(); ok {
+               for _, module := range info.Deps {
+                       fmt.Println("Imports: ", module.Path)
+                       // XXX: we do not have a way to get the list of included go packages at runtime
+                       if module.Path == permissionPath {
+                               return true
+                       }
+               }
+       }
+       return false
+}

eliasnaur · 2022-11-12T17:49:41Z

For some reason github wont let me comment on the change requests directly any more.

I moved code out of gioui.org/app/permissions, but I was not able to find a solution to check that gioui.org/app/permissions/foreground was imported when built. For example, I thought that BuildInfo might have package-level resolution, but it does not:
+func hasPermission(permission string) bool {
+       permissionPath := filepath.Join("gioui.org/app/permission/", permission)
+       if info, ok := debug.ReadBuildInfo(); ok {
+               for _, module := range info.Deps {
+                       fmt.Println("Imports: ", module.Path)
+                       // XXX: we do not have a way to get the list of included go packages at runtime
+                       if module.Path == permissionPath {
+                               return true
+                       }
+               }
+       }
+       return false
+}

I don't think you need to check for the import. It should be enough to detect the Java exception/error when trying to open a foreground notification.

mixmasala · 2022-12-29T18:35:17Z

I don't think you need to check for the import. It should be enough to detect the Java exception/error when trying to open a foreground notification.

Good Idea. I tried to catch java.lang.SecurityException, but the exception is not raised to the caller of android.startForegroundService, as it is thrown in the method onStartCommand which is called by the android environment after the service is started. Do you have any ideas how to handle this?

eliasnaur · 2023-01-01T16:35:53Z

I don't think you need to check for the import. It should be enough to detect the Java exception/error when trying to open a foreground notification.

Good Idea. I tried to catch java.lang.SecurityException, but the exception is not raised to the caller of android.startForegroundService, as it is thrown in the method onStartCommand which is called by the android environment after the service is started. Do you have any ideas how to handle this?

One idea is to make onStartCommand report back to Gio whether the foreground notification succeeded or not. I think you'll need to wrap a chan error or similar with cgo.NewHandle to pass it to the Intent argument to onStartCommand. Then, in app.StartForeground wait for the channel to return an error (and free the handle).

eliasnaur

I somehow failed to send this review out after writing it. I apologize.

eliasnaur · 2023-01-07T00:55:57Z

app/Gio.java

+
+	static Intent startForegroundService(Context ctx, String title, String text) throws ClassNotFoundException {
+		Intent intent = new Intent();
+		intent.setClass(ctx, ctx.getClassLoader().loadClass("org/gioui/GioForegroundService"));


There must be more to the error than what you pasted. A stack trace or similar that can tell us why the class cannot be loaded.

eliasnaur · 2023-01-07T00:58:15Z

app/os_android.go

+				foregroundService.intent = C.jni_NewGlobalRef(env, foregroundService.intent)
+
+			} else {
+				panic(err)


You should propagate this error to the caller. In particular when the XXX issue above is resolved so that startForegroundService can fail for configuration reasons.

eliasnaur · 2023-01-07T01:00:56Z

app/os_android.go

+// Start starts the foreground service
+func Start(title, text string) (stop func(), err error) {


The documentation needs expanding. To make the documentation visible for everyone, rename this function startForeground and add a documented StartForeground in package app that calls it.

eliasnaur · 2023-01-07T01:01:21Z

app/os_nandroid.go

+package app
+
+// Start is a no-op on platforms other than android
+func Start(title, text string) (stop func(), err error) {


As described above, rename this function to startForeground.

This adds the permission android.permission.FOREGROUND_SERVICE and adds GioForegroundService which creates the tray Notification necessary to implement the Foreground Service. This adds the method Start to package app, which on android, notifies the system that the program will perform background work and that it shouldn't be killed. The foreground service is stopped using the cancel function returned by Start(). If multiple calls to Start are made, the foreground service will not be stopped until the final cancel function has been called. See https://developer.android.com/guide/components/foreground-services and https://developer.android.com/training/notify-user/build-notification Signed-off-by: Masala <masala@riseup.net>

mixmasala marked this pull request as draft November 18, 2021 11:09

mixmasala marked this pull request as ready for review November 18, 2021 11:09

eliasnaur reviewed Nov 18, 2021

View reviewed changes

mixmasala marked this pull request as draft November 19, 2021 12:43

mixmasala force-pushed the add_foreground_service branch from 596a0f2 to 87a7c2f Compare November 26, 2021 11:48

mixmasala marked this pull request as ready for review November 26, 2021 11:49

mixmasala force-pushed the add_foreground_service branch from 87a7c2f to d295b22 Compare November 26, 2021 11:51

eliasnaur reviewed Nov 26, 2021

View reviewed changes

mixmasala force-pushed the add_foreground_service branch 4 times, most recently from c65a7be to 917b1da Compare November 29, 2021 16:22

eliasnaur reviewed Dec 2, 2021

View reviewed changes

mixmasala force-pushed the add_foreground_service branch 6 times, most recently from 206d76d to 37f30b7 Compare December 9, 2021 23:32

eliasnaur reviewed Dec 10, 2021

View reviewed changes

mixmasala force-pushed the add_foreground_service branch 2 times, most recently from f0f3c2a to 84b41ef Compare December 10, 2021 09:16

mixmasala force-pushed the add_foreground_service branch from 84b41ef to 44a59e2 Compare January 2, 2022 11:28

mixmasala force-pushed the add_foreground_service branch 2 times, most recently from 698540a to 61f8a00 Compare October 8, 2022 11:58

mixmasala force-pushed the add_foreground_service branch from 61f8a00 to 66d3f9e Compare November 5, 2022 14:15

eliasnaur reviewed Nov 7, 2022

View reviewed changes

mixmasala force-pushed the add_foreground_service branch 4 times, most recently from fece051 to e2f1cfa Compare November 10, 2022 12:53

mixmasala force-pushed the add_foreground_service branch from e2f1cfa to c517124 Compare November 10, 2022 16:52

mixmasala force-pushed the add_foreground_service branch 2 times, most recently from 7fd3b23 to 079448b Compare January 2, 2023 15:52

eliasnaur reviewed Jan 19, 2023

View reviewed changes

whereswaldon force-pushed the main branch from 8ad48ab to ae3bd2a Compare September 8, 2023 16:49

mixmasala force-pushed the add_foreground_service branch from 079448b to 1c97118 Compare September 17, 2023 18:52

mixmasala force-pushed the add_foreground_service branch 2 times, most recently from 5a91e41 to a1625b7 Compare September 27, 2023 11:19

whereswaldon force-pushed the main branch 4 times, most recently from 0d543a0 to 1686874 Compare October 7, 2023 00:14

whereswaldon force-pushed the main branch from 21bf070 to fc6e51d Compare November 15, 2023 17:24

gioui deleted a comment from ddkwork Apr 11, 2024

whereswaldon force-pushed the main branch from 38d8889 to 691adf4 Compare May 1, 2024 18:00

whereswaldon force-pushed the main branch 2 times, most recently from 67c77c9 to 46cc311 Compare May 30, 2024 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app, cmd/gogio: add android foreground permission and service #67

app, cmd/gogio: add android foreground permission and service #67

mixmasala commented Nov 18, 2021

mixmasala commented Nov 18, 2021

eliasnaur commented Nov 18, 2021

eliasnaur commented Nov 18, 2021

mixmasala commented Nov 18, 2021

mixmasala commented Nov 19, 2021 •

edited

mixmasala commented Nov 26, 2021

mixmasala commented Nov 29, 2021

eliasnaur left a comment

eliasnaur left a comment

eliasnaur Nov 7, 2022

mixmasala Nov 10, 2022

eliasnaur Nov 7, 2022

mixmasala Nov 10, 2022

eliasnaur Nov 12, 2022

mixmasala Dec 29, 2022

mixmasala Dec 29, 2022

eliasnaur Jan 7, 2023

mixmasala Sep 27, 2023

eliasnaur Nov 7, 2022

mixmasala Nov 10, 2022

eliasnaur Nov 12, 2022

mixmasala Nov 24, 2022

mixmasala Jan 15, 2023

mixmasala commented Nov 10, 2022

eliasnaur commented Nov 12, 2022

mixmasala commented Dec 29, 2022

eliasnaur commented Jan 1, 2023

eliasnaur left a comment

eliasnaur Jan 7, 2023

eliasnaur Jan 7, 2023

eliasnaur Jan 7, 2023

mixmasala Sep 27, 2023

eliasnaur Jan 7, 2023

mixmasala Sep 27, 2023

		// Start starts the foreground service
		func Start(title, text string) (stop func(), err error) {

app, cmd/gogio: add android foreground permission and service #67

Are you sure you want to change the base?

app, cmd/gogio: add android foreground permission and service #67

Conversation

mixmasala commented Nov 18, 2021

mixmasala commented Nov 18, 2021

eliasnaur commented Nov 18, 2021

eliasnaur commented Nov 18, 2021

mixmasala commented Nov 18, 2021

mixmasala commented Nov 19, 2021 • edited

mixmasala commented Nov 26, 2021

mixmasala commented Nov 29, 2021

eliasnaur left a comment

Choose a reason for hiding this comment

eliasnaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixmasala commented Nov 10, 2022

eliasnaur commented Nov 12, 2022

mixmasala commented Dec 29, 2022

eliasnaur commented Jan 1, 2023

eliasnaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixmasala commented Nov 19, 2021 •

edited