[ui] NodeEditor: Addressed Tab Retention when switching Node selection #2624

waaake · 2024-12-23T03:51:34Z

Description

This PR addresses an issue with the Node Editor, where the selected tab was not getting retained when switching between selected nodes.

Features list

Fixed Node Editor Switching to ensure the current tab retains when it can, depending on the node type.
Fixed issue with StatViewer causing the polish-loop during updates.
~~- Introduced the currentProjectChanged signal which is meant to trigger update on various components when the scene is changed~~
~~- Currently the onCurrentProjectChanged slot is linked to refreshing the NodeEditor tabs.~~

Implementation remarks

Updated the event trigger to be onVisibleChanged rather than onIsComputableChanged as the latter does not get triggered when there is no node selected and then an incomputable node gets selected but the value of isComputable remains the same.

Visibility acts as a better trigger in my view, getting triggered when a node gets selected or deselected.
The condition is also updated to check if the current tab is not valid for the incomputable node before resetting it to 0.

This ensures that when documentation or such tab is selected, we don't end up resetting it back to 0 as it can exist in both the node types.

The available indices can be updated as we progress further with different types of nodes in the future.

…ng node selection Updated the event trigger to be onVisibleChanged and the condition to check whether the current selected node is an incomputable node and the current tab index does not exist for it before resetting the index to 0

codecov · 2024-12-23T03:52:41Z

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.93%. Comparing base (aeb77d8) to head (3f47c54).
Report is 13 commits behind head on develop.

Files with missing lines	Patch %	Lines
meshroom/core/graph.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2624      +/-   ##
===========================================
+ Coverage    69.90%   69.93%   +0.02%     
===========================================
  Files          121      121              
  Lines         7078     7088      +10     
===========================================
+ Hits          4948     4957       +9     
- Misses        2130     2131       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cbentejac

This PR works as intended but there's an unexpected (and unwanted) side effect to it: when the selected tab is the "Statistics" one, moving from one node to another triggers a polish loop:
meshroom/ui/qml/GraphEditor/StatViewer.qml:305:9: QML ColumnLayout: Layout polish loop detected for QQuickColumnLayout(0x2055aed0, parent=0x2044f590, geometry=0,0 438x39). Aborting after two iterations.
(MESHROOM_OUTPUT_QML_WARNINGS needs to be set to 1 for the warning to appear)

There are likely some minor changes to do on the "Statistics" component so it is loaded correctly when it appears as the first component.

I also think the tab index should be reset when we are changing the active project. Currently, it is only reset if we select a non-computable node or if we go back to the homepage, but it never is if we keep on loading projects from the application and selecting computable nodes. What do you think?

The sub column layouts in the parent column layouts were causing polish loop during show/resize ops

The 'isSaving' flag is a way to identify if the project is currently being saved and serves as a way to correctly distinguish whether the current filepath change is due to a save or a new scene or a load operation

cbentejac

The polish loop issue has been fixed and everything else is working as expected, but I think there are a couple of simplifications that can be done.

We're almost there! 😄

cbentejac · 2024-12-30T10:30:54Z

meshroom/core/graph.py

+
+    @property
+    def isSaving(self):
+        """ Return True if the graph is currently being loaded. """


Suggested change

""" Return True if the graph is currently being loaded. """

""" Return True if the graph is currently being saved. """

cbentejac · 2024-12-30T18:22:41Z

meshroom/ui/qml/Application.qml

+        if (! _reconstruction.graph.isSaving) {
+            currentProjectChanged();
+        }


Couldn't we do

Suggested change

if (! _reconstruction.graph.isSaving) {

currentProjectChanged();

}

if (! _reconstruction.graph.isSaving) {

nodeEditor.refresh();

}

?

That would prevent us from having one more signal/slot for something that can be done in a single operation.

currentProjectChanged could thus be completely removed.

The intention was to have a callback and then use that callback for this and other purposes in the future. I do agree with you since we don't have anything right now, let's use that itself and we can review this later when it will be needed.

cbentejac · 2024-12-30T18:25:29Z

meshroom/ui/qml/Application.qml

@@ -20,6 +20,23 @@ Page {
    property alias unsavedDialog: unsavedDialog
    property alias workspaceView: workspaceView

+    property var scenefile: _reconstruction ? _reconstruction.graph.filepath : "";


Suggested change

property var scenefile: _reconstruction ? _reconstruction.graph.filepath : "";

readonly property var scenefile: _reconstruction ? _reconstruction.graph.filepath : "";

Since there's no reason to ever edit this property from the QML side, we can probably set it as read-only.

Nice catch !!, Thank you 🙂

… the tabBar

…or to reset the current Tab index

waaake requested review from fabiencastan and cbentejac December 23, 2024 03:51

waaake self-assigned this Dec 23, 2024

cbentejac added this to the Meshroom 2024.1.0 milestone Dec 23, 2024

cbentejac requested changes Dec 23, 2024

View reviewed changes

waaake added 2 commits December 30, 2024 09:28

[fix][ui] StatViewer: Updated Layouts for Stat Viewer

d142a74

The sub column layouts in the parent column layouts were causing polish loop during show/resize ops

[core] Graph: Introduced 'isSaving' flag

535f2f0

The 'isSaving' flag is a way to identify if the project is currently being saved and serves as a way to correctly distinguish whether the current filepath change is due to a save or a new scene or a load operation

cbentejac requested changes Dec 30, 2024

View reviewed changes

[ui] NodeEditor: Added refresh function to update the currentIndex of…

a2f4ac5

… the tabBar

waaake force-pushed the fix/NodeEditorTab branch from 477949b to a0745e1 Compare December 31, 2024 04:22

[ui] Application: Changing the current project refreshes the NodeEdit…

3f47c54

…or to reset the current Tab index

waaake force-pushed the fix/NodeEditorTab branch from a0745e1 to 3f47c54 Compare December 31, 2024 04:26

cbentejac approved these changes Dec 31, 2024

View reviewed changes

cbentejac merged commit 0586336 into develop Dec 31, 2024
5 checks passed

cbentejac deleted the fix/NodeEditorTab branch December 31, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ui] NodeEditor: Addressed Tab Retention when switching Node selection #2624

[ui] NodeEditor: Addressed Tab Retention when switching Node selection #2624

waaake commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

cbentejac left a comment

cbentejac left a comment

cbentejac Dec 30, 2024

cbentejac Dec 30, 2024

cbentejac Dec 30, 2024

waaake Dec 31, 2024

cbentejac Dec 30, 2024

waaake Dec 31, 2024

	""" Return True if the graph is currently being loaded. """
	""" Return True if the graph is currently being saved. """

	property var scenefile: _reconstruction ? _reconstruction.graph.filepath : "";
	readonly property var scenefile: _reconstruction ? _reconstruction.graph.filepath : "";

[ui] NodeEditor: Addressed Tab Retention when switching Node selection #2624

[ui] NodeEditor: Addressed Tab Retention when switching Node selection #2624

Conversation

waaake commented Dec 23, 2024 • edited Loading

Description

Features list

Implementation remarks

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

cbentejac left a comment

Choose a reason for hiding this comment

cbentejac left a comment

Choose a reason for hiding this comment

cbentejac Dec 30, 2024

Choose a reason for hiding this comment

cbentejac Dec 30, 2024

Choose a reason for hiding this comment

cbentejac Dec 30, 2024

Choose a reason for hiding this comment

waaake Dec 31, 2024

Choose a reason for hiding this comment

cbentejac Dec 30, 2024

Choose a reason for hiding this comment

waaake Dec 31, 2024

Choose a reason for hiding this comment

waaake commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading