Fix: Trigger active value not correct in some cases

[KCeh]

📄 Context

Fix for the issue #46

📝 Changes

There were 2 issues:

In SensorTrigger start() trigger would always be set true.
IMO we should remember what active value was for the trigger (overridden by the user in settings or manifest). This is why I introduced activeHolder (name could be probably better).
Example of how to replicate a related bug is to turn off the trigger (in settings for example) and put app in background. Open the app from the background again. Then trigger is turned on.
I am not sure if this is an oversight or a feature 😅

Another issue is that the override value from the manifest wouldn't be loaded into memory properly on the first (clean with no cache) app start. When overrides for triggers are loaded we just invoke repository.save which saves DAO. But this doesn't update cache/in memory object (flow onEach for loaded entities or something like that is also not invoked). I just added a call to updateCache when DAO is saved. This resolves this issue and trigger's active value is properly reflected in memory.

🛠️ How to test

You can clone the repo and try out a sample app with manifest overrides for triggers.

⏱️ Next steps

More fixes for other issues 😄

[KCeh]

CI is broken
I will push lint fixes and open a new PR with Java version fix for Github action

[antunflas]

What's the whole idea of updating the active flag based on the availability of a sensor? Why don't we just leave it as is, if there is no sensor, we won't receive a change anyway 🤷‍♂️

Otherwise, by setting it to true and false in the start and stop we overwrite the user's value, which is forever lost.

Even with this change, I don't see how this does anything different than not updating the flag. If active was false, the activeHolder would be false and the active flag would stay false. If the active was true, the activeHolder would be true and the active flag would stay true. Note that activeHolder cannot be null, since if it was, on L30 it would be set to a non-null value, there is just a case of concurrency which I assume we aren't relying on here.

However, if active was true, the sensor started and then the active flag was changed to false, in that case stopping the trigger would change the active flag to false (but activeHolder would still be true), and then when started again, activeHolder won't be reset, meaning the active flag would be true out of nowhere.

I have a feeling changing the active flag in here is just wrong and probably there was an idea to set it to false in case the sensor is not available. We can still keep that part if there is a reason, but I would prefer if we remove every active toggling from the sensor itself.

[KCeh]

Not that this is code for only SensorTrigger (proximity & shake).

There is an issue with the initialization of the Sensor and that is why code this.active = true exist. Logic is a bit well, unlogical at first.
I added activeHolder logic to avoid the issue of the sensor always becoming active after start() has been invoked regardless of the user's settings.
I see your point about L30.

I will try to revert this change and see what is happening from user's perspective.

[KCeh]

So this is problematic part of the code:

override fun start() {
           ...
            this.active = true
}

This is result with code with reverted SensorTrigger.kt changes.
Imagine I am activating proximity sensor after I am bringing app from background

Record_2024-03-06-09-07-23.mp4

It is more of an annoyance than anything else

[antunflas]

My suggestion was to completely remove the flipping of the active flag. So start and stop method would look something like this. And the flag setting would be removed from other methods as well 🤷‍♂️

    override fun start() {
        sensorManager = (context.getSystemService(Context.SENSOR_SERVICE) as? SensorManager)
        sensorManager?.let {
            registerSensor(it)
        }
    }

    override fun stop() {
        queue?.clear()
        unregisterSensor()
        sensorManager = null
    }

[KCeh]

I think I tried this, but I found out that flipping of flag is necessary

[KCeh]

I guess we can remove active flipping and see if anyone reports issue 🤷‍♂️

[antunflas]

registerSensor also has the flipping to false. I'd remove every occurrence of writing to that flag from this class, otherwise the flag would be flipped incorrectly again in some cases.

[KCeh]

Well I wouldn't change that because it is fallback if sensor manager can't find that sensor

[antunflas]

Okay, that case is not that important, but what would be the downside of removing also that? The trigger would appear to be active but it wouldn't report anything as the sensor is not working.

[KCeh]

Maybe only it would be confusing to the user in Sentinel UI?
Not sure

[antunflas]

This change is crucial and probably the fix for all the problems. Good work 👏

[KCeh]

@antunflas do you think we are good to go?

[antunflas]

Let's go 🚀