Skip to content

Conversation

@AlexTyrer
Copy link
Contributor

@AlexTyrer AlexTyrer commented Nov 18, 2024

Description

Null Reference Exceptions errors could be logged to the console window when pasting items into the Input Actions Editor window.

FIX: Fixed pasting bindings into empty Input Action asset (case ISXB-1180)
FIX: Fixed pasting newly created Action Map when copied from from Project Settings to new Input Action Asset (case ISXB-1182)

Several issues were addressed:

  • Ensure PasteBlocks() deals with invalid selection - when copied type is binding we need to have a valid Action selected to paste into.
  • Improved the context menu population in the Input Actions Editor window (Action Maps and Action views):
    o Ensure Paste option is only present in context menu when appropriate.
    o Fix menu separator presence (or absense).
    o Action Maps view: can Paste ActionMap or Action (if a map is present).
    o Actions view: can paste Actions or Bindings depending on selected item context.

Testing status & QA

Local testing / automated tests.

Overall Product Risks

Relatively small - only affects the context menu in the Input Actions Editor window.

Complexity: 0
Halo Effect: 0

Comments to reviewers

None in particular - I tried lots of copy / paste with various selections & no selection made, including after deleting an item previously selected.

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.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • 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.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

…case ISXB-1180) - take #2

FIX: Fixed pasting bindings into empty Input Action asset (case ISXB-1180)

Ensure PasteBlocks() deals with invalid selection - when copied type is binding we need to have a valid Action selected to paste into.
@AlexTyrer AlexTyrer changed the title [Input System] Fixed pasting bindings into empty Input Action asset (case ISXB-1180) - take #2 FIX: Fixed pasting bindings into empty Input Action asset (case ISXB-1180) - take #2 Nov 18, 2024
@AlexTyrer AlexTyrer requested a review from Pauliusd01 November 18, 2024 20:18
@Pauliusd01
Copy link
Collaborator

The error is gone but I'm still allowed to paste it and it pastes it into the void

19.11.2024.-.Unity.25.mp4

@AlexTyrer
Copy link
Contributor Author

Action selected to paste into.

Yea, there's a few places where stuff like that happens - I'm not sure yet where the context menu gets updated (it shouldn't be on the menu really when there's no valid target).

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.

Updating status

@AlexTyrer
Copy link
Contributor Author

Action selected to paste into.

Yea, there's a few places where stuff like that happens - I'm not sure yet where the context menu gets updated (it shouldn't be on the menu really when there's no valid target).

I've discussed this with Rita after having identified a few more inconsistencies with the context menu.

Now I better understand where this menu gets updated I will now make a pass through the code and make the context menu behaviour more consistent.

…window (Action Maps and Action views) (case ISXB-1180) (case ISXB-1182)

o Ensure Paste option is only present in context menu when appropriate.
o Fix menu separator presence (or absense).
o Action Maps view: can Paste ActionMap or Action (if a map is present).
o Actions view: can paste Actions or Bindings depending on selected item context.
@AlexTyrer
Copy link
Contributor Author

@Pauliusd01 I've reworked a lot of the context menu and Copy/Paste behaviour so it's more consistent.

Would really appreciate you testing it out.

Branch: isxb-1180/fix-paste-bindings-from-clipboard-2

@Pauliusd01 Pauliusd01 self-requested a review November 21, 2024 12:38
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.

LGTM, checked cutting/copying bindings, actions and maps into various situations (empty list, invalid list, clicking outside of selection, etc)

I did find a bug with composite bindings being able to be pasted into non composite actions and then once pasted they disappear but that is also in stable and unrelated to this PR.

@AlexTyrer AlexTyrer merged commit a0219d1 into develop Nov 21, 2024
76 of 77 checks passed
@AlexTyrer AlexTyrer deleted the isxb-1180/fix-paste-bindings-from-clipboard-2 branch November 21, 2024 16:32
@AlexTyrer
Copy link
Contributor Author

LGTM, checked cutting/copying bindings, actions and maps into various situations (empty list, invalid list, clicking outside of selection, etc)

I did find a bug with composite bindings being able to be pasted into non composite actions and then once pasted they disappear but that is also in stable and unrelated to this PR.

Thanks Paulius, appreciate the time and effort.

I'm not surprised there's a few more niggles to shake out in there (I didn't pay much attention to the composite stuff) - it feels more solid to me now with these changes and I've been able to close out a couple more bugs with the fixes.

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.

2 participants