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

Move the config-yaml to general plugin folder to avoid deletion when plugin is updated. #185

Closed
wants to merge 5 commits into from

Conversation

merydian
Copy link
Collaborator

@merydian merydian commented Oct 4, 2023

No description provided.

@merydian merydian linked an issue Oct 4, 2023 that may be closed by this pull request
@MichaelsJP
Copy link
Member

MichaelsJP commented Oct 6, 2023

What happens if we have an update for the config file, new parameters etc., and there is still an old one in the permanent location?

@merydian
Copy link
Collaborator Author

merydian commented Oct 6, 2023

Currently there will probably be a permission denied error. One way to solve this could be adding the plugin version to the config file and checking for it, overwriting the old config file when theres a new version.

… config file is old

When creating the config file used by the plugin, the plugin version is appended. This version is checked and if there's a new one, the old providers will be copied to the config file used by the plugin.
@MichaelsJP
Copy link
Member

Currently there will probably be a permission denied error. One way to solve this could be adding the plugin version to the config file and checking for it, overwriting the old config file when theres a new version.

Wouldn't that be the opposite of what this issue targets to achieve?

@merydian
Copy link
Collaborator Author

The way the current code in this PR handles it is every time the plugin is reloaded or QGis is started, in the init, there's a check that compares the plugin version with the version of the config file. If there's a difference, the old providers will be copied.
For the case that new parameters (or such) are introduced, @TheGreatRefrigerator and I concluded that a custom logic to incorporate them has to be included. I.e. we can't care for the new parameters in advance. There are warning comments in the init and the config-yaml.

Maybe this is too much trouble for this issue?

@merydian merydian requested a review from koebi October 17, 2023 09:53
@MichaelsJP
Copy link
Member

MichaelsJP commented Nov 2, 2023

Right now, we only keep the providers. Imo, that is fine how it is now. Maybe it's worth keeping an additional logic in case we modify the provider selection somehow. Old config structures shouldn't break new configs. If we're not going to change the provider logic, we could also just merge how it is now.

@merydian What do you think?

@merydian
Copy link
Collaborator Author

merydian commented Nov 2, 2023

The problem i still see is potential issues with the config-yaml being in the plugin folder. I am not certain on how we can make sure there's nothing wrong with that.

@MichaelsJP
Copy link
Member

The problem i still see is potential issues with the config-yaml being in the plugin folder. I am not certain on how we can make sure there's nothing wrong with that.

@koebi or @TheGreatRefrigerator what do you think?

@TheGreatRefrigerator
Copy link
Collaborator

Maybe we should look into moving our settings to the QGIS plugin settings:
https://docs.qgis.org/3.28/en/docs/pyqgis_developer_cookbook/settings.html?highlight=storing%20settings

We could just JSON.dumps the config object.

@merydian merydian marked this pull request as draft November 3, 2023 16:25
@koebi
Copy link
Collaborator

koebi commented Nov 6, 2023

Maybe this blog post is of relevance here?

@merydian
Copy link
Collaborator Author

As @TheGreatRefrigerator suggested this now lives here wit QgsSettings: #249

@merydian merydian closed this May 24, 2024
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.

Don't override custom provider on update
4 participants