WIP: [intents-handling] Extend intent functionality #1

Ganblejs · 2022-07-12T01:05:29Z

Ganblejs commented

2022-07-12 01:05:29 +00:00

First-time contributor

Now also supports starting activities in addition to sending broadcasts. Targeting services is yet to be implemented.

action, category, mimeType, data, extra, package, class and target information can now be supplied to intents (these are also the corresponding keys to use when programming intent messages to send to gadgetbridge).

Regarding WIP tag:

If/when this PR gets merged, the documentation here should be updated regarding intents as well. However, the changes don't 'break' the functionality as it is currently described in the documentation. They only extend it.
I will probably try to add support for targeting services. But that can be a separate PR.
I have tested the changes, but maybe not extensively... Though I feel like this PRs implementation are at least in some ways more reliable than how it's handled currently.
Maybe there 'should' be some cleanup of some LOG.info()-lines.
Maybe there should be more descriptive comments.
I haven't really done java and android programming before, so I could be doing mistakes without realizing it.
~~I think this PR can be merged as it is now, but I'm fine with waiting as well.~~ see comments.

Now also supports starting activities in addition to sending broadcasts. Targeting services is yet to be implemented. action, category, mimeType, data, extra, package, class and target information can now be supplied to intents (these are also the corresponding keys to use when programming intent messages to send to gadgetbridge). Regarding WIP tag: - If/when this PR gets merged, the documentation [here](https://github.com/espruino/EspruinoDocs/blob/master/info/Gadgetbridge.md) should be updated regarding intents as well. However, the changes don't 'break' the functionality as it is currently described in the documentation. They only extend it. - I will probably try to add support for targeting services. But that can be a separate PR. - I have tested the changes, but maybe not extensively... Though I feel like this PRs implementation are at least in some ways more reliable than how it's handled currently. - Maybe there 'should' be some cleanup of some LOG.info()-lines. - Maybe there should be more descriptive comments. - I haven't really done java and android programming before, so I could be doing mistakes without realizing it. - ~~I think this PR can be merged as it is now, but I'm fine with waiting as well.~~ see comments.

Ganblejs added 2 commits 2022-07-12 01:05:30 +00:00

36282676b2 Extend intent functionality

Now also supports starting activities in addition to sending broadcasts. Targeting services is yet to be implemented.

action, category, mimeType, data, extra, package, class and target information can now be supplied to intents (these are also the correspondin keys to use when programming intent messages to send to gadgetbridge).

72db23b730 add one word

Ganblejs added 1 commit 2022-07-12 09:18:45 +00:00

142b66416f Remove some comments used while testing

gfwilliams commented

2022-07-12 15:25:46 +00:00

Thanks! This looks good - and really useful, but is there a specific reason to iterate over arguments and use a switch rather than just checking for them with .has(name) and accessing them directly? It seems different to the way other code in that file works.

I guess you can report on extra fields you weren't expecting, but it does seem to have the slighting odd issue that the order of JSON fields matters, so the JSON {..., package:..., class:...} works, but {..., class:..., package:...,} doesn't.

Thanks! This looks good - and really useful, but is there a specific reason to iterate over arguments and use a switch rather than just checking for them with `.has(name)` and accessing them directly? It seems different to the way other code in that file works. I guess you can report on extra fields you weren't expecting, but it does seem to have the slighting odd issue that the order of JSON fields matters, so the JSON `{..., package:..., class:...}` works, but `{..., class:..., package:...,}` doesn't.

Ganblejs commented

2022-07-12 17:05:25 +00:00

First-time contributor

Thanks for the feedback!

[...] is there a specific reason to iterate over arguments and use a switch rather than just checking for them with .has(name) and accessing them directly? It seems different to the way other code in that file works.

Maybe not! It's probably just a sign of my inexperience and using trial and error until it worked. I'll see if I can look at other parts of the code and use a similar style.

I guess you can report on extra fields you weren't expecting, [...]

Would you recommend I don't?

[...] but it does seem to have the slighting odd issue that the order of JSON fields matters, so the JSON {..., package:..., class:...} works, but {..., class:..., package:...,} doesn't.

Could you share the messages you sent to test this?

Thanks for the feedback! > [...] is there a specific reason to iterate over arguments and use a switch rather than just checking for them with `.has(name)` and accessing them directly? It seems different to the way other code in that file works. Maybe not! It's probably just a sign of my inexperience and using trial and error until it worked. I'll see if I can look at other parts of the code and use a similar style. > I guess you can report on extra fields you weren't expecting, [...] Would you recommend I don't? > [...] but it does seem to have the slighting odd issue that the order of JSON fields matters, so the JSON `{..., package:..., class:...}` works, but `{..., class:..., package:...,}` doesn't. Could you share the messages you sent to test this?

Ganblejs added 1 commit 2022-07-12 19:33:23 +00:00

5d3512b8a6 Use if-statements to read json instead of switch case

Ganblejs added 1 commit 2022-07-12 19:56:47 +00:00

8c31fced4c Undo name change of iter for handling extra

Ganblejs added 1 commit 2022-07-12 20:17:25 +00:00

cdbe1c7044 move a break; up a line

Ganblejs commented

2022-07-12 20:56:44 +00:00

First-time contributor

@gfwilliams, I've done some changes trying to take your tips into account. I'm unsure if I should change the switch-case for handling target to if-statements as well? I feel like it makes sense to keep that as is though?

Ganblejs commented

2022-07-13 00:44:14 +00:00

First-time contributor

Just noticed that starting the activity ( startActivity() ) to open and play a song on spotify isn't as reliable as sending a broadcast to Tasker and letting tasker send the same intent to start the activity instead.

Without Tasker it works when certain apps are displayed, e.g. Gadgetbridge and android settings app. But not when e.g. my launcher app or Signal messaging app is displayed. With Tasker it works independent of what app is currently in the foreground.

I'm guessing it has something to do with setting the right context when calling startActivity(). But I don't understand how to set a context which lets me activate the activity from anywhere.

Just noticed that starting the activity ( startActivity() ) to open and play a song on spotify isn't as reliable as sending a broadcast to Tasker and letting tasker send the same intent to start the activity instead. Without Tasker it works when certain apps are displayed, e.g. Gadgetbridge and android settings app. But not when e.g. my launcher app or Signal messaging app is displayed. With Tasker it works independent of what app is currently in the foreground. I'm guessing it has something to do with setting the right context when calling startActivity(). But I don't understand how to set a context which lets me activate the activity from anywhere.

gfwilliams commented

2022-07-15 09:16:21 +00:00

Hi - yeah, I'm afraid I don't know much about startActivity so I can't really help you there - have you checked to see if it's used anywhere else in Gadgetbridge? It might give you some hints.

Those changes look great to me. Personally I'd probably make the text for target all lowercase without spaces, just to match what we send for other fields a bit better (http://www.espruino.com/Gadgetbridge#messages-sent-to-bangle-js-from-phone) but unless you have any reasons not to I'd be very happy to merge this in now?

Hi - yeah, I'm afraid I don't know much about startActivity so I can't really help you there - have you checked to see if it's used anywhere else in Gadgetbridge? It might give you some hints. Those changes look great to me. Personally I'd probably make the text for `target` all lowercase without spaces, just to match what we send for other fields a bit better (http://www.espruino.com/Gadgetbridge#messages-sent-to-bangle-js-from-phone) but unless you have any reasons not to I'd be very happy to merge this in now?

Ganblejs commented

2022-07-15 09:27:02 +00:00

First-time contributor

have you checked to see if it's used anywhere else in Gadgetbridge? It might give you some hints.

I'll do that!

Those changes look great to me. Personally I'd probably make the text for target all lowercase without spaces, just to match what we send for other fields a bit better (http://www.espruino.com/Gadgetbridge#messages-sent-to-bangle-js-from-phone)

I'll check that out as well!

but unless you have any reasons not to I'd be very happy to merge this in now?

Now when I've seen that starting activities is a little hit and miss, I would like to see if I can fix that before it's merged.

Thanks for the tips! :)

