Fix: Tree-view items always collapsed by default #119166

Merged
Member

Since 660867fa00, having tree-view items uncollapsed by default using
the set_collapsed() functions wouldn't work anymore. An attempt to do
this would assert even, so eb71d9f7bc disabled the assert.

I think a function designed to handle exactly this is the best solution,
it makes the intent & behavior more clear than before, and highlights
that this is a special case.

It's a bit of a shame that something as simple as the collapsed state is
becoming so tricky to manage. I have some ideas to improve this, but
that will require some more work.

Fixes #117957.

Since 660867fa00, having tree-view items uncollapsed by default using the `set_collapsed()` functions wouldn't work anymore. An attempt to do this would assert even, so eb71d9f7bc disabled the assert. I think a function designed to handle exactly this is the best solution, it makes the intent & behavior more clear than before, and highlights that this is a special case. It's a bit of a shame that something as simple as the collapsed state is becoming so tricky to manage. I have some ideas to improve this, but that will require some more work. Fixes #117957.
Julian Eisel added the
Module
User Interface
label 2024-03-07 14:56:37 +01:00
Julian Eisel added 1 commit 2024-03-07 14:56:46 +01:00
Since 660867fa00, having tree-view items uncollapsed by default using
the `set_collapsed()` functions wouldn't work anymore. An attempt to do
this would assert even, so eb71d9f7bc disabled the assert.

I think a function designed to handle exactly this is the best solution,
it makes the intent & behavior more clear than before, and highlights
that this is a special case.

It's a bit of a shame that something as simple as the collapsed state is
becoming so tricky to manage. I have some ideas to improve this, but
that will require some more work.

Fixes #117957.
Julian Eisel added this to the 4.1 milestone 2024-03-07 15:02:01 +01:00
Julian Eisel requested review from Sybren A. Stüvel 2024-03-07 15:02:14 +01:00
Julian Eisel requested review from Hans Goudey 2024-03-07 15:02:14 +01:00
Hans Goudey approved these changes 2024-03-07 15:04:38 +01:00
Hans Goudey left a comment
Member

Looks better, thanks for doing that. I don't think this fixes #117957 though. Maybe it just lowers the priority to normal.

Looks better, thanks for doing that. I don't think this fixes #117957 though. Maybe it just lowers the priority to normal.
@ -529,1 +529,4 @@
void AbstractTreeViewItem::uncollapse_by_default()
{
BLI_assert_msg(get_tree_view().is_reconstructed() == false,
Member

this->get_tree_view()

`this->get_tree_view()`
JulianEisel marked this conversation as resolved
Julian Eisel added 1 commit 2024-03-07 15:38:45 +01:00
Author
Member

Looks better, thanks for doing that. I don't think this fixes #117957 though. Maybe it just lowers the priority to normal.

It fixes the regression from 660867fa00, anything else is not a bug it's simply not implemented.

> Looks better, thanks for doing that. I don't think this fixes #117957 though. Maybe it just lowers the priority to normal. It fixes the regression from 660867fa00, anything else is not a bug it's simply not implemented.
Member

The problem mentioned by the title of the task will still be there after this commit.

The problem mentioned by the title of the task will still be there after this commit.
Sybren A. Stüvel reviewed 2024-03-07 16:00:00 +01:00
Sybren A. Stüvel left a comment
Member

The 'uncollapse by default' concept feels a bit strange to me. I can see that this works around the assertion, but it might be good to give the function a different name that matches more with the intented (future) behaviour.

How about naming it set_collapsed_default() and making it a virtual function? That way subclasses can determine for themselves whether they should be collapsed or expanded by default.

Or not even make it virtual just yet. But I would at least try and transition from "uncollapse by default" to "now set the default state". And then the base implementation of that function has "expanded" as default state, but that's of no interest to the caller. I think that'll produce a looser coupling, that's then later easier to work with.

The 'uncollapse by default' concept feels a bit strange to me. I can see that this works around the assertion, but it might be good to give the function a different name that matches more with the intented (future) behaviour. How about naming it `set_collapsed_default()` and making it a `virtual` function? That way subclasses can determine for themselves whether they should be collapsed or expanded by default. Or not even make it `virtual` just yet. But I would at least try and transition from "uncollapse by default" to "now set the default state". And then the base implementation of that function has "expanded" as default state, but that's of no interest to the caller. I think that'll produce a looser coupling, that's then later easier to work with.
Author
Member

The 'uncollapse by default' concept feels a bit strange to me. I can see that this works around the assertion, but it might be good to give the function a different name that matches more with the intented (future) behaviour.

I'm not sure what that intended future behavior would be. For me it states exactly what is the intention, it uncollapses items by default (until the user changes the collapsed state). I'm not too attached to the exact wording of course, set_collapsed_default() could be used, but then you have to read the docs first to tell you that collapsing happens by default, so you don't need to call this with a false value to ensure the collapsed state. uncollapse_by_default() makes this more clear I think.

How about naming it set_collapsed_default() and making it a virtual function? That way subclasses can determine for themselves whether they should be collapsed or expanded by default.

Or not even make it virtual just yet. But I would at least try and transition from "uncollapse by default" to "now set the default state". And then the base implementation of that function has "expanded" as default state, but that's of no interest to the caller. I think that'll produce a looser coupling, that's then later easier to work with.

I'd rather make this virtual:

  • IMO every virtual function adds complexity, and it should be used sparingly. The class hierarchy can become more complicated, there's more potential for error (Do I have to call the base class function first? What about side-effects not expected during the API design? Is this override really doing what it's supposed to? Which override will it call?), it increases the virtual part of the API which class-users may feel like they need to know to use it well, ... Such overrides also add boilerplate code, which I already see too much of with tree-views for my taste. A simple non-virtual uncollapse_by_default() function is very explicit and clear.
  • As opposed to the promise of OOP, people often find that in practice increases coupling, because there are a lot of implicit, behavioral dependencies between base classes and overriding classes. The overriding class depends on what the caller or base class do, and vise-versa. They all have to do the right things for the right result, and there's plenty of potential for error, as explained above. So I don't think "loose coupling" is a good design rationale, it's arguably a false promise. Of course virtual functions are a useful tool still, I use them quite often. I just don't think it's the right tool in this case.
  • I'm not sure if it should be "baked into" a class if it should expand by default or not, this can depend very much on the use case. For example I think we'll have some reusable catalog tree items, in some cases we'd want root level catalogs to be collapsed and in some not. A virtual function would mean I'd have to sub-class the sub-class just to override the default collapsed state. I think the class user should decide over such simple presentation attributes, not the class.
> The 'uncollapse by default' concept feels a bit strange to me. I can see that this works around the assertion, but it might be good to give the function a different name that matches more with the intented (future) behaviour. I'm not sure what that intended future behavior would be. For me it states exactly what is the intention, it uncollapses items by default (until the user changes the collapsed state). I'm not too attached to the exact wording of course, `set_collapsed_default()` could be used, but then you have to read the docs first to tell you that collapsing happens by default, so you don't need to call this with a `false` value to ensure the collapsed state. `uncollapse_by_default()` makes this more clear I think. > How about naming it `set_collapsed_default()` and making it a `virtual` function? That way subclasses can determine for themselves whether they should be collapsed or expanded by default. > > Or not even make it `virtual` just yet. But I would at least try and transition from "uncollapse by default" to "now set the default state". And then the base implementation of that function has "expanded" as default state, but that's of no interest to the caller. I think that'll produce a looser coupling, that's then later easier to work with. I'd rather make this `virtual`: - IMO every `virtual` function adds complexity, and it should be used sparingly. The class hierarchy can become more complicated, there's more potential for error (_Do I have to call the base class function first? What about side-effects not expected during the API design? Is this override really doing what it's supposed to? Which override will it call?_), it increases the `virtual` part of the API which class-users may feel like they need to know to use it well, ... Such overrides also add boilerplate code, which I already see too much of with tree-views for my taste. A simple non-virtual `uncollapse_by_default()` function is very explicit and clear. - As opposed to the promise of OOP, people often find that in practice increases coupling, because there are a lot of implicit, behavioral dependencies between base classes and overriding classes. The overriding class depends on what the caller or base class do, and vise-versa. They all have to do the right things for the right result, and there's plenty of potential for error, as explained above. So I don't think "loose coupling" is a good design rationale, it's arguably a false promise. Of course `virtual` functions are a useful tool still, I use them quite often. I just don't think it's the right tool in this case. - I'm not sure if it should be "baked into" a class if it should expand by default or not, this can depend very much on the use case. For example I think we'll have some reusable catalog tree items, in some cases we'd want root level catalogs to be collapsed and in some not. A `virtual` function would mean I'd have to sub-class the sub-class just to override the default collapsed state. I think the class user should decide over such simple presentation attributes, not the class.
Author
Member

Talked to Sybren in the chat. He said it's fine if I continue with this, he didn't mean to block it.

Talked to Sybren in the chat. He said it's fine if I continue with this, he didn't mean to block it.
Julian Eisel merged commit 745fd2a2cb into blender-v4.1-release 2024-03-18 19:33:05 +01:00
Julian Eisel deleted branch temp-fix-tree-view-item-default-collapse 2024-03-18 19:33:11 +01:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#119166
No description provided.