-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 drag and drop for stacked diagrams (follow-up) #60600
base: master
Are you sure you want to change the base?
Fix drag and drop for stacked diagrams (follow-up) #60600
Conversation
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
f5d8202
to
1940074
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…s (items don't have children so far), because of that, we can drop at any index as well
…ee view when dropped 'inside' an item
…s dropping handling
… ease drag'n'drop, as well as sorting the eventually multiple dragged subDiagrams to get a clean drop. Additionally, fixes previous copying of items.
1940074
to
ec63a27
Compare
(Rebased) |
@@ -1505,6 +1505,7 @@ | |||
], | |||
"QgsStackedDiagramProperties": [ | |||
"apply()", | |||
"rowsMoved()", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add documentation instead of expanding the missing doc list?
doc.appendChild( rootElem ); | ||
stream << doc.toString( -1 ); | ||
} | ||
stream << index.row(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal, we're abusing mimeData to pass on a simple row index integer, instead of copying the actual diagram. This means we won't be able to do stuff like right-click -> copy diagram to then paste as new diagram, etc.
What's the rational behind this move?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, I'll go back to the proper mimeData content while keeping the fixes this PR introduces.
BTW, I took graduated and classification symbology for vector layers, as well as classification from point clouds as a basis. They seem to follow another approach with respect to copy&paste, but I agree we should keep the actual diagram's XML instead, thanks.
Follow-up #60278.
This PR adjusts issues and improves consistency with drag and drop behavior in other parts of QGIS. See #60596 for screencasts of the buggy behavior in the current master.
Namely, this PR:
Replaces the drop indicator, switching from a box to a horizontal line (which expands to the whole row). The rationale is that stacked diagrams don't handle (yet) a parent-child hierarchy (i.e., one cannot have nested stacked diagrams in the GUI), but act instead as a flat list. This is consistent with drag and drop for other flat lists in QGIS (e.g., classification symbol lists or graduated symbol lists).
Fixes an issue that moves the dropped item to the end of the list (i.e., an append operation), even if the desired position was, e.g., between other items.
Fixes an issue that makes it possible to obtain copies of the dragged object.
Enables moving multiple items and positioning them in the expected way (e.g., if a diagram A is below a diagram B before the drag, it should continue being below B once dropped).
Minor fixes: Allow dropping items below all other items (i.e., in the blank area), as well as in the whole row span (i.e., also for columns > 0).
Screencast of the result:
dragndrop_stackeddiagrams.mp4
Fix #60596