FIX: collapsed and expanded treeview state is preserved when saving#2096
FIX: collapsed and expanded treeview state is preserved when saving#2096
Conversation
ekcoh
left a comment
There was a problem hiding this comment.
Look trough the code (didn't run it) and in general I must say I have love PRs that remove more lines than they add :) Not sure I fully understand how this rather compact PR solves the problem though.
Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionsTreeView.cs
Outdated
Show resolved
Hide resolved
ekcoh
left a comment
There was a problem hiding this comment.
Code changes LGTM if it works. I noticed you have changed to a index based id it seems so would recommend covering expansion state robustness in the tests, e.g. what happens if you expand two branches and then insert a new branch between them? So just want to sort this out before approving.
ekcoh
left a comment
There was a problem hiding this comment.
Unfortunately UI tests are failing based on this change it seems, see CI jobs.
ritamerkl
left a comment
There was a problem hiding this comment.
I would recommend checking if the id's need to be consistent between rebuilds - some functionality may use the id's where the information might get lost assigning new ids.
You are right, I've changed this back to the original behaviour, but with a consistent id that does not depend on order of insertion. |
Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionsTreeView.cs
Outdated
Show resolved
Hide resolved
ritamerkl
left a comment
There was a problem hiding this comment.
LGTM now, good to get rid of unused code
Pauliusd01
left a comment
There was a problem hiding this comment.
LGTM, I checked expanding/collapsing actions, cut/copy/paste workflow, adding new actions and bindings and binding actual keys to them. The expansion states no longer reset and flash as well selection is maintained properly when using the binding dropdown
Description
Fix for ISXB 1164
I've removed (unused) methods related to saving the expanded state of the tree. These might have been an artifact from an earlier implementation, any any case, these were unused.
I changed the guidToId caching mechanism to use guid.GetHashCode() so that we get a deterministic guid->int mapping.
Testing status & QA
Ran local package tests.
Overall Product Risks
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: