-
Notifications
You must be signed in to change notification settings - Fork 317
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: fixed logic when checking for next composite part in the TreeView #1925
FIX: fixed logic when checking for next composite part in the TreeView #1925
Conversation
@@ -608,33 +608,41 @@ public static List<TreeViewItemData<ActionOrBindingData>> GetActionsAsTreeViewDa | |||
{ | |||
var serializedInputBinding = actionBindings[i]; | |||
var inputBindingId = new Guid(serializedInputBinding.id); | |||
|
|||
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.
It seems these changes prevent any exception from being thrown which is an improvement so it could land in that sense but I think we should instead prevent the last instance of each composite part being supported by the composite class from being deleted. If we do not prevent this we have a non-reversible behavior in the editor where deleting a composite part cannot be re-added. We allow duplicating the parts which is fine I guess for adding multiple bindings per part but I believe we should avoid deleting the last instance of each part.
Not reproing the issue anymore so would be willing to approve but I see there's discussion on the implementation so will hold off. Poke me if It's fine to pass and I'll approve |
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.
Just updating status that I'm waiting for a resolution on whether the approach here is correct. I can approve it is fine
c037f15
to
7518fd6
Compare
@ekcoh is this PR ok? IIRC we discussed accepting this PR, and scheduling other work (to add a + button to a composite) for a future time. |
@simonwittber Sorry for the heavily delayed feedback. Let's land it as a fix for the bug. |
…deletion-take-two
…deletion-take-two
Description
Deleting the last part of a composite action raised an exception in the editor. This PR fixes this issue.
ISXB-804
Changes made
The treeview logic formerly looked for the next item to process without checking if that item exists. Changes made to address this.
Checklist
Before review:
Changed
,Fixed
,Added
sections.([case %number%](https://issuetracker.unity3d.com/issues/...))
.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.