FIX: Allow Copy/Paste between windows (ISX-1823). #1844

graham-huws · 2024-02-15T00:44:48Z

Description

Changes made

This was previously not working because the Paste command would only apply to the relevant Action Maps/Actions List/Tree, so if neither of those were focused, nothing would happen. We now also listen for Paste at the top level of the element hierarchy and send paste commands from there.

Notes

Issues:
• Undoing adding an Action Map/Action leaves the list unselected. Pasting at that point fails because the state thinks there's still a valid item selected. Mentioned here, and should be added as a separate bug.
• Currently won't work if the window is tabbed to - user has to click somewhere in the window. Think I'll need to ask UITK team about 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.

…gher up the hierarchy. (ISX-1823)

Packages/com.unity.inputsystem/CHANGELOG.md

ekcoh · 2024-02-15T20:33:02Z

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/CopyPasteHelper.cs

@@ -288,13 +292,14 @@ private static void PasteAction(SerializedProperty arrayProperty, string jsonToI
        private static int PasteBindingOrComposite(SerializedProperty arrayProperty, string json, int index, string actionName, bool createCompositeParts = true)
        {
            var pastePartOfComposite = IsPartOfComposite(json);
-            if (index > 0)
+            if (index > 0 && arrayProperty.arraySize > 0 && index - 1 < arrayProperty.arraySize)


I am confused about an index that must be greater than zero and at least two elements from the end of the anway? Would recommend adding an inline comment if this is correct but some kind of special case.

This somewhat doubled up from merging develop - should be clearer now :)

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/CopyPasteHelper.cs

ekcoh · 2024-02-15T20:36:16Z

As a side-note, when #1834 lands there will exist an interesting scenario for this functionality when the same asset is open simultaneously in two windows, both in Project Settings and in a free-floating Input Action Editor.

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/CopyPasteHelper.cs

ritamerkl · 2024-02-16T10:50:32Z

...ges/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/InputActionsEditorView.cs

@@ -159,6 +169,49 @@ public class ViewState
            public IEnumerable<InputControlScheme> controlSchemes;
            public int selectedControlSchemeIndex;
        }
+
+        void OnExecuteCommand(ExecuteCommandEvent evt)


Feels like double work - do we handle events twice here? (additional to handling them in the InputActionsTreeView / InputActionMapsView)

If you mean, could both be called with one action - I don't believe this is possible since we StopPropagation.

If you mean there's code overlap - I think I've now reduced the code in this section to the point where reducing it further would just mean adding an extra string comparison and function call to jump to the maps/actions views' OnExecuteCommand calls, but let me know what you think 🙂

On a somewhat related note to that - do we need the HasPastableClipboardData calls in OnExecuteCommand in ActionsTreeView/ActionMapsView? Won't that be checked anyway in one of the functions that PasteItems calls? It feels like that's logically where it should happen, if it doesn't already.

Is it? Even with using the events on TrickleDown? If the second event is used OnBubbleUp this means we could catch the event beforehand.
I thought about a delegation concept when I said "do we handle events twice", a event handler somewhere that delegates to the list/treeView. It was more a rough idea, if the trickledown/bubbleup does not work out.
About your second comment: I think there is a safety check, and if we could indeed skip the one in the list/treeviews.

