Freeyourgadget/Gadgetbridge
Freeyourgadget
/
Gadgetbridge
57
816
Fork 337
Code Issues 758 Pull Requests 37 Releases Wiki Activity

Bangle.js: Intents - continue extending functionality #2826

Merged
ashimokawa merged 1 commits from Ganblejs/Gadgetbridge:Intents into master 2022-09-19 19:52:41 +00:00
Conversation 33 Commits 1 Files Changed 7 +114 -31
Ganblejs commented 2022-08-21 10:39:51 +00:00
Contributor
Copy Link

This is a continuation of PR #2769.

DONE:

  • Reorder code to follow this order "target, action, flags, categories, package, class, mimetype, data and extra". As to group them with some kind of logic. (The target isn't actually part of the intent. The flags say how an action should behave. categories, package and class specifies what app(s)/activity/class can or should be used for the action. mimetyp, data and extra are used to supply information to act on.)

  • Add support for setting flags for intents.

  • Add ability for GB to wake the device and leave the lock screen to start activities when the device is sleeping. (The password can be bypassed if the user adds the Bangle.js as a trusted device) (This works now with the help of a new waking activity, but needs some further work to be reliable when launching follow up activity. I believe that as it is now the waking activity will not always have the time to wake and unlock the phone before the next activity tries to launch, so nothing happens in that case. SEE "Improve waking ability"-point below.)

  • Add support for setting multiple categories for an intent.

  • Possibly remove imports I introduced which are maybe not needed.

  • Improve waking ability. (send the intent you want for starting activity you want, then send intent for waking the android device. Sending only the waking intent twice will open the android device on the activity active when it went to sleep.) (how to send waking intent from Bangle.js: Bluetooth.println(JSON.stringify({t:"intent", target:"activity", flags:["FLAG_ACTIVITY_NEW_TASK", "FLAG_ACTIVITY_CLEAR_TASK", "FLAG_ACTIVITY_EXCLUDE_FROM_RECENTS", "FLAG_ACTIVITY_NO_ANIMATION"], package:"gadgetbridge", class:"nodomain.freeyourgadget.gadgetbridge.activities.WakeActivity"}));
    )

  • Add service intent support. (Think this is implemented now, but have not tested it yet.) (Started testing this, doesn't seem to work yet..) (Works at least for explicit intents. From my testing it doesn't work for implicit intents, see this stackoverflow question. Tested and working with sending the following from Bangle.js to Gadgetbridge to control the music app Poweramp: Bluetooth.println(JSON.stringify({t:"intent", target:"foregroundservice", action:"com.maxmpz.audioplayer.API_COMMAND", package:"com.maxmpz.audioplayer", extra:{cmd:"TOGGLE_PLAY_PAUSE"}}));
    )

  • Add dismiss-button to 'display over other apps' permission pop up, add instruction to the message on how to disable 'Check permission status'.

Here's the Bangle.js app I use for testing. And here's its js-code.

FUTURE WORK FOR ANOTHER PR:

  • Instead of asking for permission to "display over other apps" along with the other permissions, ask for it when toggling "Allow Intents" setting on under "Device specific settings".
    • This is proving to be beyond what I'm able to grasp and implement at the moment, it's hard for me to get an overview of how the code I would like to hook into works. If anyone would like to do this in a follow up PR that would be much appreciated (See @joserebelo 's response regarding this in the comments). Otherwise I might give it a go again sooner or later.
This is a continuation of PR #2769. DONE: - [x] Reorder code to follow this order "target, action, flags, categories, package, class, mimetype, data and extra". As to group them with some kind of logic. (The target isn't actually part of the intent. The flags say how an action should behave. categories, package and class specifies what app(s)/activity/class can or should be used for the action. mimetyp, data and extra are used to supply information to act on.) - [x] Add support for setting flags for intents. - [x] Add ability for GB to wake the device and leave the lock screen to start activities when the device is sleeping. (The password can be bypassed if the user adds the Bangle.js as a trusted device) (This works now with the help of a new waking activity, ~~but needs some further work to be reliable when launching follow up activity. I believe that as it is now the waking activity will not always have the time to wake and unlock the phone before the next activity tries to launch, so nothing happens in that case.~~ **SEE "Improve waking ability"-point below.**) - [x] Add support for setting multiple categories for an intent. - [x] Possibly remove imports I introduced which are maybe not needed. - [x] Improve waking ability. (send the intent you want for starting activity you want, then send intent for waking the android device. Sending only the waking intent twice will open the android device on the activity active when it went to sleep.) *(how to send waking intent from Bangle.js:* `Bluetooth.println(JSON.stringify({t:"intent", target:"activity", flags:["FLAG_ACTIVITY_NEW_TASK", "FLAG_ACTIVITY_CLEAR_TASK", "FLAG_ACTIVITY_EXCLUDE_FROM_RECENTS", "FLAG_ACTIVITY_NO_ANIMATION"], package:"gadgetbridge", class:"nodomain.freeyourgadget.gadgetbridge.activities.WakeActivity"}));` *)* - [x] Add service intent support. ~~(Think this is implemented now, but have not tested it yet.)~~ ~~(Started testing this, doesn't seem to work yet..)~~ (Works at least for explicit intents. From my testing it doesn't work for implicit intents, see [this stackoverflow question](https://stackoverflow.com/questions/73586755/how-to-use-implicit-intents-to-start-a-service-from-another-package-app). Tested and working with sending the following from Bangle.js to Gadgetbridge to control the music app Poweramp: `Bluetooth.println(JSON.stringify({t:"intent", target:"foregroundservice", action:"com.maxmpz.audioplayer.API_COMMAND", package:"com.maxmpz.audioplayer", extra:{cmd:"TOGGLE_PLAY_PAUSE"}}));` ) - [x] Add dismiss-button to 'display over other apps' permission pop up, add instruction to the message on how to disable 'Check permission status'. [Here's the Bangle.js app I use for testing](https://thyttan.github.io/BangleApps/?q=testintents). And [here's its js-code](https://github.com/thyttan/BangleApps/blob/thyttan-app-loader/apps/testintents/app.js). FUTURE WORK FOR ANOTHER PR: - Instead of asking for permission to "display over other apps" along with the other permissions, ask for it when toggling "Allow Intents" setting on under "Device specific settings". - This is proving to be beyond what I'm able to grasp and implement at the moment, it's hard for me to get an overview of how the code I would like to hook into works. If anyone would like to do this in a follow up PR that would be much appreciated (See @joserebelo 's [response](https://codeberg.org/Freeyourgadget/Gadgetbridge/pulls/2826#issuecomment-607870) regarding this in the comments). Otherwise I might give it a go again sooner or later.
Ganblejs changed title from WIP: Bangle.js: Continued extension of intents functionality to WIP: Bangle.js: Intents - continue extending functionality 2022-08-21 12:32:56 +00:00
Ganblejs force-pushed Intents from 726c223bbb to 29d9f8b9d6 2022-08-21 17:39:18 +00:00 Compare
joserebelo reviewed 2022-08-22 10:00:29 +00:00
app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/banglejs/BangleJSDeviceSupport.java Outdated
@ -750,0 +777,4 @@
and android.content.Intent.FLAG_RECEIVER_OFFLOAD_FOREGROUND
*/
// Commented out some cases below that Android Studio said it could not resolve, mainly those starting with "android.content.Intent.".
switch (flag) {
joserebelo commented 2022-08-22 10:00:29 +00:00
Member
Copy Link

I'd replace this switch with a map:

final Map<String, Integer> intentFlags = new HashMap<String, Integer>() {{
    put("FLAG_GRANT_READ_URI_PERMISSION", Intent.FLAG_GRANT_READ_URI_PERMISSION);
    put("FLAG_GRANT_WRITE_URI_PERMISSION", Intent.FLAG_GRANT_WRITE_URI_PERMISSION);
    // ...
}};

if (intentFlags.containsKey(flag)) {
    // ...
}
I'd replace this switch with a map: ```java final Map<String, Integer> intentFlags = new HashMap<String, Integer>() {{ put("FLAG_GRANT_READ_URI_PERMISSION", Intent.FLAG_GRANT_READ_URI_PERMISSION); put("FLAG_GRANT_WRITE_URI_PERMISSION", Intent.FLAG_GRANT_WRITE_URI_PERMISSION); // ... }}; if (intentFlags.containsKey(flag)) { // ... } ```
Ganblejs commented 2022-08-22 10:04:42 +00:00
Author
Contributor
Copy Link

Ok! I will look at doing that. I don't know the advantages of switch vs map myself. I'll just trust your judgement! :P

Ok! I will look at doing that. I don't know the advantages of switch vs map myself. I'll just trust your judgement! :P
joserebelo commented 2022-08-22 10:06:01 +00:00
Member
Copy Link

I just find it more readable / less error-prone, but it may be a bit personal preference.

I just find it more readable / less error-prone, but it may be a bit personal preference.
👍 1
Ganblejs commented 2022-08-22 20:38:52 +00:00
Author
Contributor
Copy Link

I just realized there is no need for this new function really. I can just add an instruction in the espruino docs to supply the "Constant Value" of flags, which are integers, instead of the name of them (example: FLAG_ACTIVITY_NEW_TASK Constant Value is 268435456).

On the other hand, having the function might improve readability of Bangle.js app code where it's easier to understand/remember what the string "FLAG_ACTIVITY_NEW_TASK" implies rather than the integer 268435456...

Thoughts on this? @joserebelo @gfwilliams

I just realized there is no need for this new function really. I can just add an instruction in the espruino docs to supply the "Constant Value" of flags, which are integers, instead of the name of them (example: [FLAG_ACTIVITY_NEW_TASK Constant Value is 268435456](https://developer.android.com/reference/android/content/Intent#FLAG_ACTIVITY_NEW_TASK)). On the other hand, having the function might improve readability of Bangle.js app code where it's easier to understand/remember what the string "FLAG_ACTIVITY_NEW_TASK" implies rather than the integer 268435456... Thoughts on this? @joserebelo @gfwilliams
joserebelo commented 2022-08-22 23:00:04 +00:00
Member
Copy Link

I like the idea of sending the flag names, it does make it easier / the code more readable, people will end up having to create the constants on the Bangle.js app code anyway.

We could also use reflection to avoid declaring all of them on Gadgetbridge side and keep it compatible going forward. I would say that this code is not performance-critical, so it should not be too bad:

final Class<Intent> intentClass = Intent.class;
final Field flagField = intentClass.getDeclaredField(flag);
// use flagField.get(null);

With some more error-checking, of course.

Just my 2 cents, I got no strong feelings :)

I like the idea of sending the flag names, it does make it easier / the code more readable, people will end up having to create the constants on the Bangle.js app code anyway. We could also use reflection to avoid declaring all of them on Gadgetbridge side and keep it compatible going forward. I would say that this code is not performance-critical, so it should not be too bad: ```java final Class<Intent> intentClass = Intent.class; final Field flagField = intentClass.getDeclaredField(flag); // use flagField.get(null); ``` With some more error-checking, of course. Just my 2 cents, I got no strong feelings :)
👍 1
Ganblejs commented 2022-08-23 09:42:09 +00:00
Author
Contributor
Copy Link

I tried this just testing it out:

final Class<Intent> intentClass = Intent.class;
final Field flagField = intentClass.getDeclaredField("FLAG_ACTIVITY_NEW_TASK");

and got this:

error: unreported exception NoSuchFieldException; must be caught or declared to be thrown
final Field flagField = intentClass.getDeclaredField("FLAG_ACTIVITY_NEW_TASK");

I've looked at "Intent.class" where this line (334) exist:

public static final int FLAG_ACTIVITY_NEW_TASK = 268435456;
I tried this just testing it out: ``` final Class<Intent> intentClass = Intent.class; final Field flagField = intentClass.getDeclaredField("FLAG_ACTIVITY_NEW_TASK"); ``` and got this: > error: unreported exception NoSuchFieldException; must be caught or declared to be thrown final Field flagField = intentClass.getDeclaredField("FLAG_ACTIVITY_NEW_TASK"); I've looked at "Intent.class" where this line (334) exist: ``` public static final int FLAG_ACTIVITY_NEW_TASK = 268435456; ```
joserebelo commented 2022-08-23 09:44:06 +00:00
Member
Copy Link

You need to put all of that inside a try ... catch, since a NoSuchFieldException will be thrown if the user sends an invalid flag.

try {
    final Class<Intent> intentClass = Intent.class;
    final Field flagField = intentClass.getDeclaredField("FLAG_ACTIVITY_NEW_TASK");
} catch (final Exception e) {
   // The user sent an invalid flag 
}
You need to put all of that inside a `try ... catch`, since a `NoSuchFieldException` will be thrown if the user sends an invalid flag. ```java try { final Class<Intent> intentClass = Intent.class; final Field flagField = intentClass.getDeclaredField("FLAG_ACTIVITY_NEW_TASK"); } catch (final Exception e) { // The user sent an invalid flag } ```
👍 1
Ganblejs commented 2022-08-23 10:31:46 +00:00
Author
Contributor
Copy Link

Ah, ok - thanks!

Ah, ok - thanks!
Ganblejs force-pushed Intents from affacc12a7 to 5254c00642 2022-08-23 12:30:14 +00:00 Compare
Ganblejs commented 2022-08-23 13:20:40 +00:00
Author
Contributor
Copy Link

I'm looking at moving the "permission to display over other apps" to appear when toggling the setting in "Device specific settings" instead. But I'm having a hard time finding where to hook into this.

I've found this on line 187 in BanglejsCoordinator.java:

settings.add(R.xml.devicesettings_device_intents);

But that's about as far as I get searching in android studio. Is there a way to know when the setting is toggled and display the pop-up then?

I'm looking at moving the "permission to display over other apps" to appear when toggling the setting in "Device specific settings" instead. But I'm having a hard time finding where to hook into this. I've found this on line 187 in BanglejsCoordinator.java: ``` settings.add(R.xml.devicesettings_device_intents); ``` But that's about as far as I get searching in android studio. Is there a way to know when the setting is toggled and display the pop-up then?
joserebelo commented 2022-08-23 13:23:57 +00:00
Member
Copy Link

You can hook into that on DeviceSpecificSettingsFragment, see the setChangeListener function.

You can hook into that on `DeviceSpecificSettingsFragment`, see the `setChangeListener` function.
❤️ 1
Ganblejs closed this pull request 2022-08-24 14:17:06 +00:00
Ganblejs reopened this pull request 2022-08-24 15:16:25 +00:00
Ganblejs changed title from WIP: Bangle.js: Intents - continue extending functionality to Bangle.js: Intents - continue extending functionality 2022-08-24 15:38:25 +00:00
Ganblejs commented 2022-08-24 15:40:42 +00:00
Author
Contributor
Copy Link

I think the PR is ready for review now. If/when the code seems adequate I can rebase and squash the commits in preparation for merging.

I think the PR is ready for review now. If/when the code seems adequate I can rebase and squash the commits in preparation for merging.
Ganblejs commented 2022-08-24 22:47:32 +00:00
Author
Contributor
Copy Link

Android Studio warn about using getApplicationContext(), so I changed to only using getContext() when executing the bangle.js intents. I've been mimicing the original implementation of broadcasts until now where getApplicationContext() was used. Is there a specific reason for using getApplicationContext(), @gfwilliams? From the testing I've done I don't notice any difference in function.

EDIT: Also, from a little testing it seems I can drop the "this." in front of getContext(), without breaking sending broadcasts or starting activities.

Android studio said:

android.content.Context public abstract Context getApplicationContext()

Return the context of the single, global Application object of the current process. This generally should only be used if you need a Context whose lifecycle is separate from the current context, that is tied to the lifetime of the process rather than the current component.

Consider for example how this interacts with registerReceiver(BroadcastReceiver, IntentFilter):
If used from an Activity context, the receiver is being registered within that activity. This means that you are expected to unregister before the activity is done being destroyed; in fact if you do not do so, the framework will clean up your leaked registration as it removes the activity and log an error. Thus, if you use the Activity context to register a receiver that is static (global to the process, not associated with an Activity instance) then that registration will be removed on you at whatever point the activity you used is destroyed.

If used from the Context returned here, the receiver is being registered with the global state associated with your application. Thus it will never be unregistered for you. This is necessary if the receiver is associated with static data, not a particular component. However using the ApplicationContext elsewhere can easily lead to serious leaks if you forget to unregister, unbind, etc.

< Android API 29 Platform >

Android Studio warn about using getApplicationContext(), so I changed to only using getContext() when executing the bangle.js intents. I've been mimicing the original implementation of broadcasts until now where getApplicationContext() was used. Is there a specific reason for using getApplicationContext(), @gfwilliams? From the testing I've done I don't notice any difference in function. EDIT: Also, from a little testing it seems I can drop the "this." in front of getContext(), without breaking sending broadcasts or starting activities. Android studio said: > android.content.Context public abstract Context getApplicationContext() > >Return the context of the single, global Application object of the current process. This generally should only be used if you need a Context whose lifecycle is separate from the current context, that is tied to the lifetime of the process rather than the current component. > >Consider for example how this interacts with registerReceiver(BroadcastReceiver, IntentFilter): If used from an Activity context, the receiver is being registered within that activity. This means that you are expected to unregister before the activity is done being destroyed; in fact if you do not do so, the framework will clean up your leaked registration as it removes the activity and log an error. Thus, if you use the Activity context to register a receiver that is static (global to the process, not associated with an Activity instance) then that registration will be removed on you at whatever point the activity you used is destroyed. > >If used from the Context returned here, the receiver is being registered with the global state associated with your application. Thus it will never be unregistered for you. This is necessary if the receiver is associated with static data, not a particular component. However using the ApplicationContext elsewhere can easily lead to serious leaks if you forget to unregister, unbind, etc. > > < Android API 29 Platform >
Ganblejs force-pushed Intents from 6226895c87 to 25e4791f0a 2022-08-28 20:55:52 +00:00 Compare
Ganblejs commented 2022-09-02 09:50:55 +00:00
Author
Contributor
Copy Link

I'm testing intents for startings services without success.

Testing with this:

Action: com.maxmpz.audioplayer.API_COMMAND Extra: cmd:1 Target: Service

From here.

Using that with tasker works for the action "play/pause" in the music player Poweramp. But so far I'm not able to make Gadgetbridge do the same.

EDIT: I've introduced (locally) <uses-permission android:name="android.permission.QUERY_ALL_PACKAGES" /> to AndroidManifest as per this stackoverflow answer.

EDIT: Opened a question on stackoverflow.

EDIT: By adding package information to the intent, making it explicit, it can control poweramp. As per these instructions.

I'm testing intents for startings services without success. Testing with this: > Action: com.maxmpz.audioplayer.API_COMMAND Extra: cmd:1 Target: Service From [here](https://forum.powerampapp.com/topic/6497-tasker-intents-help/). Using that with tasker works for the action "play/pause" in the music player Poweramp. But so far I'm not able to make Gadgetbridge do the same. EDIT: I've introduced (locally) `<uses-permission android:name="android.permission.QUERY_ALL_PACKAGES" />` to AndroidManifest as per [this stackoverflow answer](https://stackoverflow.com/a/67675305). EDIT: Opened [a question on stackoverflow](https://stackoverflow.com/questions/73586755/how-to-use-implicit-intents-to-start-a-service-from-another-package-app). EDIT: By adding package information to the intent, making it explicit, it can control poweramp. As per [these instructions](https://github.com/maxmpz/powerampapi/blob/master/poweramp_api_lib/readme.md).
Ganblejs commented 2022-09-03 14:28:15 +00:00
Author
Contributor
Copy Link

From my point of view the PR is almost ready for merging now, again. Maybe just wait until @gfwilliams have chimed in re getApplicationContext().

If/when the code seems adequate I can rebase and squash the commits in preparation for merging.

From my point of view the PR is almost ready for merging now, again. Maybe just wait until @gfwilliams have chimed in re getApplicationContext(). If/when the code seems adequate I can rebase and squash the commits in preparation for merging.
Ganblejs reviewed 2022-09-03 14:39:43 +00:00
app/src/main/AndroidManifest.xml Outdated
@ -52,1 +52,4 @@
<!-- Used for starting services from all other packages with intents when targeting Android API level 30 or later on devices running Android 11 or later.
https://support.google.com/googleplay/android-developer/answer/10158779?hl=en#zippy=%2Cpermitted-uses-of-the-query-all-packages-permission%2Cexceptions -->
<uses-permission android:name="android.permission.QUERY_ALL_PACKAGES" />
Ganblejs commented 2022-09-03 14:39:43 +00:00
Author
Contributor
Copy Link

It might be hard to convince Google Play Store to allow this permission. Hopefully not. @gfwilliams

It might be hard to convince Google Play Store to allow this permission. Hopefully not. @gfwilliams
gfwilliams commented 2022-09-05 09:15:51 +00:00
Member
Copy Link

Sorry for the delay - was on holiday.

Is there a specific reason for using getApplicationContext()

Not at all, no. I think maybe I'd seen it used in an example somewhere

I've introduced (locally) to AndroidManifest

This is already in app/src/banglejs/AndroidManifest.xml since it was needed anyway for SDK 30 that the Bangle.js build has... So you may not want this in the main AndroidManifest?

Sorry for the delay - was on holiday. > Is there a specific reason for using getApplicationContext() Not at all, no. I think maybe I'd seen it used in an example somewhere > I've introduced (locally) <uses-permission android:name="android.permission.QUERY_ALL_PACKAGES" /> to AndroidManifest This is already in `app/src/banglejs/AndroidManifest.xml` since it was needed anyway for SDK 30 that the Bangle.js build has... So you may not want this in the main AndroidManifest?
Ganblejs commented 2022-09-05 17:18:44 +00:00
Author
Contributor
Copy Link

No worries what so ever! Sorry if I seem impatient, just wanted to catch you when you got back! :)

This is already in app/src/banglejs/AndroidManifest.xml since it was needed anyway for SDK 30 that the Bangle.js build has... So you may not want this in the main AndroidManifest?

Ok! Didn't catch that. Yes, I think that makes sense for now at least then. Since the main flavor doesn't target api level 30 or above there should not be an issue for people on that flavor that also want this service intents functionality. But if main flavor will target api level 30 or higher, then I would suggest adding it in to the main AndroidManifest.xml file instead.

What do @joserebelo, @vanous think? Should it be moved to the main manifest now or should it wait (or never :p)?

No worries what so ever! Sorry if I seem impatient, just wanted to catch you when you got back! :) > This is already in app/src/banglejs/AndroidManifest.xml since it was needed anyway for SDK 30 that the Bangle.js build has... So you may not want this in the main AndroidManifest? Ok! Didn't catch that. Yes, I think that makes sense for now at least then. Since the main flavor doesn't target api level 30 or above there should not be an issue for people on that flavor that also want this service intents functionality. But if main flavor will target api level 30 or higher, then I would suggest adding it in to the main AndroidManifest.xml file instead. What do @joserebelo, @vanous think? Should it be moved to the main manifest now or should it wait (or never :p)?
vanous commented 2022-09-06 09:11:08 +00:00
Owner
Copy Link

It seems like a magic trap each time - they promise some extra more functionality but behind the scenes you loose some functionality as well and this has causes new regressions lately... this comes up from time to time, unfortunately i did not link the original issues and cannot find them now... i previously compiled the issues into this: info https://codeberg.org/Freeyourgadget/Gadgetbridge/wiki/Developer-Documentation#currently-used-sdk-versions

It seems like a magic trap each time - they promise some extra more functionality but behind the scenes you loose some functionality as well and this has causes new regressions lately... this comes up from time to time, unfortunately i did not link the original issues and cannot find them now... i previously compiled the issues into this: info https://codeberg.org/Freeyourgadget/Gadgetbridge/wiki/Developer-Documentation#currently-used-sdk-versions
👍 1
Ganblejs commented 2022-09-06 11:56:36 +00:00
Author
Contributor
Copy Link

Ok! I'm not arguing for bumping the sdk version of either compile, min or target. For this PR it might be better not to.

I was just thinking that if it is bumped in the future, the service intent functionality introduced in this PR will most likely break for people who use the main build version/flavour on Android 11 and up. So it would be a preemptive fix to move QUERY_ALL_PACKAGES permission to the main build manifest now. I think this is a good idea after just having tested building a main nightly version and installed it on my device (Xiaomi MI A2 lite running Android 10) - it worked with no hiccups.

On the other hand, applying the 'don't fix what ain't broken' mindset might be better. And then move the permission in to main build if/when target sdk version is bumped.

Ok! I'm not arguing for bumping the sdk version of either compile, min or target. For this PR it might be better not to. I was just thinking that if it is bumped in the future, the service intent functionality introduced in this PR will most likely break for people who use the main build version/flavour on Android 11 and up. So it would be a preemptive fix to move QUERY_ALL_PACKAGES permission to the main build manifest now. I think this is a good idea after just having tested building a main nightly version and installed it on my device (Xiaomi MI A2 lite running Android 10) - it worked with no hiccups. On the other hand, applying the 'don't fix what ain't broken' mindset might be better. And then move the permission in to main build if/when target sdk version is bumped.
Ganblejs force-pushed Intents from e9b70dc8d0 to f8b0dcdd0a 2022-09-07 14:12:08 +00:00 Compare
Ganblejs commented 2022-09-07 14:17:49 +00:00
Author
Contributor
Copy Link

Rebased and squashed commits.

Rebased and squashed commits.
👍 1
Ganblejs force-pushed Intents from f8b0dcdd0a to 0c16a8e448 2022-09-08 22:53:33 +00:00 Compare
Ganblejs force-pushed Intents from 0c16a8e448 to fdf113ce2d 2022-09-09 07:43:40 +00:00 Compare
Ganblejs commented 2022-09-09 07:45:20 +00:00
Author
Contributor
Copy Link

Rebased.

Rebased.
ashimokawa commented 2022-09-11 16:18:13 +00:00
Owner
Copy Link

There is another reason for not bumping the sdk version: We can never go back. That's impossible.

So if SDK 30 breaks things without workaround, it is bad.

If SDK 30 does not show any regression then we can bump.

Candidates for regressions are:

  • Background behaviour
  • Rejecting/accepting phone calls
  • "Find phone" from a gadget...

etc.

There is another reason for not bumping the sdk version: We can never go back. That's impossible. So if SDK 30 breaks things without workaround, it is bad. If SDK 30 does not show any regression then we can bump. Candidates for regressions are: - Background behaviour - Rejecting/accepting phone calls - "Find phone" from a gadget... etc.
Ganblejs commented 2022-09-11 17:12:18 +00:00
Author
Contributor
Copy Link

@ashimokawa

Yes, I understand and agree with your reasoning! :)