> have you checked to see if it's used anywhere else in Gadgetbridge? It might give you some hints. I'll do that! > Those changes look great to me. Personally I'd probably make the text for `target` all lowercase without spaces, just to match what we send for other fields a bit better (http://www.espruino.com/Gadgetbridge#messages-sent-to-bangle-js-from-phone) I'll check that out as well! > but unless you have any reasons not to I'd be very happy to merge this in now? Now when I've seen that starting activities is a little hit and miss, I would like to see if I can fix that before it's merged. Thanks for the tips! :)

gfwilliams commented

2022-07-15 09:56:45 +00:00

Ok, great - thanks! This is going to be a really neat addition when it's merged in

Ganblejs commented

2022-07-15 20:06:24 +00:00

First-time contributor

I got this question in the Gadgetbridge matrix chat:

I do wonder why you're submitting this to be Gadgetbridge fork instead of to the main project? Given intents is part of the main project it seems most logical if extending it would be too?

What do you say @gfwilliams, should the PR be switched over to the main Gadgetbridge project?

I got this question in the Gadgetbridge matrix chat: > I do wonder why you're submitting this to be Gadgetbridge fork instead of to the main project? Given intents is part of the main project it seems most logical if extending it would be too? What do you say @gfwilliams, should the PR be switched over to the main Gadgetbridge project?

gfwilliams commented

2022-07-18 14:13:28 +00:00

Yes, that sounds great to me. Saves me having to push it back :)

Ganblejs added 1 commit 2022-07-18 20:50:55 +00:00

e60c92880c Add permission for drawing over other applications

... to let intents sent from Gadgetbridge start activities also when in the background.

Ganblejs added 1 commit 2022-07-18 20:56:09 +00:00

aa220c4667 small change to make code more uniform

Ganblejs added 1 commit 2022-07-18 21:30:58 +00:00

278b4c88bf Reorder writing of intent contents

Ganblejs added 1 commit 2022-07-18 21:43:15 +00:00

c8abe37507 Remove some LOG.info()-lines

Ganblejs added 1 commit 2022-07-18 21:48:52 +00:00

482cce2184 all lowercase w/o spaces for keys and values of bangle.js intent message

Ganblejs commented

2022-07-18 22:05:42 +00:00

First-time contributor

This Tasker forum post had the answer to how to let Gadgetbridge start activities while in the background. It needs the permission SYSTEM_ALERT_WINDOW which is now introduced to the bangle.js AndroidManifest.xml file. The user needs to grant this permission in system settings app info section. Ideally I would want there to be a pop-up like with other permissions (e.g. location, notifications, do not disturb) which forwarded the user to this section for easier access. I don't know how to do this though, yet at least.

Personally I'd probably make the text for target all lowercase without spaces, just to match what we send for other fields a bit better (http://www.espruino.com/Gadgetbridge#messages-sent-to-bangle-js-from-phone)

I have changed it to what I think you suggested. But from looking at the espruino gadgetbridge documentation in the link I don't really see a precedent for the values of the dictionary/JSON always being lower case without spaces - only the keys. Is the change in line with what you thought of?

Yes, that sounds great to me. Saves me having to push it back :)

Ok, I'll do a PR to the main project and close this one after you've answered my question regarding keys and values above. :)

