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

fix: QT Positioning #285

Merged
merged 2 commits into from
Jun 19, 2024
Merged

fix: QT Positioning #285

merged 2 commits into from
Jun 19, 2024

Conversation

ItachiSan
Copy link
Contributor

Rather than manually fetching the position, have our background object receive the various updates of the location and magically update the config.

This is because QT needs its loop running in order to have the location working.

Fixes #274

@ItachiSan
Copy link
Contributor Author

TODO: Add the magic part for updating the config.
From my debugging, the configuration reads the value but we cannot set it.
Need input from @oskarsh on how he wants to approach this 😄

"""Track the position provided by the service."""
self._lastPosition = position

# TODO: Here trigger a refresh.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you still figuring that out? If so, please mark this PR as a draft

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, just read the comment in the PR.

The location in the config file is never changed automatically, it's always the user-defined value. If the location is automatically determined, we use the functions directly instead of the value in the file. If the detection fails, we use the user-defined value as a fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe a good thing would be to update the value in the config too.
At least, that is what I would expect if I select the "auto" option...
However, the choice is yours and if you say "no", then the code is complete and I just need to remove the comment 😉

@oskarsh
Copy link
Owner

oskarsh commented Apr 3, 2024

I think updating the config automatically is the best approach here.

@l0drex
Copy link
Collaborator

l0drex commented Apr 3, 2024

If we update the config, then the user-defined values will be overwritten. If the position detection fails, the coordinates might be nonsense (f.e. 0, 0). As we don't know if the user is at 0, 0 or the detection failed, I would prefer if we don't update the config values. Especially since the config values act as a fallback.
Also, the detected position might be inaccurate: If the device has no GPS, then the position is guessed via the IP-address, which in my case is a few hundreds kilometers away. When switching back to the manual position, I expect to see my previously-defined manual position in the UI.

We could add a detected-position entry to the file and use that as a cache, but I don't think that is particularly useful - the position is read two times per day automatically and every time the UI is opened, which shouldn't be that often.

@l0drex l0drex changed the base branch from master to beta April 3, 2024 10:36
@ItachiSan
Copy link
Contributor Author

What should I do then? 😄

@oskarsh
Copy link
Owner

oskarsh commented Apr 9, 2024

Go with the recommendation of @l0drex then. He is more in the topic and it defintely makes sense.

@ItachiSan
Copy link
Contributor Author

Hi everyone, sorry for the silence. Was busy with life and stuff :)
It seems to me that no config update is the way, so that we can keep the user defined value safely.
I will thus remove the comments, making the PR ready 😉

@ItachiSan
Copy link
Contributor Author

Well, life has been even busier!
Will push the code in a few minutes. 😉

Rather than manually fetching the position, have our background object receive the various updates of the location and magically update the config.

This is because QT needs its loop running in order to have the location working.

Fixes oskarsh#274
Remove the TODO and the hackish code for updating the position.
Didn't work, plus it is better to have a safe default.
@l0drex l0drex merged commit 6249c3d into oskarsh:beta Jun 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QT6 Positioning won't work with Geoclue v2
3 participants