And just to be clear, as I said I don't argue for bumping the SDK version. This PR, as it's coded now, is not dependent on it being bumped.

QUERY_ALL_PACKAGES-permission was already present in the Bangle flavour AndroidManifest because Bangle flavour was bumped to SDK 30 earlier.

The reason I suggest instead moving the permission to the main build version now is so that if at some point in the future the main build is also bumped to SDK 30+ then the functionality in this PR remains intact for the main Gadgetbridge build version.

I have tested this PR, as it is coded now, on the Gadgetbridge main build (SDK 29) on my Xiaomi Mi A2 lite running Android 10 without problems. So it doesn't break any functionality today what I can see. It should only future-proof the service intent-feature introduced.

But with that said, I'm ok with keeping the QUERY_ALL_PACKAGES permission in the Bangle build version AndroidManifest if you think it is better? But then someone will maybe have to do a fix later on.

Thanks for chiming in! 👍

@ashimokawa Yes, I understand and agree with your reasoning! :) And just to be clear, as I said I don't argue for bumping the SDK version. This PR, as it's coded now, is not dependent on it being bumped. QUERY_ALL_PACKAGES-permission was already present in the Bangle flavour AndroidManifest because Bangle flavour was bumped to SDK 30 earlier. The reason I suggest instead moving the permission to the main build version now is so that if at some point in the future the main build is also bumped to SDK 30+ then the functionality in this PR remains intact for the main Gadgetbridge build version. I have tested this PR, as it is coded now, on the Gadgetbridge main build (SDK 29) on my Xiaomi Mi A2 lite running Android 10 without problems. So it doesn't break any functionality today what I can see. It should only future-proof the service intent-feature introduced. But with that said, I'm ok with keeping the QUERY_ALL_PACKAGES permission in the Bangle build version AndroidManifest if you think it is better? But then someone will maybe have to do a fix later on. Thanks for chiming in! 👍
Ganblejs force-pushed Intents from fdf113ce2d to 7ad60eedd6 2022-09-11 17:38:12 +00:00 Compare
Ganblejs commented 2022-09-11 17:40:02 +00:00
Author
Contributor
Copy Link

Rebased.

Rebased.
Ganblejs force-pushed Intents from 7ad60eedd6 to e70fc6526a 2022-09-11 17:59:09 +00:00 Compare
Ganblejs commented 2022-09-11 18:04:30 +00:00
Author
Contributor
Copy Link

Reverted moving QUERY_ALL_PACKAGES-permission, now back in Banglejs flavour AndroidManifest.

Reverted moving QUERY_ALL_PACKAGES-permission, now back in Banglejs flavour AndroidManifest.
ashimokawa commented 2022-09-11 18:05:35 +00:00
Owner
Copy Link

@gfwilliams

What do you think? Merge?

@gfwilliams What do you think? Merge?
gfwilliams commented 2022-09-12 08:15:30 +00:00
Member
Copy Link

What do you think? Merge?

Yes, thanks! This looks good to me!

About SDK 30: It seems that SDK 30 works ok on the Bangle.js version but then I have only tested it with Bangles so haven't really used every codepath.

However the only reason to have SDK 30 on there is because of Google's Arbitrary rule for the Play Store. So in a way, it seems there are no benefits to changing the main Gadgetbridge right now - only potential downsides of things getting broken :)

At least maybe in the future if it is useful, the Bangle.js build will have been able to have provided some advanced testing though :)

> What do you think? Merge? Yes, thanks! This looks good to me! About SDK 30: It *seems* that SDK 30 works ok on the Bangle.js version but then I have only tested it with Bangles so haven't really used every codepath. However the only reason to have SDK 30 on there is because of Google's Arbitrary rule for the Play Store. So in a way, it seems there are no benefits to changing the main Gadgetbridge right now - only potential downsides of things getting broken :) At least maybe in the future if it is useful, the Bangle.js build will have been able to have provided some advanced testing though :)
Ganblejs force-pushed Intents from e70fc6526a to c15bb08a87 2022-09-13 12:34:02 +00:00 Compare
Ganblejs commented 2022-09-13 12:34:47 +00:00
Author
Contributor
Copy Link

Rebased.

Rebased.
Ganblejs force-pushed Intents from c15bb08a87 to a2e9f78f92 2022-09-16 14:23:17 +00:00 Compare
Ganblejs commented 2022-09-16 14:23:45 +00:00
Author
Contributor
Copy Link

Rebased.

Rebased.
ashimokawa merged commit 82315b3281 into master 2022-09-19 19:52:41 +00:00
Ganblejs commented 2022-09-19 20:12:46 +00:00
Author
Contributor
Copy Link

Thanks again for all help on this!

Thanks again for all help on this!
ashimokawa commented 2022-09-27 13:59:49 +00:00
Owner
Copy Link

@Ganblejs
@gfwilliams

This will make Android < 8.0 crash

getContext().startForegroundService(in);

Did not notice when merging.

@Ganblejs @gfwilliams This will make Android < 8.0 crash ``` getContext().startForegroundService(in); ``` Did not notice when merging.
Ganblejs commented 2022-09-27 14:04:36 +00:00
Author
Contributor
Copy Link

@ashimokawa

Oh, that's not good! Sorry about that then... Do you have a hint of how I could remedy this? Is it that foreground services were introduced with android 8.0? EDIT3: So reading the documentation in EDIT2 seems to indicate this is the problem. I will try to fix it.

EDIT: I presume my change from using this.getContext().getApplicationContext(). to only getContext(). for getting the context isn't the problem?

EDIT2: Here's the documentation for startForegroundService(): https://developer.android.com/reference/android/content/Context#startForegroundService(android.content.Intent)

@ashimokawa Oh, that's not good! Sorry about that then... Do you have a hint of how I could remedy this? Is it that foreground services were introduced with android 8.0? EDIT3: So reading the documentation in EDIT2 seems to indicate this is the problem. I will try to fix it. EDIT: I presume my change from using `this.getContext().getApplicationContext().` to only `getContext().` for getting the context isn't the problem? EDIT2: Here's the documentation for startForegroundService(): https://developer.android.com/reference/android/content/Context#startForegroundService(android.content.Intent)
ashimokawa commented 2022-09-27 14:24:34 +00:00
Owner
Copy Link

grafik

![grafik](/attachments/143b0551-6d94-4fc1-9884-ef219e8d0e61)
grafik.png
147 KiB
👀 1
Ganblejs commented 2022-09-27 14:29:08 +00:00
Author
Contributor
Copy Link

@ashimokawa

I will try using startService() together with startForeground() instead when API level is below 26: https://developer.android.com/reference/android/app/Service#startForeground(int,%20android.app.Notification)