[This Tasker forum post](https://groups.google.com/g/tasker/c/974macgH-OA/m/h1kbEoFgAAAJ) had the answer to how to let Gadgetbridge start activities while in the background. It needs the permission SYSTEM_ALERT_WINDOW which is now introduced to the bangle.js AndroidManifest.xml file. The user needs to grant this permission in system settings app info section. Ideally I would want there to be a pop-up like with other permissions (e.g. location, notifications, do not disturb) which forwarded the user to this section for easier access. I don't know how to do this though, yet at least. > Personally I'd probably make the text for target all lowercase without spaces, just to match what we send for other fields a bit better (http://www.espruino.com/Gadgetbridge#messages-sent-to-bangle-js-from-phone) I have changed it to what I think you suggested. But from looking at the espruino gadgetbridge documentation in the link I don't really see a precedent for the values of the dictionary/JSON always being lower case without spaces - only the keys. Is the change in line with what you thought of? > Yes, that sounds great to me. Saves me having to push it back :) Ok, I'll do a PR to the main project and close this one after you've answered my question regarding keys and values above. :)

Ganblejs added 1 commit 2022-07-18 22:38:23 +00:00

48d3812f80 Make code more uniform by removing some spaces

gfwilliams commented

2022-07-19 08:50:50 +00:00

This looks really good to me - thanks!

Ideally I would want there to be a pop-up like with other permissions (e.g. location, notifications, do not disturb) which forwarded the user to this section for easier access. I don't know how to do this though, yet at least.

You could take a look at app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/ControlCenterv2.java - that's where we request permissions for the other stuff. You can see where we check for INTERNET_ACCESS (based on the current build)

Maybe something like this might do it (untested!):

if (BuildConfig.FLAVOR.equals("banglejs"))
	if (ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) {
    	wantedPermissions.add(Manifest.permission.SYSTEM_ALERT_WINDOW);
    }

I don't really see a precedent for the values of the dictionary/JSON always being lower case without spaces

Well, I think they are all upper or lower, and without spaces?

But yes, ideally they should all be lowercase but that's not always the case.

Is the change in line with what you thought of?

Yes, that's perfect - thanks :)

This looks really good to me - thanks! > Ideally I would want there to be a pop-up like with other permissions (e.g. location, notifications, do not disturb) which forwarded the user to this section for easier access. I don't know how to do this though, yet at least. You could take a look at app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/ControlCenterv2.java - that's where we request permissions for the other stuff. You can see where we check for INTERNET_ACCESS (based on the current build) Maybe something like this might do it (untested!): ``` if (BuildConfig.FLAVOR.equals("banglejs")) if (ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) { wantedPermissions.add(Manifest.permission.SYSTEM_ALERT_WINDOW); } ``` > I don't really see a precedent for the values of the dictionary/JSON always being lower case without spaces Well, I think they are all upper or lower, and without spaces? But yes, ideally they should all be lowercase but that's not always the case. > Is the change in line with what you thought of? Yes, that's perfect - thanks :)

Ganblejs commented

2022-07-19 09:14:58 +00:00

First-time contributor

Well, I think they are all upper or lower, and without spaces?

Yes, I get it now when I look again. I was confused by e.g. "src":"Hangouts".

I'll give the permission pop-up a try. :)

> Well, I think they are all upper or lower, and without spaces? Yes, I get it now when I look again. I was confused by e.g. "src":"Hangouts". I'll give the permission pop-up a try. :)

Ganblejs added 1 commit 2022-07-19 11:37:02 +00:00

c2576d8b85 Add info for intents on Bangle.js

Ganblejs added 1 commit 2022-07-19 11:46:39 +00:00

5963972279 Add pop-up asking for permission to "Display over other apps"

Ganblejs commented

2022-07-19 11:50:47 +00:00

First-time contributor

I've implemented the pop-up for allowing displaying over other apps.

I'll close this PR now and open a new one to the main Gadgetbridge project.

Thanks for all the feedback! :)

I've implemented the pop-up for allowing displaying over other apps. I'll close this PR now and open a new one to the main Gadgetbridge project. Thanks for all the feedback! :)

Ganblejs closed this pull request

2022-07-19 11:51:06 +00:00

