Skip to content
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

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

smnwttbr
Copy link
Collaborator

@smnwttbr smnwttbr commented May 13, 2024

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:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@smnwttbr smnwttbr requested review from ekcoh and Pauliusd01 May 13, 2024 03:07
@smnwttbr smnwttbr self-assigned this May 13, 2024
@@ -608,33 +608,41 @@ public static List<TreeViewItemData<ActionOrBindingData>> GetActionsAsTreeViewDa
{
var serializedInputBinding = actionBindings[i];
var inputBindingId = new Guid(serializedInputBinding.id);

Copy link
Collaborator

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.

@Pauliusd01
Copy link
Collaborator

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

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a 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

@smnwttbr smnwttbr force-pushed the isxb-804-fix-exception-on-last-composite-deletion-take-two branch from c037f15 to 7518fd6 Compare June 6, 2024 01:50
@simonwittber
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants