Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Trigger active value not correct in some cases #48

Merged
merged 9 commits into from
Mar 6, 2024
Merged

Fix: Trigger active value not correct in some cases #48

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8109d2a
Add remember active state of sensor trigger
KCeh Feb 13, 2024
247642a
Update cache when saving Trigger DAO
KCeh Feb 13, 2024
8a5f749
Fix lint
KCeh Feb 13, 2024
23ebca4
Merge branch 'develop' into fix/trigger-init-not-working-properly-on-…
KCeh Mar 1, 2024
4464222
Merge branch 'develop' into fix/trigger-init-not-working-properly-on-…
KCeh Mar 1, 2024
3d28a7e
Merge branch 'develop' into fix/trigger-init-not-working-properly-on-…
KCeh Mar 4, 2024
4dbbe12
Remove sensor trigger
KCeh Mar 6, 2024
c0eee7b
Merge branch 'develop' into fix/trigger-init-not-working-properly-on-…
KCeh Mar 6, 2024
d4741a6
Merge branch 'develop' into fix/trigger-init-not-working-properly-on-…
KCeh Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .../src/main/kotlin/com/infinum/sentinel/data/models/memory/triggers/shared/SensorTrigger.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,16 @@ internal abstract class SensorTrigger(

abstract fun processEvent(event: SensorEvent): Boolean

private var activeHolder: Boolean? = null

override fun start() {
sensorManager = (context.getSystemService(Context.SENSOR_SERVICE) as? SensorManager)
sensorManager?.let {
registerSensor(it)
this.active = true
if (activeHolder == null) {
activeHolder = active
}
this.active = activeHolder ?: true // use remembered active state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@KCeh KCeh Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} ?: run {
this.active = false
}
Expand All @@ -34,6 +39,7 @@ internal abstract class SensorTrigger(
queue?.clear()
unregisterSensor()
sensorManager = null
activeHolder = active // remember active state
this.active = false
}

Expand Down
1 change: 1 addition & 0 deletions sentinel/src/main/kotlin/com/infinum/sentinel/domain/triggers/TriggersRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ internal class TriggersRepository(
override suspend fun save(input: TriggerParameters) =
input.entity?.let {
dao.save(it)
updateCache(it)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} ?: error("Cannot save null entity")

override fun load(input: TriggerParameters): Flow<List<TriggerEntity>> =
Expand Down
Loading