-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Conversation
There was a problem hiding this 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?
* | ||
* \since QGIS 3.38 | ||
*/ | ||
class CORE_EXPORT QgsProfileSourceRegistry |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
For sure, and thank you for the review! |
@nyalldawson, I've added some tests for this PR. |
Tests failed for Qt 5One or more tests failed using the build from commit 625fdee custom_profilecustom_profileTest 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. |
625fdee
to
d6ff1f5
Compare
@nyalldawson, 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! |
Thanks a lot for the suggestions @nyalldawson and @3nids. |
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. |
… map layers, e.g., based on web services
…ce is not a map layer (e.g., from web services)
79f6805
to
1986f12
Compare
@nyalldawson and @3nids, it turns out the error was about two enums in the test that were not recognized by Qt6. Namely:
This was hiding another issue with the produced images, which required a mask file. Thanks again for your hints. |
@nyalldawson Is this PR ok for you as is or you want to review it once more? In other words, can we merge it? |
@gacarrillor Hi ! I'm sure this will allow a lot of nice implementation in plugins. Do you have an example or a proof of concept to share ? |
Hi @nicogodet, sure, you can have a look at the Swiss locator plugin, namely to the profile module. Here is an unofficial dev version if you'd like to test it. Note it requires QGIS current master or QGIS v3.38 (2024-06-21): |
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 viaregisterProfileSource()
:Plugins should unregister (most likely on their
unload()
method) their registered sources in this way:Funded by the QGIS user group Switzerland.