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] Introduce stacked diagrams #58568

Merged
merged 30 commits into from
Sep 27, 2024
Merged

Conversation

gacarrillor
Copy link
Member

@gacarrillor gacarrillor commented Sep 4, 2024

This PR extends the QGIS diagram module allowing it to create and display stacked diagrams, i.e., diagrams located side by side. We've also taken the opportunity to move the diagram settings from a modal dialog to the Styling dock widget.

Stacked diagrams can be configured and displayed horizontally or vertically. The spacing between the so called subdiagrams can also be configured.

Stacked diagrams allow users to create complex diagrams like population pyramids.

Stacked diagrams can show mixed subdiagram types (e.g., a pie chart alongside a histogram) and users can add as many subdiagrams as they want. Moreover, subdiagrams can use different renderer types: e.g., one subdiagram could use a single category renderer, whereas the other could use a linearly interpolated one.

Subdiagrams can set their own size and their own legend settings (e.g., one subdiagram could show its categories in the legend, whereas another one could hide its own categories).

Subdiagram ordering is given by item ordering in the Stacked Diagram page's list. In the future, users could re-arrange subdiagram ordering by dragging and dropping subdiagrams (left as follow-up).

Note there are some shared settings like placement settings (e.g., over point), that the stacked diagram will take from its first subdiagram. Thus, other subdiagrams won't be able to set or change those specific settings, unless they become the first subdiagram in the stacked diagram. Therefore, when editing a subdiagram that is not located in the first place, we show labels to let users know why they don't see the expected widgets (e.g., placement ones) and where they can find them (i.e., in the first subdiagram).

GUI Stacked Diagrams

vokoscreenNG-2024-09-18_00-30-27.mp4

GUI Single Diagram

Before / After (click to see it in large size)
image

image

Other samples
(Note the corresponding point feature's geometry)

expected_stackedhistograms

expected_stackedpieshorizontalwithspacing

image

image

Mixed renderer type (pies using single category and histograms using linearly interpolated one):
expected_stackedpiehistogram

Nested stacked diagrams (just because we can! Not exposed through GUI, but possible in core and showcased via unit test):
expected_stackeddiagramsnested


Includes tests.

Additional fixes:


Funded by Landesamt für Vermessung und Geoinformation (LVG) Vorarlberg in collaboration with the QGIS user group Switzerland.

@github-actions github-actions bot added this to the 3.40.0 milestone Sep 4, 2024
@gacarrillor gacarrillor added Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. Diagrams labels Sep 4, 2024
@qgis-bot
Copy link
Collaborator

qgis-bot commented Sep 4, 2024

@gacarrillor
This pull request has been tagged as requiring documentation.

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.
Also, any commit having [needs-doc] or [Needs Documentation] in will see its message pushed to the issue, so please be as verbose as you can.

Thank you!

@nyalldawson
Copy link
Collaborator

Nice work @gacarrillor !

I don't think the use of tabs is appropriate here for configuring sub diagrams. I'd rather see the UI modeled off our existing rule based labeling approach, which IMO is a better fit for this kind of configuration.

@nyalldawson
Copy link
Collaborator

I'd also suggest that the "stacked diagram" option should appear in the topmost combo box alongside the actual diagram types (eg alongside Histogram etc). Then there's no added GUI complexity when using the common single diagram type, and selecting "stacked diagrams" would expose the list of subdiagrams in the panel below that combo.

Copy link

github-actions bot commented Sep 4, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit b8f9760)

@andreasneumann

This comment was marked as outdated.

@andreasneumann
Copy link
Member

Looking forward to use this in the future - a really nice extension to the diagram system!

@Gustry Gustry added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Sep 4, 2024
@qgis-bot
Copy link
Collaborator

qgis-bot commented Sep 4, 2024

@gacarrillor

This pull request has been tagged for the changelog.

  • The description will be harvested so please provide a "nearly-ready" text for the final changelog
  • If possible, add a nice illustration of the feature. Only the first one in the description will be harvested (GIF accepted as well)
  • If you can, it's better to give credits to your sponsor, see below for different formats.

You can edit the description.

Format available for credits
  • Funded by NAME
  • Funded by URL
  • Funded by NAME URL
  • Sponsored by NAME
  • Sponsored by URL
  • Sponsored by NAME URL

Thank you!

@gacarrillor

This comment was marked as outdated.

@gacarrillor
Copy link
Member Author

Thank you all for the feedback!
I'm excited about this feature!

@andreasneumann

This comment was marked as outdated.

@m-kuhn
Copy link
Member

m-kuhn commented Sep 11, 2024

@gacarrillor is there anything that keeps this in "draft"? Feature freeze is in two days and this would be a fantastic feature to ship with the next LTR, so I wonder if we have any chance to see this there.

@gacarrillor
Copy link
Member Author

@gacarrillor is there anything that keeps this in "draft"? Feature freeze is in two days and this would be a fantastic feature to ship with the next LTR, so I wonder if we have any chance to see this there.

Thanks for pointing this out. I've just removed the draft mark.
I'm currently addressing the GUI/UX suggestions made by @nyalldawson, which are really convenient here.
Looking forward to seeing this landing in QGIS 3.40.

@gacarrillor gacarrillor marked this pull request as ready for review September 11, 2024 13:46
@nyalldawson nyalldawson added the Freeze Exempt Feature Freeze exemption granted label Sep 13, 2024
@gacarrillor
Copy link
Member Author

Thank you @nyalldawson for the exemption.
While addressing your review, we felt we had a great opportunity to finally move the diagram dialog to the Style dock widget, and we're really excited about bringing this to QGIS (we're doing our best). I hope we can push by mid next week.

Preview:
image

…d of a list when docked; harmonize stacked diagram configuration with rule-based labeling; allow QgsDiagramProperties to sync to renderers and diagram layer settings, in addition to syncing to layers; when editing a subdiagram of a stacked diagram, only show widgets for diagram layer settings if the subdiagram is the first one, for the rest, hide those widgets and show a note informing users; make sure stacked diagrams handle enabled and disabled subdiagrams (i.e., don't take into account disabled subdiagrams) and add a test for it; switching from single to stacked diagram: take the single diagram definition as the first stacked diagram; fix qgis#58782 (calling twice the apply method for label rendering)
@gacarrillor
Copy link
Member Author

gacarrillor commented Sep 18, 2024

Known issues to be addressed in upcoming commits:

  • Scale dependent visibility on stacked diagrams.
  • Remove a couple of horizontal scroll bars in dock mode.
  • Replace remaining dialogs in dock widget mode.
  • GUI/UX bring back some default values/default selections lost with recent changes.

Most-likely for a follow-up:

  • Drag and drop for diagrams.
  • Copy/paste of diagrams.
  • Passing from stacked to single diagram: take first stacked diagram as the single one.

@nyalldawson, there are still some details to be solved, but we've almost finished addressing your suggestions, thanks!

…box to remove horizontal scroll bar in panel; bring back default values that had been lost in recent changes
Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

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

I looked at the core part. I will do the gui tomorrow!
Looks good so far!

src/core/diagram/qgsstackedbardiagram.h Outdated Show resolved Hide resolved
src/core/diagram/qgsstackeddiagram.cpp Show resolved Hide resolved
src/core/qgsdiagramrenderer.cpp Show resolved Hide resolved
src/core/qgsdiagramrenderer.cpp Show resolved Hide resolved
src/core/qgsdiagramrenderer.cpp Show resolved Hide resolved
src/core/qgsdiagramrenderer.cpp Outdated Show resolved Hide resolved
src/core/qgsdiagramrenderer.cpp Show resolved Hide resolved
src/core/qgsdiagramrenderer.cpp Outdated Show resolved Hide resolved
Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

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

Not much more comments regarding the GUI, good job!

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

@DelazJ, @nirvn, I'd love your feedback on the GUI/UX part if you can find some time to check it.

@3nids 3nids merged commit ca3b40c into qgis:master Sep 27, 2024
30 checks passed
@qgis-bot
Copy link
Collaborator

@gacarrillor
A documentation ticket has been opened at qgis/QGIS-Documentation#9277
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

@gacarrillor
Copy link
Member Author

Thanks a lot @3nids for your insightful review!

@gacarrillor gacarrillor deleted the stacked_diagrams branch September 27, 2024 17:13
@gacarrillor gacarrillor restored the stacked_diagrams branch September 30, 2024 13:57
@gacarrillor gacarrillor deleted the stacked_diagrams branch September 30, 2024 15:00
@zacharlie zacharlie added ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Oct 13, 2024
{
if ( renderer )
{
mDiagramRenderers.append( renderer );
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gacarrillor What is supposed to happen with ownership here? Right now mDiagramRenderers is never cleaned up, so these are all leaked. There's also no SIP_TRANSFER annotation on the function declaration, so you'll get a crash if you call this method from python.

There's also a (very bad) ownership issue with clone() here, because that calls a default copy constructor which just directly copies mDiagramRenderers to the clone, even though all the pointers in that container belong to the original instance.

Can you please address this ASAP? We are closing in on release and this API is broken in its current state...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have a look at it, thanks a lot for pointing this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChangelogHarvested This PR description has been harvested in the Changelog already. Diagrams Feature Freeze Exempt Feature Freeze exemption granted Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo.
Projects
None yet
8 participants