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] Add the expression for the diagram attributes - #8682 #1039

Closed
wants to merge 2 commits into from

Conversation

sbrunner
Copy link
Contributor

I break the python API should be notified somewhere ?

I change the diagram verifications image, I think that the previous one is wrong, she indicate that some plane hasn't any Pilot ...

All comments are welcome :-)

@NathanW2
Copy link
Member

Given that we are now in 2.x releases we need to maintain a stable API if we can. To do this I think you need to add a overloaded method in C++ which one taking QgsAttributes and the other taking QgsFeature. In sip you will just have to have methods with different names but both pointing to the same C++ method. There should be some places we already do this.

@sbrunner
Copy link
Contributor Author

OK, but If I do this I will probably duplicate the code and it the expressions are use I should return an error.
Is what we want ?
Is there a tag used to deprecated a function ?
Thanks in advance for your responses :-)

@sbrunner
Copy link
Contributor Author

@Matthias-Kuhn Your probably right, I will work on that :-)

@NathanW2
Copy link
Member

OK, but If I do this I will probably duplicate the code and it the expressions are use I should return an error.

No you don't have to duplicate it. You just create a feature and set it's attributes in the QgsAttributes ones that then calls the QgsFeature one. Example (will need tweaking)

void QgsHistogramDiagram::renderDiagram( const QgsAttributes& att, QgsRenderContext& c, const QgsDiagramSettings& s, const QPointF& position )
{
   QgsFeature feature();
   feature.setAttributes(att);
   renderDiagram(feature, c, s, position);
}

All the work still gets done in the QgsFeature based one, we are just making sure we don't break the API.

@m-kuhn
Copy link
Member

m-kuhn commented Dec 19, 2013

Deprecation for SIP: /Deprecated/
Deprecation for C++: /** @deprecated Information what should be used instead*/

@jef-n
Copy link
Member

jef-n commented Dec 19, 2013

Deprecation for SIP: /Deprecated/
Deprecation for C++: Q_DECL_DEPRECATED
Deprecation note for API documentation: /** @deprecated Information what should be used instead*/

@sbrunner
Copy link
Contributor Author

Thanks for all your responses I will work on them :-)

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

Hello,

I just add the required changes, and do a rebase, ready for a second review :-)

@m-kuhn
Copy link
Member

m-kuhn commented Jan 8, 2014

Hi Stéphane,

It looks good and works fine in some quick tests.

Minor issues I noticed:

  • When I open the expression builder, there is always the currently selected column present. That's fine, but it should be quoted.

  • The expressiontest fails, does it work for you?:

    Start 4: qgis_diagramtest
    4/83 Test fix #3800 for branch release-1_7_0 #4: qgis_diagramtest ..................... Passed 3.80 sec
    Start 5: qgis_diagramexpressiontest
    5/83 Test Radims fix for raster otf of wms render issues #5: qgis_diagramexpressiontest ...........***Failed 3.59 sec

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

When I open the expression builder, there is always the currently selected column present. That's fine, but it should be quoted.

Effectively, I will fix it :-)

The expressiontest fails, does it work for you?:

   Start 4: qgis_diagramtest
   4/83 Test #4
   : qgis_diagramtest ..................... Passed 3.80 sec
   Start 5: qgis_diagramexpressiontest
   5/83 Test #5
   : qgis_diagramexpressiontest ...........***Failed 3.59 sec

Yey it's working, I will do a second checkout to be sure that all is ok !

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

Hello @Matthias-Kuhn,

I just add the missing quote, but for the test my second build succeed, but on the same host based on Ubuntu 13.10, can you have some additional information to fix it ?

Thanks :-)

@m-kuhn
Copy link
Member

m-kuhn commented Jan 8, 2014

It seems that it's a one-pixel-problem...

QDEBUG : TestQgsDiagramExpression::testPieDiagramExpression() Expected size: 3507w x 2480h
QDEBUG : TestQgsDiagramExpression::testPieDiagramExpression() Actual   size: 3507w x 2480h
QDEBUG : TestQgsDiagramExpression::testPieDiagramExpression() 1/8697360 pixels mismatched
QDEBUG : TestQgsDiagramExpression::testPieDiagramExpression() "<DartMeasurement name="Mismatch Count " type="numeric/integer">1/8697360</DartMeasurement>" 

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

Nice :-)

Can you send me (at [email protected]) your generated files:

/tmp/piediagram_expression_rendered.png
/tmp/piediagram_expression_result_diff.png

To get exactly what's wrong...

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

I just fixed my email address :-)

@m-kuhn
Copy link
Member

m-kuhn commented Jan 8, 2014

I just sent them to you. I think I heard there is a tolerance parameter, if it's just a change in the anti-aliasing alogorithm or similar.

Anyway I was wondering if an image in this resolution is required for such a test. Seems to be a bit of an overkill (That was my first test I wrote and I was really new to QGIS development when I did that...)

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

Effectively there is just one pixel who has the code #8f0f0f in place of #8e0e0e ...

