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

[feature] adds diagram legend #1977

Closed
wants to merge 3 commits into from
Closed

Conversation

vmora
Copy link
Contributor

@vmora vmora commented Apr 7, 2015

prepare-commit.sh added some spacing in some otherwise unmodified lines.

@@ -94,6 +94,9 @@ class QgsDiagramSettings

void readXML( const QDomElement& elem, const QgsVectorLayer* layer );
void writeXML( QDomElement& rendererElem, QDomDocument& doc, const QgsVectorLayer* layer ) const;

//! @note caller is responsible for deletion of QStandardItems
QList< QStandardItem * > legendItems() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sure, but possibly these should be marked /Factory/ so that python will clean the created items when they go out of scope

Copy link
Member

Choose a reason for hiding this comment

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

That's correct. Also it would be useful to add notes that the method was added in 2.10

@nyalldawson
Copy link
Collaborator

Generally looks good to me, nice work! I wonder if it's worth adding an option to disable the legend for a diagram? It's also not possible to rename the legend items, which means they can't be customized in composer legends. This should be addressed as the default name looks odd given that it's always surrounded by quotation marks (or is a raw expression).

@wonder-sk
Copy link
Member

Is there any reason to use QStandardItem instead of QgsLayerTreeModelLegendNode instances? The QStandardItem instances just seems as a means to transport icon + label. I strongly recommend using directly QgsLayerTreeModelLegendNode, otherwise it will be difficult to add further functionality to the diagram legend nodes when the legend nodes API is extended.

@vmora
Copy link
Contributor Author

vmora commented Apr 8, 2015

@wonder-sk QgsRendererV2::legendSymbologyItems does not return QgsLayerTreeModelLegendNode(s) either and QgsLayerTreeModelLegendNode needs a QgsLayerTreeLayer. Should I pass that as an argument to QgsDiagramRendererV2::legendItems() ?

I started with returning a list of QSharedPtr but the python conversion won't work well in this case.

Also I tried to embed the symbol (when embeddable) and noticed that the logic in QgsLayerTreeModel won't allow that easily.

@vmora
Copy link
Contributor Author

vmora commented Apr 20, 2015

ping @wonder-sk

@vmora
Copy link
Contributor Author

vmora commented Apr 20, 2015

I believe those modifications take care of all the mentioned issues.

@nyalldawson
Copy link
Collaborator

@vmora - hmm, something's still missing which is preventing editing the item text in composer legends. @wonder-sk any ideas?

@wonder-sk
Copy link
Member

sorry for the lag, I have been busy recently. I will try to have a look this week...

@wonder-sk
Copy link
Member

@vmora It looks great - thanks for the updates.

The missing bit for composer legend will be a missing implementation of data() for RuleKeyRole. Probably QgsSimpleLegendNode (which is used for diagram legend nodes) could be extended to keep also the "rule key" string which uniquely identifies the node within legend of layer.

The PR cannot be automatically merged anymore - could you please update it? (and possibly fix the composer issue)

added editable labels in the legend configuration
added rule key to QgsSymbolV2LegendNode
@vmora
Copy link
Contributor Author

vmora commented Apr 27, 2015

@wonder-sk I did the rebase (first commit)

I also tried adding a RuleKeyRole (second commit) but it does not seem to have an effect in the composer. Do you know what I'm missing ?

@@ -206,6 +207,16 @@ QList<QgsLayerTreeModelLegendNode*> QgsDefaultVectorLayerLegend::createLayerTree
if ( nodes.count() == 1 && nodes[0]->data( Qt::EditRole ).toString().isEmpty() )
nodes[0]->setEmbeddedInParent( true );


if ( mLayer->diagramRenderer() )
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be

if ( mLayer->diagramsEnabled() )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. You're right, I avoid having the lengend appear even if the diagram is disabled. Thanks @nyalldawson

Change test for legend to diagramEnabled in map layer diagram
@nyalldawson
Copy link
Collaborator

@vmora 4a9a936 was the missing bit for the composer legend. I've squashed and pushed all this work now. Thanks again!

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.

3 participants