I think I'm not sure what the problem you want to solve is - do you believe there's a way the event would be acted upon twice? If so, how? (I'm definitely new to the events system!) If it's just that you'd prefer the actual Paste command is only in one place, fair enough, but an intermediary handler to decide which View to send the command to would surely be more complex than what's here in the editor View?
Apologies if I'm entirely misunderstanding!

Ah - were you suggesting removing the Paste event handlers for the Action Maps/Actions, and having a single handler for the entire window? And then events from anywhere lower in the hierarchy can bubble up to the root?

yes, that was what I was thinking of as one solution. The other way would be to catch the events at actionMap/actionTreeView on trickleDown, and stop the event propagation and then only if nothing was registered there we catch the rest of the events on bubble up on the root element to handle them from there.
For your reference: https://docs.unity3d.com/2021.2/Documentation/Manual/UIE-Events-Handling.html
This describes the event phases pretty well.

Thanks Rita - Have pushed a couple changes, looks much better now I think!

I think like that the logic to paste actions into action maps (and add them as last element) is broken because it is impossible to determine from outside of the actionMapView in wich list the paste was performed. Added @Pauliusd01

…events to work.

stefanunity

Finished for now. Two bugs not yet fixed are:

BUG: trying to paste an action into empty map throws error:
--ArgumentException: '0' cannot be greater than -1.
BUG: trying to paste a modifier into empty map throws errror
--NullReferenceException: Object reference not set to an instance of an object

Once they are fixed it looks good.

graham-huws · 2024-02-22T12:09:09Z

Finished for now. Two bugs not yet fixed are:

1. BUG: trying to paste an action into empty map throws error:
   --ArgumentException: '0' cannot be greater than -1.

2. BUG: trying to paste a  modifier into empty map throws errror
   --NullReferenceException: Object reference not set to an instance of an object

Once they are fixed it looks good.

Thanks @stefanunity - I believe these should both be fixed now, can you confirm?

Pauliusd01 · 2024-02-22T12:13:41Z

Unity_2024-02-22_14-11-34.mp4

Cutting and pasting into an empty asset somehow "resurrects" all of the previously deleted actions. (I do deselect ProjectWideActions to confirm the auto save but that does not seem to help)

stefanunity · 2024-02-22T13:02:56Z

Thanks @stefanunity - I believe these should both be fixed now, can you confirm?

Both are fixed now.

ekcoh · 2024-02-23T13:51:48Z

The bug called out @Pauliusd01 doesn't really make any sense to me but should be fixed. Where is this coming from if not the copy buffer?

…between-windows

Pauliusd01

Updating status - waiting for fixes

graham-huws force-pushed the ISX-1823-cut-copy-between-windows branch from 94d27e7 to 8944d39 Compare February 15, 2024 00:46

FIX: Allow Copy/Paste between windows by intercepting the commands hi…

4d21dbf

…gher up the hierarchy. (ISX-1823)

graham-huws force-pushed the ISX-1823-cut-copy-between-windows branch from 8944d39 to 4d21dbf Compare February 15, 2024 11:41

graham-huws marked this pull request as ready for review February 15, 2024 11:42

graham-huws requested review from ritamerkl and Pauliusd01 February 15, 2024 11:42

ekcoh reviewed Feb 15, 2024

View reviewed changes

ritamerkl reviewed Feb 16, 2024

View reviewed changes

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

4e41f29

stefanunity requested review from stefanunity and removed request for Pauliusd01 February 19, 2024 10:07

graham-huws and others added 4 commits February 20, 2024 11:18

Address feedback, and fix focusing when tabbing in to the window for …

9cda4d8

…events to work.

Catch errors, and simplify OnExecuteCommand.

108c268

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

5b5e663

Avoid array OOB.

384cb74

stefanunity approved these changes Feb 21, 2024

View reviewed changes

graham-huws added 2 commits February 21, 2024 15:58

Deduplicate Paste handling.

b7617fe

Fix SelectBinding not doing so, and simplify Paste handling.

2e64bb2

ritamerkl requested a review from Pauliusd01 February 22, 2024 11:45

graham-huws and others added 3 commits February 22, 2024 12:01

Fix array index bounds. Clamp doesn't work if arraySize is 0.

7ae236a

Catch issue with pasting a binding into an empty map.

a5e7de0

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

9a9cf85

Pauliusd01 and others added 2 commits February 22, 2024 14:14

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

78904a7

Fix formatting after merge

1b56964

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

3179bf0

ritamerkl and others added 2 commits February 26, 2024 10:53

Merge remote-tracking branch 'origin/develop' into ISX-1823-cut-copy-…

0774a65

…between-windows

Stop propagation of paste event when we handle it.

5c4aa27

Pauliusd01 requested changes Feb 28, 2024

View reviewed changes

graham-huws and others added 6 commits February 29, 2024 03:19

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

2bad8bb

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

d1d99a9

Update after merging develop

519abd9

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

80b2705

Make use of allowUICommandExecution protection bool like in other Views.

6227e7e

Merge branch 'develop' into ISX-1823-cut-copy-between-windows

0abdaef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Allow Copy/Paste between windows (ISX-1823). #1844

FIX: Allow Copy/Paste between windows (ISX-1823). #1844

graham-huws commented Feb 15, 2024 •

edited

ekcoh Feb 15, 2024

graham-huws Feb 20, 2024

ekcoh commented Feb 15, 2024

ritamerkl Feb 16, 2024

graham-huws Feb 20, 2024

ritamerkl Feb 20, 2024

graham-huws Feb 20, 2024

graham-huws Feb 20, 2024

ritamerkl Feb 21, 2024

graham-huws Feb 21, 2024

ritamerkl Feb 22, 2024

stefanunity left a comment

graham-huws commented Feb 22, 2024

Pauliusd01 commented Feb 22, 2024 •

edited

stefanunity commented Feb 22, 2024

ekcoh commented Feb 23, 2024

Pauliusd01 left a comment

FIX: Allow Copy/Paste between windows (ISX-1823). #1844

Are you sure you want to change the base?

FIX: Allow Copy/Paste between windows (ISX-1823). #1844

Conversation

graham-huws commented Feb 15, 2024 • edited

Description

Changes made

Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekcoh commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanunity left a comment

Choose a reason for hiding this comment

graham-huws commented Feb 22, 2024

Pauliusd01 commented Feb 22, 2024 • edited

stefanunity commented Feb 22, 2024

ekcoh commented Feb 23, 2024

Pauliusd01 left a comment

Choose a reason for hiding this comment

graham-huws commented Feb 15, 2024 •

edited

Pauliusd01 commented Feb 22, 2024 •

edited