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.

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

Make minimum accuracy user configurable #890

Closed

Conversation

anyuta1166
Copy link
Contributor

Make minimum accuracy user configurable.
It's a long awaited feature and also fixes #868

@anyuta1166 anyuta1166 marked this pull request as draft September 7, 2020 08:33
@anyuta1166
Copy link
Contributor Author

anyuta1166 commented Sep 7, 2020

Oops, it seems that I broke geocode sensor. Location sensor works fine though.

Any ideas? I've copied the code from LocationSensorManager. What I've missed?

09-07 11:31:33.005 20427 20427 E GeocodeSM: Failed to get geocoded location
09-07 11:31:33.005 20427 20427 E GeocodeSM: kotlin.UninitializedPropertyAccessException: lateinit property integrationUseCase has not been initialized
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at io.homeassistant.companion.android.sensors.GeocodeSensorManager.getIntegrationUseCase(GeocodeSensorManager.kt:29)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at io.homeassistant.companion.android.sensors.GeocodeSensorManager$getMinimumAccuracy$1.invokeSuspend(GeocodeSensorManager.kt:57)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:274)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:84)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source:1)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source:1)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at io.homeassistant.companion.android.sensors.GeocodeSensorManager.getMinimumAccuracy(GeocodeSensorManager.kt:56)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at io.homeassistant.companion.android.sensors.GeocodeSensorManager.access$getMinimumAccuracy(GeocodeSensorManager.kt:16)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at io.homeassistant.companion.android.sensors.GeocodeSensorManager$updateGeocodedLocation$1.onSuccess(GeocodeSensorManager.kt:73)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at io.homeassistant.companion.android.sensors.GeocodeSensorManager$updateGeocodedLocation$1.onSuccess(GeocodeSensorManager.kt:16)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at com.google.android.gms.tasks.zzn.run(Unknown Source:4)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at android.os.Handler.handleCallback(Handler.java:873)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at android.os.Handler.dispatchMessage(Handler.java:99)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at android.os.Looper.loop(Looper.java:193)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at android.app.ActivityThread.main(ActivityThread.java:6923)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at java.lang.reflect.Method.invoke(Native Method)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
09-07 11:31:33.005 20427 20427 E GeocodeSM:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:870)

@anyuta1166
Copy link
Contributor Author

anyuta1166 commented Sep 7, 2020

Sorry for me being a newbie :-) Seems like I've missed to add some code. Fixed.

Seems to work fine now.

@anyuta1166 anyuta1166 marked this pull request as ready for review September 7, 2020 09:18
@skynetua
Copy link
Contributor

skynetua commented Sep 7, 2020

I thougth about this feature like setting per sensor but not like integretionUsecase settng. It is not so global to be defined there as for me. We could store setting required for particular sensor manager in the daabase and show them in sensor details fragment. In sensor manager we will be able to get it by existing sensorDao object.

@anyuta1166
Copy link
Contributor Author

anyuta1166 commented Sep 7, 2020

I thougth about this feature like setting per sensor but not like integretionUsecase settng. It is not so global to be defined there as for me.

It affects 2 sensors - location sensor and geocoded sensor. And the setting should be the same for both.

@anyuta1166
Copy link
Contributor Author

I also wonder if we should remove doubling minimum accuracy for the last attempt after making it configurable to avoid user confusion.

@dshokouhi
Copy link
Member

dshokouhi commented Sep 7, 2020

I thougth about this feature like setting per sensor but not like integretionUsecase settng. It is not so global to be defined there as for me.

It affects 2 sensors - location sensor and geocoded sensor. And the setting should be the same for both.

It does use 2 sensors but it's a little odd having it here because not all users will grant the app location permissions and if they don't we shouldn't show this option to them. I believe the logic for geocoded is not the same as location because we don't do the retry attempts in geocoded like we do for the device tracker. We also need to hide it from minimal users too.

I also wonder if we should remove doubling minimum accuracy for the last attempt after making it configurable to avoid user confusion.

I think we may want to let users keep things as the current default if they are happy with the current implementation but if a user changes this then yes we we should not double it. Not sure what the others think about this.

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Just a general comment on the input field itself, may need to discuss whats best for its behavior.

app/src/main/res/xml/preferences.xml Show resolved Hide resolved
@anyuta1166
Copy link
Contributor Author

not all users will grant the app location permissions and if they don't we shouldn't show this option to them.

Implemented this.

@anyuta1166 anyuta1166 force-pushed the configurable-accuracy branch 2 times, most recently from 194bc2d to 6ec284f Compare September 8, 2020 13:38
@anyuta1166
Copy link
Contributor Author

Rebased after #895.

@anyuta1166
Copy link
Contributor Author

Fixed a small copy-paste error. :-)

@JBassett
Copy link
Collaborator

After thinking about this for a while I think we want this to be a per location update type setting, not just a global one if we are going to make it adjustable. Take for example if you request an accurate location, I might want an accurate location vs a zone change I might be ok with less accurate.

@anyuta1166
Copy link
Contributor Author

So are you going to leave it global and unconfigurable until someone comes up with a better PR?

@JBassett
Copy link
Collaborator

Closing in favor of #964

@JBassett JBassett closed this Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some issues with geo location sensor
5 participants