I put a smaller image and I hope that's will be ok ...

@m-kuhn
Copy link
Member

m-kuhn commented Jan 8, 2014

Perfect, works here.

Would it be easy for you to make the diagramtest image smaller too?

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 8, 2014

OK, I will do it tomorrow :-)

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 9, 2014

@Matthias-Kuhn Just done :-)

@m-kuhn
Copy link
Member

m-kuhn commented Jan 9, 2014

thank you very much. passes the tests here now.

do you have an idea why the diagramtest takes is about 7 times slower than the diagramexpression test?

@m-kuhn
Copy link
Member

m-kuhn commented Jan 9, 2014

Hi Stéphane,

I don't recall the comment I wrote on your first code draft literally, but I think it was concerning the prepared execution of expressions.
Looking at the code right now, I see, that there is an expression prepared in "prepareLabelingAndDiagrams" but this (prepared) expression scopes out without being used and when rendering the feature, a new expression is generated, whenever the diagramSize() or the renderDiagram() method is called (so once per rendered feature and expression). To make use of the prepared expression, this would need to be cached inside the QgsDiagram class or a subclass of it.

And two minor things:

In the sip file, the deprecated methods are marked as pure virtual ( =0 ) while in the header it is just virtual. Not sure if it makes any difference, but would be nice to have the same there.

Can you run scripts/prepare-commit.sh before doing commits, so we have a consistent coding style in the repository.

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 9, 2014

do you have an idea why the diagramtest takes is about 7 times slower than the diagramexpression test?

No but with the new version it's much faster :-)

I don't recall the comment...

Thanks with your suggest the tests run much faster :-)

In the sip file, the deprecated methods are marked as pure virtual ( =0 ) while in the header it is just virtual. Not sure if it makes any difference, but would be nice to have the same there.

Your right I remove them :-)

Can you run scripts/prepare-commit.sh before doing commits, so we have a consistent coding style in the repository.

Thanks for the tip, I correct the related issue :-)

Finally should I stash my commits in one or to commits ?

CU and thanks for your reactivity and your good review :-)
Stéph

@m-kuhn
Copy link
Member

m-kuhn commented Jan 9, 2014

Very nice, thank you for the fast changes :-)

If you could squash them all into one single commit it would be great.

If you feel like optimizing more, you can change the lookup of the expressions into something based on IDs (QVector<QgsExpression*> instead of QMap<QString,QgsExpression*>).

I'll have a look at it again when it is squashed.

@sbrunner
Copy link
Contributor Author

sbrunner commented Jan 9, 2014

If you feel like optimizing more, you can change the lookup of the expressions into something based on IDs (QVector instead of QMap).

Why not but we should use a special index for the size and I think that's clearer with a QMap ...

Otherwise I stacked the commits :-)

@m-kuhn
Copy link
Member

m-kuhn commented Jan 10, 2014

I was thinking in terms of memory footprint (The QMap saves a copy of the cleartext expression once more, or does it make use of implicit sharing?) and lookup times (every time you use an expression, text comparisons have to take place)

But honestly, I don't know if there is a difference and how much it actually is.

I will try to get time for the review this afternoon.

@m-kuhn
Copy link
Member

m-kuhn commented Jan 10, 2014

Good stuff, Stéph, I think it is ready to merge. Just give me a short feedback concerning the .gitignore before and I will merge.

And a final question,
What happens if the user entered an invalid expression? Will there be some kind of warning?

@sbrunner
Copy link
Contributor Author

What happens if the user entered an invalid expression? Will there be some kind of warning?

The expression builder don't allow to validate an invalid expression, then the possible issue I see is when we edit the project file ..., and in this case we don't have any user message, the value is just considered to be 0 !

@m-kuhn
Copy link
Member

m-kuhn commented Jan 10, 2014

The expression could be parsed at least in the config dialog and somewhere shown an appropriate message if this fails. When evaluating the expression, it could be written to the message log, so the user gets a hint that something went wrong.

Anyway:

Merged in a0911d6 / 1d3dd1b
Thank you very much for this feature.

@m-kuhn m-kuhn closed this Jan 10, 2014
@sbrunner
Copy link
Contributor Author

Many thanks @Matthias-Kuhn :-)

@m-kuhn
Copy link
Member

m-kuhn commented Jan 13, 2014

It would be very nice if you could write a short summary and make a screenshot for the changelog:
http://changelog.linfiniti.com/qgis/version/21/

Best to send it directly to Tim by email

@sbrunner
Copy link
Contributor Author

Thanks, done :-)

@sbrunner
Copy link
Contributor Author

I don't see the commit a0911d6 in the master branch any more what's going on ?

@NathanW2
Copy link
Member

It's still there. In fact github just link to it for you.

@sbrunner
Copy link
Contributor Author

Thanks @NathanW2 for your quick response, I just mixup my Git remote, sorry...

@sbrunner sbrunner deleted the diagram-property branch January 12, 2015 10:17
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.

4 participants