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

[fix] Improve ownership handling and documentation for Stacked Diagrams #59123

Merged
merged 9 commits into from
Oct 18, 2024

Conversation

gacarrillor
Copy link
Member

@gacarrillor gacarrillor commented Oct 17, 2024

Followup #58568 (probably the last one in this series)

Improves ownership handling of sub renderers in QgsStackedDiagramRenderer and QgsStackedDiagramPropertiesModel.

Adds documentation on ownership transfer when receiving or returning sub renderers.

Other adjustments:

  • Replaces subDiagram by subDiagramRenderer when we actually refer to renderers.
  • Avoids auto for non-complex types.
  • Passing from stacked to single diagram: take first stacked diagram as the basis for the new single one.
  • Stacked diagram list in GUI: Match Diagram type column to names from the main diagram combobox. This helps us to better differentiate between a stacked bars diagram and a stacked diagram (using the diagram name alone was a bit confusing: e.g., Stacked).

@github-actions github-actions bot added this to the 3.40.0 milestone Oct 17, 2024
@gacarrillor gacarrillor added API API improvement only, no visible user interface changes Diagrams labels Oct 17, 2024
Copy link

github-actions bot commented Oct 17, 2024

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

src/gui/vector/qgsstackeddiagramproperties.cpp Outdated Show resolved Hide resolved
@gacarrillor
Copy link
Member Author

@nyalldawson, do you know how to get rid of the error in code layout?

ERROR: Issues in 'warning' category found:
/home/runner/work/QGIS/QGIS/src/core/qgsdiagramrenderer.cpp:867,warning,missingMemberCopy,Member variable 'QgsStackedDiagramRenderer::mDiagramRenderers' is not assigned in the copy constructor. Should it be copied?

@nyalldawson
Copy link
Collaborator

@gacarrillor

do you know how to get rid of the error in code layout?

Add an explicit empty constructor for mDiagramRenderers -- that indicates that you're intentionally NOT copying that field

@gacarrillor gacarrillor marked this pull request as ready for review October 18, 2024 01:19
…st one (if it exists) in the stacked diagram as a basis for the new single one

Before this commit, the stacked diagram properties were taken as a basis, losing valuable diagram settings in the process.
Note this reverts commit 2f9a0758b309ef162d6390b7411cba71af3375be
…from the main diagram combobox

This helps us to better differentiate between a stacked bars diagram and a stacked diagram (using the diagram name alone was a bit confusing: e.g., Stacked).
@gacarrillor
Copy link
Member Author

@nyalldawson,
Sorry, I added two last commits to sort of close this followup series.
Thanks for your review!

@nyalldawson nyalldawson merged commit 03aaf41 into qgis:master Oct 18, 2024
29 checks passed
@gacarrillor gacarrillor deleted the fix_leaks_stacked_diagrams branch October 19, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes Diagrams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants