[ui] NodeActions : fix position on swiching status #2981
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2981 +/- ##
========================================
Coverage 79.80% 79.80%
========================================
Files 64 64
Lines 8674 8674
========================================
Hits 6922 6922
Misses 1752 1752 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Alxiice
requested review from
CedricThebault,
cbentejac,
fabiencastan and
nicolas-lambert-tc
There was a problem hiding this comment.
Pull request overview
This PR fixes a positioning bug in NodeActions that occurred when switching node status, where the action buttons would be misaligned due to height changes not triggering position updates.
Changes:
- Added
onHeightChangedhandler to trigger position updates when the NodeActions height changes - Updated all position update calls to use
Qt.callLaterfor deferred execution to handle timing issues - Added null safety check for chunks in ChunksListView to prevent index out of range errors
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| meshroom/ui/qml/Controls/NodeActions.qml | Added height change handler and converted position updates to use Qt.callLater for proper timing |
| meshroom/ui/qml/GraphEditor/ChunksListView.qml | Added null check for chunks before accessing its properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (currentIndex >= chunks.count) | ||
| if (!chunks) | ||
| currentIndex = -1 | ||
| else if (chunks && currentIndex >= chunks.count) |
There was a problem hiding this comment.
The condition chunks && is redundant here since it's already in the else-if branch that only executes when chunks is truthy (the if statement on line 21 handles the null/falsy case).
| else if (chunks && currentIndex >= chunks.count) | |
| else if (currentIndex >= chunks.count) |
Alxiice
force-pushed
the
dev/fix_nodeactions_position
branch
from
0083441 to
55bc4ac
Compare
cbentejac
left a comment
cbentejac
left a comment
There was a problem hiding this comment.
This needs to be tested but I feel like the suggested changes are solving the graphical update issues, and with them we see only one update instead of two when selecting another node.
| } | ||
|
|
||
| onHeightChanged: { | ||
| Qt.callLater(actionHeader.updatePosition) |
There was a problem hiding this comment.
| Qt.callLater(actionHeader.updatePosition) | |
| actionHeader.updatePosition() |
|
|
||
| onWidthChanged: { | ||
| updatePosition() | ||
| Qt.callLater(actionHeader.updatePosition) |
There was a problem hiding this comment.
| Qt.callLater(actionHeader.updatePosition) | |
| actionHeader.updatePosition() |
| if (y < 0) { | ||
| y = 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Before the if (!selectedNodeDelegate || !draggable) return, I would add:
if (width == 0 && height == 0) {
actionItemsRow.visible = true
return
} else if (width == 0 || height == 0) {
actionItemsRow.visible = false
return
}
actionItemsRow.visible = true
if (!selectedNodeDelegate || !draggable) return
...
Description of the PR
Fix a little but that occurred when we switched the status
failing node action position
-> fixed
Description of the bug
The event when the height is changed was not catched so what happened was :
updatePositionis called on the delegate change and it is set before we update the heightonHeightChangedfunction therefore the NodeAction position is offSpecial point of interest during the review
I added the
onHeightChangedwhich fixes the bug. However one little thing seems off to me :updatePositionis called twice : one withheight=0and a second time withheight=33. We can see a small glitch because of these 2 updates. I tried to avoid that by usingQt.callLaterbut this didn't work. If you have an idea on something we could do, it would be great !