But use startForgroundService() if API level is 26 or higher. Will use if/else-statement for this.

@ashimokawa I will try using startService() together with startForeground() instead when API level is below 26: https://developer.android.com/reference/android/app/Service#startForeground(int,%20android.app.Notification) But use startForgroundService() if API level is 26 or higher. Will use if/else-statement for this.
ashimokawa commented 2022-09-27 14:29:30 +00:00
Owner
Copy Link

@Ganblejs

The link you sent says "Added in API level 26"

I would suggest using startService() when API is below 26

@Ganblejs The link you sent says "Added in API level 26" I would suggest using startService() when API is below 26
ashimokawa commented 2022-09-27 14:31:00 +00:00
Owner
Copy Link

Something like:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
	getContext().startForegroundService(in);
} else {
	getContext().startService(in);
}
Something like: ``` if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { getContext().startForegroundService(in); } else { getContext().startService(in); } ````
❤️ 1
ashimokawa commented 2022-09-27 14:34:09 +00:00
Owner
Copy Link

@Ganblejs

This happened not the first time with PRs. It is a general problem that people use more modern java language or API features than available with what we have as a minimum requirement (right now Android 5.0).

Do you use Android Studio? It would have told you. I do not like it but at least it shows you all those hidden problems.

@Ganblejs This happened not the first time with PRs. It is a general problem that people use more modern java language or API features than available with what we have as a minimum requirement (right now Android 5.0). Do you use Android Studio? It would have told you. I do not like it but at least it shows you all those hidden problems.
Ganblejs commented 2022-09-27 14:36:38 +00:00
Author
Contributor
Copy Link

@ashimokawa

Yes, sorry about that and thank you!

Yes, weirdly I do use Android Studio but I don't get that same warning you print screened? (I haven't used Android Studio before doing these two PRs re Banglejs and intents, and am new to java and android programming...) EDIT: Or do you have to enter some kind of special mode to see that warning?

@ashimokawa Yes, sorry about that and thank you! Yes, weirdly I do use Android Studio but I don't get that same warning you print screened? (I haven't used Android Studio before doing these two PRs re Banglejs and intents, and am new to java and android programming...) EDIT: Or do you have to enter some kind of special mode to see that warning?
ashimokawa commented 2022-09-27 14:39:45 +00:00
Owner
Copy Link

grafik

I don't know in the top right corner you should see this, if you are in reader mode than you should exit it.

This is BangleJSSupport, the file with the most warnings afaik :P

![grafik](/attachments/875150cd-edb0-4cef-a5ba-edf6a659afe5) I don't know in the top right corner you should see this, if you are in reader mode than you should exit it. This is BangleJSSupport, the file with the most warnings afaik :P
grafik.png
4.3 KiB
❤️ 1
Ganblejs commented 2022-09-27 14:43:55 +00:00
Author
Contributor
Copy Link

Yes I exited reader mode (which I don't remember consiously entering) and now I get the same error/warning. Thanks!

Yes I exited reader mode (which I don't remember consiously entering) and now I get the same error/warning. Thanks!
Ganblejs commented 2022-09-27 15:24:37 +00:00
Author
Contributor
Copy Link

@ashimokawa PR with fix at #2911.

@ashimokawa PR with fix at #2911.
gfwilliams commented 2022-09-28 07:45:38 +00:00
Member
Copy Link

This is BangleJSSupport, the file with the most warnings afaik :P

Argh, sorry, that's not good. When I get a bit of time I'll see what I can do about sorting out some of those

> This is BangleJSSupport, the file with the most warnings afaik :P Argh, sorry, that's not good. When I get a bit of time I'll see what I can do about sorting out some of those
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
device mi band 7

Mi Band 7

  
activity post processing

activity data processing, import, export

  
activity/health

   
Android 12

   
Android 13

   
android integrations

   
architecture

  
Bangle.js

   
bug

   
changes requested

the provided PR needs some modifications

  
charts

   
details not provided

  
developer documentation

Information for contributors

  
device amazfit band 5

Amazfit Band 5

  
device amazfit bip

   
device amazfit cor

   
device Casio

   
device fossil

   
device garmin

   
device gtr 2e

   
device gts 2 mini

   
device h30

   
device hplus

   
device huami

   
device Huawei

Huawei honor

  
device liveview

   
device mi band

   
device mi band 2

   
device mi band 3

   
device mi band 4

   
device mi band 5

Mi Smart Band 5

  
device mi band 6

Mi Smart Band 6

  
device no.1 f1

   
device pace

   
device pebble

   
device pebble 2

   
device pinetime infinitime

   
device request

   
device sony

Sony

  
device support

   
device watch 9

   
device xiaomi

   
discussion

   
documentation

   
duplicate

   
enhancement

   
feature request

   
Gadgetbridge

Gadgetbridge

  
good first issue

good entry level issue for new contributors

  
help wanted

   
i am developing my own app can you help

no, sorry

  
icebox

An icebox for abandoned PRs. Feel free to pick it up, update and make a new PR

  
intent api

   
internationalisation

metric/imperial/celsius/fahrentheit...

  
invalid

   
needs work

   
network companion app

ideas pool for a network enabled companion app

  
new device

   
no feedback

   
not a bug

   
notifications

ideas and improvements for notifications

  
one of the 1000 issues about disconnection

   
pairing/connecting

   
potentially fixed / confirm and close

   
question

   
research

   
security

   
seems abandoned

PR seems to abandoned

  
Solved, waiting for F-Droid release

   
suggest to close

probably should/could close

  
task

   
user interface / UX

   
wear os

   
weather

   
wontfix

   
Zepp OS
No Label
device mi band 7
activity post processing
activity/health
Android 12
Android 13
android integrations
architecture
Bangle.js
bug
changes requested
charts
details not provided
developer documentation
device amazfit band 5
device amazfit bip
device amazfit cor
device Casio
device fossil
device garmin
device gtr 2e
device gts 2 mini
device h30
device hplus
device huami
device Huawei
device liveview
device mi band
device mi band 2
device mi band 3
device mi band 4
device mi band 5
device mi band 6
device no.1 f1
device pace
device pebble
device pebble 2
device pinetime infinitime
device request
device sony
device support
device watch 9
device xiaomi
discussion
documentation
duplicate
enhancement
feature request
Gadgetbridge
good first issue
help wanted
i am developing my own app can you help
icebox
intent api
internationalisation
invalid
needs work
network companion app
new device
no feedback
not a bug
notifications
one of the 1000 issues about disconnection
pairing/connecting
potentially fixed / confirm and close
question
research
security
seems abandoned
Solved, waiting for F-Droid release
suggest to close
task
user interface / UX
wear os
weather
wontfix
Zepp OS
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Freeyourgadget/Gadgetbridge#2826
No description provided.
Delete Branch "Ganblejs/Gadgetbridge:Intents"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?