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

support for Qt 6 builds #102

Merged
merged 8 commits into from
Oct 8, 2024
Merged

support for Qt 6 builds #102

merged 8 commits into from
Oct 8, 2024

Conversation

3nids
Copy link
Member

@3nids 3nids commented Oct 7, 2024

This makes the plugin compatible with Qt6 builds.

QGIS minimum version is raised to 3.34, the config dialogs is shown only for 3.40+.

Another approach would be to set the new minimum version to 3.40 (so we would leave the old version for 3.34-3.38), no strong opinion here.

@3nids 3nids requested a review from gacarrillor October 7, 2024 12:46
@3nids
Copy link
Member Author

3nids commented Oct 7, 2024

(The test is failing because the new API from recent commits has not yet reached the docker image)

@3nids 3nids changed the title update to new settings API support for Qt 6 builds Oct 7, 2024
Copy link
Contributor

@gacarrillor gacarrillor left a comment

Choose a reason for hiding this comment

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

There are some things going on that I cannot completely grasp (not enough experience handling settings from QGIS), but in general it looks good to me. 👍🏽

I'll test in GUI and let you know if I find anything strange.

class Settings(SettingManager):
def __init__(self):
SettingManager.__init__(self, pluginName)
class Settings(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that objectis not necessary for Python 3. It was a Python 2 thing.

@gacarrillor
Copy link
Contributor

Another approach would be to set the new minimum version to 3.40 (so we would leave the old version for 3.34-3.38), no strong opinion here.

This one looks a bit better to me, but I guess there are pros and cons for both options.
In February 2025, 3.40 will be LTR anyways.

@gacarrillor
Copy link
Contributor

For reference, followup #92

gacarrillor

This comment was marked as resolved.

@3nids
Copy link
Member Author

3nids commented Oct 8, 2024

Thanks a lot for the review.

There's one scenario I'm a bit worried about raising to 3.40.
Let's say someone has both versions used with the same profile: you upgrade Swiss Locator when you open 3.40, it becomes unusable when you open 3.34.

Another point to take into account is how important it is to push new releases to 3.34:

  • with the min 3.34 approach, it's included
  • with the min 3.40 approach, we need to have a separate branch

@3nids
Copy link
Member Author

3nids commented Oct 8, 2024

let's bump to 3.40 and we can revert to 3.34 if needed later on!

@3nids 3nids merged commit d1b4ca9 into master Oct 8, 2024
1 of 2 checks passed
@3nids 3nids deleted the settings-new-api branch October 8, 2024 06:56
@gacarrillor
Copy link
Contributor

Thanks a lot for the review.

There's one scenario I'm a bit worried about raising to 3.40. Let's say someone has both versions used with the same profile: you upgrade Swiss Locator when you open 3.40, it becomes unusable when you open 3.34.

Another point to take into account is how important it is to push new releases to 3.34:

* with the min 3.34 approach, it's included

* with the min 3.40 approach, we need to have a separate branch

Yeah, good points. You know more about pros and cons with respect to the user base, let's hope it works with 3.40 as min and if not we need to be ready to revert as you said. 👍🏽

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.

2 participants