Feature/table view - #3142
Conversation
We use repeaters instead of a real tableview as tablemodel does not seem to support dynamic layout generation.
There was a problem hiding this comment.
Code Review
This pull request introduces a new tableViewAttributeComponent in AttributeItemDelegate.qml to render list attributes of type GroupAttribute as a table. The reviewer provided valuable feedback, identifying that directly setting cell values bypasses Meshroom's scene management and undo/redo stack, and that resizing columns and rows using local coordinates on moving MouseArea elements causes feedback loops and jitter. Additionally, the reviewer suggested replacing the manual scrollbar implementation with a Flickable component to support native scrolling features.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3142 +/- ##
===========================================
+ Coverage 85.35% 86.27% +0.92%
===========================================
Files 73 81 +8
Lines 11453 12367 +914
===========================================
+ Hits 9776 10670 +894
- Misses 1677 1697 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Value assignement is now compatible with Meshroom's undo/redo stack
And a counter displays the number of items in the list
For row resize, the whole node UI scrolls
mouse wheel alone allows vertical scrolling. ctrl+mouse wheel allows horizontal scrolling.
The tableviews now has a button for windowed visualisation, supports most meshroom param types, and some UI fixes
Alxiice
left a comment
There was a problem hiding this comment.
So I have a few general changes that I would prefer to see before diving again in this PR:
- First I don't think this is a good design to have a 700-lines component inserted here, I think you could use the
AttributeControlsfolder to put your delegates which would help cleaning this PR a lot. I see at least aTableViewAttribute.qml,TableViewColumnDelegate.qmlandTableViewCellDelegate.qmlcomponents that you can create. - I think this would make it much easier if you use this which is a native Table View QML component.
- Please avoid patterns like
var h = rowHeightsto make "smaller" variables, this makes the code less readable. - Finally, I have several UX remarks:
- The table view is not responsibe (no layout fillWidth)
- I think you should add the "add row" action to the table view Window too.
|
And I would have preferred you read the implementation remarks I left before diving into your comment ;-) The native Table View component was rejected due to, to my knowledge, lack of dynamic layout support. |
…onsible table view, add row button added to the window
There was a problem hiding this comment.
Architecturally speaking, the split between the 3 QML files is relevant and works well! It is functional, and I already see where the component could be re-used (outside of the Attribute Editor and of node-custom interfaces).
If I were to sum up some of my in-file comments, I'd say:
- Avoid using absolute colors; use the palette instead, and variations using
Qt.lighterandQt.darkerto ensure we stick to the current theme (dark/light palettes). - Syntax-wise, do not align variables with spaces when assigning them, do not use one-liners for
if/forstatements. Beware of magic numbers in width/height computations. - I think we do need to take the definition of the components out of
AttributeItemDelegate.qml(and out ofTableViewCellDelegate.qml), but most likely in another PR (that would require to be merged prior to this one).
Now from a purely functional point of view, I noticed a few issues (that I haven't connected to the code, since I have not looked at it in-depth):
-
The initial height display for lists with a single element is incorrect, especially when compared to lists with several elements:
.
It is also the case when opening it in full screen:
. -
When extending a "full screen" window to the actual full screen, the display is weird if there are few elements:
We would prefer having fixed-size rows, and if the window is higher than the number of elements there are to display, then it should be empty when there's no element to fill it. This for example creates issue when clicking out of an attribute, as there is absolutely nowhere to "empty" to click. This may lead to involuntary changes of attribute values that may not even be noticed until it is too late (it happened to me, with the intrinsics displayed on the screenshot here).
- Since the width of the columns is based on the length of the column title, it sometimes gives strange displays:
It'd probably be best to have a minimum column width that is wider than what we have here, and that may also depend of the component that is displayed in it.
-
It is not visible in the previous screenshot, but the focus highlight is on the whole cell rather than on the element that is being edited. It is quite striking when using the full screen mode (with rows that are higher than expected because they're filling up the empty space).
-
I noticed some inconsistent behaviors with the scrollbars (especially the vertical one) depending on where we are hovering when attempting to scroll. For example it does not work at all when hovering (without clicking or having the focus anywhere on the table view) on an integer attribute, but it does when hovering on a string attribute, on the very same row. When hovering on the header, the scroll starts but is then "stopped" before we get to reach the bottom of the list.
-
The horizontal is not always visible, as it is attached to the bottom of the table view, which is itself often hidden:
It is a problem because scrolling down the table view vertically never brings us to reach it if it is not visible in the first place, as only the outer scrollbar (the Attribute Editor's) allows it. There probably is a way to attach it to the bottom of the view rather than the bottom of the component itself.
-
Scrolling when hovering (without having selected) on an attribute may change its value. I believe this a behavior that we have on several attribute components but it should be disabled here unless the attribute is selected. Otherwise attributes' values are changed unwillingly and unknowingly. At least they are correctly recorded on the undo stack!
-
When scrolling over int attributes in a large group (and apparently attempting to change their values), I got the following errors:
[2026-07-30 19:16:20.815][root][ERROR] Error while trying command 'Set Attribute 'CameraInit_1.viewpoints[0].viewId'':
Traceback (most recent call last):
File "meshroom/core/desc/attribute.py", line 475, in validateValue
return int(value)
^^^^^^^^^^
ValueError: cannot convert float NaN to integer
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "meshroom/ui/commands.py", line 66, in tryAndPush
res = command.redoImpl()
^^^^^^^^^^^^^^^^^^
File "meshroom/ui/commands.py", line 321, in redoImpl
attribute.value = self.value
^^^^^^^^^^^^^^^
File "meshroom/core/attribute.py", line 249, in _setValue
convertedValue = self.validateValue(value)
^^^^^^^^^^^^^^^^^^^^^^^^^
File "meshroom/core/attribute.py", line 420, in validateValue
return self._desc.validateValue(value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "meshroom/core/desc/attribute.py", line 477, in validateValue
raise ValueError(f"IntParam only supports int value (param: {self.name}, value: "
ValueError: IntParam only supports int value (param: viewId, value: nan, type: <class 'float'>)There was a problem hiding this comment.
I had not realized that the components from AttributeItemDelegate were almost all duplicated here. It will be a bit of a hassle but in the end I think it'd be best to externalize them into their own components, and instantiate them both in AttributeItemDelegate and here.
I might even go as far as saying that removing the components' definition from AttributeItemDelegate.qml could be a stand-alone PR on which this one would rely. To be discussed.
Description
This PR adds a new QML to visualise ListAttributes of GroupAttributes as a table.
It contains the following:
Features list
Implementation remarks