gfwilliams reviewed 2022-07-19 12:30:27 +00:00

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/ControlCenterv2.java

					
				@ -581,0 +600,4 @@

				                    .setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() {

				                        public void onClick(DialogInterface dialog, int id) {

				                            startActivity(new Intent(android.provider.Settings.ACTION_APPLICATION_DETAILS_SETTINGS,

				                                    Uri.fromParts("package", "com.espruino.gadgetbridge.banglejs", null)));

gfwilliams commented

2022-07-19 12:30:27 +00:00

Just thinking here we might want to use BuildConfig.APPLICATION_ID instead, so it can work with other builds.

...and, does this definitely not come up for 'normal' Gadgetbridge builds where SYSTEM_ALERT_WINDOW isn't in the manifest?

Just thinking here we might want to use BuildConfig.APPLICATION_ID instead, so it can work with other builds. ...and, does this definitely not come up for 'normal' Gadgetbridge builds where SYSTEM_ALERT_WINDOW isn't in the manifest?

gfwilliams reviewed 2022-07-19 12:31:35 +00:00

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/ControlCenterv2.java

					
				@ -581,0 +594,4 @@

				            // Use the Builder class for convenient dialog construction

				            AlertDialog.Builder builder = new AlertDialog.Builder(getActivity());

				            Context context = getContext();

				            builder.setMessage(context.getString(R.string.permission_display_over_other_apps,

gfwilliams commented

2022-07-19 12:31:35 +00:00

Just checking, but is permission_display_over_other_apps defined? I don't see it in strings.xml

Ganblejs commented

2022-07-19 12:38:50 +00:00

First-time contributor

Just thinking here we might want to use BuildConfig.APPLICATION_ID instead, so it can work with other builds.

This is probably an oversight on my part b/c of my inexperience. Trial and error is yet again how I got to that code. I will try to implement what you suggest! :)

...and, does this definitely not come up for 'normal' Gadgetbridge builds where SYSTEM_ALERT_WINDOW isn't in the manifest?

I don't know, really... If you think so I would believe that is the case. Hmm, do you have a suggestion on what I should to to do it right?

Just checking, but is permission_display_over_other_apps defined? I don't see it in strings.xml

Yeah, I believe so. The pop-up message looks as I want it at least. I'll check though. EDIT: I only had it locally. I have uploaded now. But since I had closed the PR now the code wont sync here.

> Just thinking here we might want to use BuildConfig.APPLICATION_ID instead, so it can work with other builds. This is probably an oversight on my part b/c of my inexperience. Trial and error is yet again how I got to that code. I will try to implement what you suggest! :) > ...and, does this definitely not come up for 'normal' Gadgetbridge builds where SYSTEM_ALERT_WINDOW isn't in the manifest? I don't know, really... If you think so I would believe that is the case. Hmm, do you have a suggestion on what I should to to do it right? > Just checking, but is permission_display_over_other_apps defined? I don't see it in strings.xml Yeah, I believe so. The pop-up message looks as I want it at least. I'll check though. EDIT: I only had it locally. I have uploaded now. But since I had closed the PR now the code wont sync here.

gfwilliams commented

2022-07-19 13:12:35 +00:00

Thanks! And sorry for suggesting all the changes :) What you've done looks great, but it's just making it so it doesn't break anything for 'normal' Gadgetbridge users.

As far as I can tell, ACTION_APPLICATION_DETAILS_SETTINGS is bringing up the settings page for the particular app so permissions can be enabled? This is a great idea as Gadgetbridge never seemed to be able to do that before. I always found giving permissions for Background Location was a real pain.

But looking at it, I see you check if (!set.contains(this.getPackageName())) { which what the Notification Listener does in the few lines above - so I think it's only checking for notification permissions? It feels like it might actually want to be checking something else?

Maybe:

if (BuildConfig.FLAVOR.equals("banglejs") && 
    ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) {

But maybe it's useful for other things too (like the background location)...

For permission_display_over_other_apps It could be changes to strings.xml didn't get committed? I don't see it in the app here and it doesn't seem to be in the PR?

Thanks! And sorry for suggesting all the changes :) What you've done looks great, but it's just making it so it doesn't break anything for 'normal' Gadgetbridge users. As far as I can tell, ACTION_APPLICATION_DETAILS_SETTINGS is bringing up the settings page for the particular app so permissions can be enabled? This is a great idea as Gadgetbridge never seemed to be able to do that before. I always found giving permissions for Background Location was a real pain. But looking at it, I see you check `if (!set.contains(this.getPackageName())) {` which what the Notification Listener does in the few lines above - so I think it's only checking for notification permissions? It feels like it might actually want to be checking something else? Maybe: ``` if (BuildConfig.FLAVOR.equals("banglejs") && ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) { ``` But maybe it's useful for other things too (like the background location)... For `permission_display_over_other_apps` It could be changes to `strings.xml` didn't get committed? I don't see it in the app here and it doesn't seem to be in the PR?

Ganblejs commented

2022-07-19 13:21:12 +00:00

First-time contributor

As far as I can tell, ACTION_APPLICATION_DETAILS_SETTINGS is bringing up the settings page for the particular app so permissions can be enabled?

Yes, I tried to mimic how permissions for notifications and do not disturb was implemented and used stackoverflow.

But looking at it, I see you check if (!set.contains(this.getPackageName())) { which what the Notification Listener does in the few lines above - so I think it's only checking for notification permissions? It feels like it might actually want to be checking something else?

Yes, I started with copying that code :P

It feels like it might actually want to be checking something else?

Maybe:

if (BuildConfig.FLAVOR.equals("banglejs") &&
ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) {

Ok, that makes sense!

For permission_display_over_other_apps It could be changes to strings.xml didn't get committed? I don't see it in the app here and it doesn't seem to be in the PR?

Yes, exactly what happened ;) And I can't sync the fork to this PR anymore.

> As far as I can tell, ACTION_APPLICATION_DETAILS_SETTINGS is bringing up the settings page for the particular app so permissions can be enabled? Yes, I tried to mimic how permissions for notifications and do not disturb was implemented and [used stackoverflow](https://stackoverflow.com/questions/32822101/how-can-i-programmatically-open-the-permission-screen-for-a-specific-app-on-andr?rq=1). > But looking at it, I see you check if (!set.contains(this.getPackageName())) { which what the Notification Listener does in the few lines above - so I think it's only checking for notification permissions? It feels like it might actually want to be checking something else? Yes, I started with copying that code :P >It feels like it might actually want to be checking something else? > >Maybe: > >if (BuildConfig.FLAVOR.equals("banglejs") && > ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) { Ok, that makes sense! >For permission_display_over_other_apps It could be changes to strings.xml didn't get committed? I don't see it in the app here and it doesn't seem to be in the PR? Yes, exactly what happened ;) And I can't sync the fork to this PR anymore.

Ganblejs commented

2022-07-19 13:40:31 +00:00

First-time contributor

if (BuildConfig.FLAVOR.equals("banglejs") &&
ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) {

I've tried that now and it's perfect! ;)

I'll get to opening the other PR now so you can follow along in the code again.

> if (BuildConfig.FLAVOR.equals("banglejs") && ActivityCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.SYSTEM_ALERT_WINDOW) == PackageManager.PERMISSION_DENIED) { I've tried that now and it's perfect! ;) I'll get to opening the other PR now so you can follow along in the code again.

Ganblejs commented

2022-07-19 14:02:52 +00:00

First-time contributor

...and, does this definitely not come up for 'normal' Gadgetbridge builds where SYSTEM_ALERT_WINDOW isn't in the manifest?

Is this still an issue after I implemented your latest suggestion do you think?

> ...and, does this definitely not come up for 'normal' Gadgetbridge builds where SYSTEM_ALERT_WINDOW isn't in the manifest? Is this still an issue after I implemented your latest suggestion do you think?

gfwilliams commented

2022-07-19 14:16:02 +00:00

Is this still an issue after I implemented your latest suggestion do you think?

No, that's fine - the BuildConfig.FLAVOR.equals("banglejs") should fix that :)

> Is this still an issue after I implemented your latest suggestion do you think? No, that's fine - the `BuildConfig.FLAVOR.equals("banglejs")` should fix that :)

gfwilliams commented

2022-07-19 14:16:17 +00:00

Thanks for doing all these tweaks - it'll be great to get this into builds!