-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: collapsed and expanded treeview state is preserved when saving #2096
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately UI tests are failing based on this change it seems, see CI jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: