Skip to content

Symbol exp var #61471

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Symbol exp var #61471

wants to merge 13 commits into from

Conversation

roya0045
Copy link
Contributor

Description

Here is an updated version of the previously proposed changes, this new version includes a few needed changes that are detailed below.

QgsLayerTReeModelLegendNode

  • changes in QgsSymbolLegendNode::evaluateLabel

    Minor change in evaluation

  • changes in QgsSymbolLegendNode::createSymbolScope

    Added some safety, retreival of stored layer style in cases where the style if the layer in the map is not the same as in the app and must be retreived.

    Added the "legend_item_expression" variable that returns a string that represent the rendering condition.

QgsSymbolLayerUtils

These changes were needed as overriding temporarily the symbology of the vector layer in order to retreive the symbol was more expensive than to simply parse the string stored.

  • Addition of QgsSymbolLayerUtils::getStyleNodeMinMax

    Utility class to transform scale limitation into text, as done in QgsRuleBasedRenderer::legendKeyToExpression

  • Addition of QgsSymbolLayerUtils::ruleNodeExpression

    Recursive function to collect all relevant rules to properly translate the rendering condition into a standalone expression, as done in QgsRuleBasedRenderer::legendKeyToExpression

  • Addition of QgsSymbolLayerUtils::legendKeyToExpression

    Utility class that takes a style string and parse it to obtain an expression for the provided symbol key. Supports existing vector renderer as of writing this.

QgsExpressionFunction

  • changes in fcnAggregateGeneric

Currently the aggregate results are cached using attributes from the expression, the layer used and the feature evaluate. In the case of the layout the feature of the context can be the atlas feature, but when it comes to symbols, the current key do not help to evaluate the result using the elements present in the layout. Additionnaly in expressions some variable can be used ( map_extent, legend_item_expression, symbol_label). Those variable name will be used to create the cache, regardless if their content changed. The proposed changes evaluate all variable into a string to help distinguish between the results.

When using in a vector layer this is not a problem as the feature id will change, but in the case where the layer id will not change and the feature id ( being the atlas feature) will not change, which caused the first evaluated result to be the same for each symbol instead of evaluating it once per symbol.

This approach might seem heavy handed with expanding geometry as wkt, but this seemed to be the best way to avoid fetching the wrong cached results

Adds:
- Expression variable to represent rendering rule of a symbol
- Parsin of style string to retreive rendering rule of a symbol
- Use evaluated expression variable in aggregate caching to avoid unwanted retreival
@github-actions github-actions bot added this to the 3.44.0 milestone Apr 14, 2025
Copy link
Contributor

github-actions bot commented Apr 14, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit e92d77c)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit e92d77c)

Copy link
Contributor

github-actions bot commented Apr 19, 2025

Tests failed for Qt 5

One or more tests failed using the build from commit 0645c1c

composer_legend_theme_expression_scope

composer_legend_theme_expression_scope

Test failed at testLegendExpressionWithStyles at tests/src/python/test_qgslayoutlegend.py:1316

Rendered image did not match tests/testdata/control_images/composer_legend/expected_composer_legend_theme_expression_scope/expected_composer_legend_theme_expression_scope.png (found 2636 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.

Copy link
Contributor

Tests failed for Qt 6

One or more tests failed using the build from commit b99f766

composer_legend_theme_expression_scope

composer_legend_theme_expression_scope

Test failed at testLegendExpressionWithStyles at tests/src/python/test_qgslayoutlegend.py:1316

Rendered image did not match tests/testdata/control_images/composer_legend/expected_composer_legend_theme_expression_scope/expected_composer_legend_theme_expression_scope.png (found 58383 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.

@roya0045
Copy link
Contributor Author

Unsure why tests are erroring with "too msny arguments" on the new sip function.

Copy link
Contributor

github-actions bot commented May 6, 2025

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label May 6, 2025
@roya0045
Copy link
Contributor Author

roya0045 commented May 6, 2025

Awaiting feedback/merge.

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label May 6, 2025
Copy link
Contributor

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label May 21, 2025
@roya0045
Copy link
Contributor Author

Unstale

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label May 21, 2025
Copy link
Contributor

github-actions bot commented Jun 5, 2025

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 5, 2025
@roya0045
Copy link
Contributor Author

roya0045 commented Jun 5, 2025

awaitng review

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 5, 2025
Copy link
Contributor

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 20, 2025
@roya0045
Copy link
Contributor Author

unstale

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 20, 2025
Copy link
Contributor

github-actions bot commented Jul 5, 2025

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 5, 2025
@roya0045
Copy link
Contributor Author

roya0045 commented Jul 5, 2025

unstale

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 5, 2025
Copy link
Contributor

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 20, 2025
@roya0045
Copy link
Contributor Author

unchanged

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 21, 2025
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.

1 participant