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

[core] Introduce QgsProfileSourceRegistry #57311

Merged
merged 9 commits into from May 22, 2024

Conversation

gacarrillor
Copy link
Member

This will allow other classes (e.g., plugins) to register profile sources other than map layers (e.g., based on profile web services).

These profile sources are used to generate profiles with custom tools, which are then displayed in the main Elevation Profile dock widget and as layout items.

To do so, plugins should subclass QgsAbstractPluginSource and pass it to the registry via registerProfileSource():

QgsApplication.profileSourceRegistry().registerProfileSource(self.profile_source)

Plugins should unregister (most likely on their unload() method) their registered sources in this way:

QgsApplication.profileSourceRegistry().unregisterProfileSource(self.profile_source)

Funded by the QGIS user group Switzerland.

@github-actions github-actions bot added this to the 3.38.0 milestone May 1, 2024
Copy link

github-actions bot commented May 1, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 1986f12)

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Nice! Can you add some tests too please?

src/gui/elevation/qgselevationprofilecanvas.cpp Outdated Show resolved Hide resolved
src/core/layout/qgslayoutitemelevationprofile.cpp Outdated Show resolved Hide resolved
src/core/layout/qgslayoutitemelevationprofile.cpp Outdated Show resolved Hide resolved
src/core/layout/qgslayoutitemelevationprofile.cpp Outdated Show resolved Hide resolved
src/gui/elevation/qgselevationprofilecanvas.cpp Outdated Show resolved Hide resolved
src/app/elevation/qgselevationprofilewidget.cpp Outdated Show resolved Hide resolved
*
* \since QGIS 3.38
*/
class CORE_EXPORT QgsProfileSourceRegistry
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could consider making this a QObject, with signals for sources added / removed, and automatically updating elevation plots when these signals are emitted.

It's not required, but would be a nice touch!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice suggestion indeed, thanks. Unfortunately, we'll need to postpone this to an eventual followup, since it's been internally considered out of scope for this sprint.

src/core/elevation/qgsprofilesourceregistry.h Outdated Show resolved Hide resolved
src/core/elevation/qgsprofilesourceregistry.h Outdated Show resolved Hide resolved
@nyalldawson nyalldawson added the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label May 1, 2024
@gacarrillor
Copy link
Member Author

Nice! Can you add some tests too please?

For sure, and thank you for the review!

@gacarrillor
Copy link
Member Author

@nyalldawson, I've added some tests for this PR.
Please let me know if you have other tests in mind.

Copy link

github-actions bot commented May 6, 2024

Tests failed for Qt 5

One or more tests failed using the build from commit 625fdee

custom_profile

custom_profile

Test failed at test_layout_item_profile_custom_source at tests/src/python/test_qgsprofilesourceregistry.py:403

Rendered image did not match tests/testdata/control_images/expected_custom_profile/expected_custom_profile.png (found 130 pixels different)

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

@gacarrillor
Copy link
Member Author

@nyalldawson, do you happen to know how to get rid of this other error?
https://cdash.orfeo-toolbox.org/test/13336109

@nyalldawson
Copy link
Collaborator

@gacarrillor

do you happen to know how to get rid of this other error?

Maybe try explicitly cleaning up all the profile related objects in the tests before removing the profile source. Running the test through gdb might give some useful clues too!

@gacarrillor
Copy link
Member Author

Thanks a lot for the suggestions @nyalldawson and @3nids.
I'm currently attempting to reproduce the error on a local environment or to connect to the container built for tests.

@3nids
Copy link
Member

3nids commented May 17, 2024

We would like to get this merged in this cycle. There is on segfaut with Qt6 only in the tests for now. I would like to ask for a freeze exception if it's possible here. We'll try to track that down in the next couple of days.

@nyalldawson nyalldawson added the Freeze Exempt Feature Freeze exemption granted label May 17, 2024
@gacarrillor
Copy link
Member Author

@nyalldawson and @3nids, it turns out the error was about two enums in the test that were not recognized by Qt6. Namely:

Qt.NoBrush --> Qt.BrushStyle.NoBrush
Qt.NoPen --> Qt.PenStyle.NoPen

This was hiding another issue with the produced images, which required a mask file.

Thanks again for your hints.

@3nids
Copy link
Member

3nids commented May 21, 2024

@nyalldawson Is this PR ok for you as is or you want to review it once more? In other words, can we merge it?

@nyalldawson nyalldawson merged commit ec563d4 into qgis:master May 22, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Freeze Exempt Feature Freeze exemption granted Profile tool Requires Tests! Waiting on the submitter to add unit tests before eligible for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants