-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
GUI for dynamic SVGs #40892
GUI for dynamic SVGs #40892
Conversation
Completely cool. I'll review tomorrow. |
UI thing: can you flip the +/- buttons to sit on the bottom right? That's where we usually place them in widgets like this |
@nyalldawson here you go: |
@3nids A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged. Please update the description (not the comments) with helpful description and screenshot to help the work from documentors. Thank you! |
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.
Code looks good.
I think the UI/UX here would greatly benefit from auto-discovery of such parameters in the source SVG file. If no dynamic parameters are found (a simple regular expression match would probably work here), disable the group box. If dynamic parameters are found, enable the group box and auto fill the tree widget with discovered parameters.
In fact, we could get rid of the [+] and [-] button interface altogether that way.
Without auto-discovery, my fear is that this super useful feature will be underutilized. It can be addressed in a follow up PR.
{ | ||
const QModelIndexList selectedRows = mParametersTreeView->selectionModel()->selectedRows(); | ||
if ( selectedRows.count() > 0 ) | ||
mParametersModel->removeParameter( selectedRows.at( 0 ) ); |
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.
Might be worth tweaking the code to allow for more than one parameter to be removed per button click.
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.
done in 1562a49
@@ -2839,6 +2851,15 @@ void QgsSVGFillSymbolLayerWidget::setFile( const QString &name ) | |||
emit changed(); | |||
} | |||
|
|||
void QgsSVGFillSymbolLayerWidget::setSvgParameters( const QMap<QString, QgsProperty> ¶meters ) | |||
{ | |||
// TODO mLayer->setParameters(parameters); |
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.
Still TODO?
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.
I will address this in a following PR to enable the params for SVG fills.
it's not that easy. Consider the SVG file path is data defined. In such scenario it's difficult/impossible to inspect all possible files.
I agree, this would be a very useful follow up. |
@3nids |
This adds the GUI for settings the parameters of dynamic SVGs in the symbology:
core part has been done in #40596
implements qgis/QGIS-Enhancement-Proposals#199