AltBeacon / android-beacon-library

Allows Android apps to interact with BLE beacons
Apache License 2.0
2.84k stars 836 forks source link

proposal: make JobService's persist option configuable #1055

Closed hotchemi closed 2 years ago

hotchemi commented 3 years ago

related: https://github.com/AltBeacon/android-beacon-library/issues/622

Summary

There're some cases that we don't need to make the job persisted and RECEIVE_BOOT_COMPLETED permission(Not sure how many but at least in our use case it is). This PR adds jobPersistedEnabled option as metadata for BeaconService as follows(defaults to true and it keeps backwards compatibility):

   <service android:name="org.altbeacon.beacon.service.BeaconService"
             tools:node="replace">
        <meta-data android:name="jobPersistedEnabled" android:value="false"/>
    </service>

~With this change, the flag would be false by default and you need to manually add the option to enable the persistent option.~ ~I know breaking backwards compatibility should be avoided and would like to discuss it if you like the idea ๐Ÿ™‡~

Thx in advance!

davidgyoung commented 3 years ago

I understand that not all apps need background detections, but many do. The library is designed to make it as easy as possible to allow that.

Rather than changing the default library behavior and potentially breaking thousands of existing apps that might upgrade the library, why not simply override the definition in your appโ€™s manifest with tools:node=โ€œreplaceโ€?

For other users who want to eliminate that permission, perhaps a documentation page with instructions would suffice.

hotchemi commented 3 years ago

Totally agree with you. Let me update ๐Ÿ™

hotchemi commented 3 years ago

Updated. c49f65825a8dabe3c641ca20001ab98358ac8480...05dadd5df75885b46785bd0e0b59ef34072479c5

hotchemi commented 3 years ago

Could you take a look when you have time? ๐Ÿ™

hotchemi commented 2 years ago

Hi~any update? ๐Ÿ™‡

davidgyoung commented 2 years ago

Hi, @hotchemi. Apologies, I misunderstood that a key part of this PR is to allow configuration in the manifest that the job is not persisted. Because I did not understand this, I thought my last suggestion had resolved the issue for you.

Is there a reason that you need this configured in the manifest? It seems it would be much simpler to have a method to configure this, say:

beaconManager.setJobPersistenceEnabled(false)

Then just set this once in your code if you do not want the default persisted jobs.

Ultimately the library is going to be moving to a new configuration API that is exclusively programmatic (not declared in the manifest), so this might be a better fit.

I think if we make this a method in BeaconManager, this change will only be about 10 lines of code.

Thoughts?

hotchemi commented 2 years ago

Ultimately the library is going to be moving to a new configuration API that is exclusively programmatic (not declared in the manifest), so this might be a better fit.

@davidgyoung Thx for the feedback!

Is there a reason that you need this configured in the manifest?

This is just to keep API consistency with longScanForcingEnabled and I don't have a strong opinion around here๐Ÿ™‚ But presumably we should follow after you accomplish migration to the new programmatic API?

We're using the forked version with this change and it's kinda not urgent now, it's also fine closing the PR and waiting for your work๐Ÿ‘

davidgyoung commented 2 years ago

@hotchemi please see the ScheduledJobScanStrategy section here. Will this kind of API work for you?

hotchemi commented 2 years ago

@davidgyoung Thx, I checked the doc and scheduledJobScanStrategy.jobPersistenceEnabled = true looks good to me ๐Ÿ‘

hotchemi commented 2 years ago

Then let me close the PR. thx!