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: Arrow key navigation of Input Actions after Action rename. #2082

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion Assets/Tests/InputSystem.Editor/InputActionsEditorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ public override void OneTimeSetUp()
{
base.OneTimeSetUp();
m_Asset = AssetDatabaseUtils.CreateAsset<InputActionAsset>();
m_Asset.AddActionMap("First Name");
var actionMap = m_Asset.AddActionMap("First Name");
m_Asset.AddActionMap("Second Name");
m_Asset.AddActionMap("Third Name");

actionMap.AddAction("Action One");
actionMap.AddAction("Action Two");
}

public override void OneTimeTearDown()
Expand Down Expand Up @@ -55,6 +58,19 @@ IEnumerator WaitForActionMapRename(int index, bool isActive, double timeoutSecs
}, $"WaitForActionMapRename {index} {isActive}", timeoutSecs);
}

IEnumerator WaitForActionRename(int index, bool isActive, double timeoutSecs = 5.0)
{
return WaitUntil(() =>
{
var actionItems = m_Window.rootVisualElement.Q("actions-container").Query<InputActionsTreeViewItem>().ToList();
if (actionItems.Count > index && actionItems[index].IsFocused == isActive)
{
return true;
}
return false;
}, $"WaitForActionRename {index} {isActive}", timeoutSecs);
}

#endregion

[Test]
Expand Down Expand Up @@ -177,5 +193,53 @@ public IEnumerator CanDeleteActionMap()
Assert.That(m_Window.currentAssetInEditor.actionMaps[0].name, Is.EqualTo("First Name"));
Assert.That(m_Window.currentAssetInEditor.actionMaps[1].name, Is.EqualTo("Third Name"));
}

[UnityTest]
[Ignore("Instability, see ISXB-1284")]
public IEnumerator CanRenameAction()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome you've added a proper editor test for this. Thank you!

{
var actionContainer = m_Window.rootVisualElement.Q("actions-container");
var actionItem = actionContainer.Query<InputActionsTreeViewItem>().ToList();
Assume.That(actionItem[1].Q<Label>("name").text, Is.EqualTo("Action Two"));

m_Window.rootVisualElement.Q<TreeView>("actions-tree-view").Focus();
m_Window.rootVisualElement.Q<TreeView>("actions-tree-view").selectedIndex = 1;

// Selection change triggers a state change, wait for the scheduler to process the frame
yield return WaitForSchedulerLoop();
yield return WaitForNotDirty();
yield return WaitForFocus(m_Window.rootVisualElement.Q<TreeView>("actions-tree-view"));

// Re-fetch the actions since the UI may have refreshed.
actionItem = actionContainer.Query<InputActionsTreeViewItem>().ToList();

// Click twice to start the rename
SimulateClickOn(actionItem[1]);
// If the item is already focused, don't click again
if (!actionItem[1].IsFocused)
{
SimulateClickOn(actionItem[1]);
}

yield return WaitForActionRename(1, isActive: true);

// Rename the action
SimulateTypingText("New Name");

// Wait for rename to end and focus to return from text field
yield return WaitForSchedulerLoop();
yield return WaitForFocus(m_Window.rootVisualElement.Q<TreeView>("actions-tree-view"));

// Check on the UI side
actionContainer = m_Window.rootVisualElement.Q("actions-container");
Assume.That(actionContainer, Is.Not.Null);
actionItem = actionContainer.Query<InputActionsTreeViewItem>().ToList();
Assert.That(actionItem, Is.Not.Null);
Assert.That(actionItem.Count, Is.EqualTo(2));
Assert.That(actionItem[1].Q<Label>("name").text, Is.EqualTo("New Name"));

// Check on the asset side
Assert.That(m_Window.currentAssetInEditor.actionMaps[0].actions[1].name, Is.EqualTo("New Name"));
}
}
#endif
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed an issue where action map delegates were not updated when the asset already assigned to the PlayerInput component were changed [ISXB-711](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-711).
- Fixed Action properties edition in the UI Toolkit version of the Input Actions Asset editor. [ISXB-1277](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1277)
- Fixed an issue where batch jobs would fail with "Error: Error building Player because scripts are compiling" if a source generated .inputactions asset is out of sync with its generated source code (ISXB-1300).
- Fixed arrow key navigation of Input Actions after Action rename. [ISXB-1024](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1024)

### Changed
- Changed location of the link xml file (code stripping rules), from a temporary directory to the project Library folder (ISX-2140).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,13 @@ public override void RedrawUI(ViewState viewState)
{
m_ActionsTreeView.Clear();
m_ActionsTreeView.SetRootItems(viewState.treeViewData);
// UI toolkit doesn't behave the same on 6000.0 way when refreshing items
// On previous versions, we need to call Rebuild() to refresh the items since refreshItems() is less predicatable
#if UNITY_6000_0_OR_NEWER
m_ActionsTreeView.RefreshItems();
#else
m_ActionsTreeView.Rebuild();
#endif
if (viewState.newElementID != -1)
{
m_ActionsTreeView.SetSelectionById(viewState.newElementID);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ internal class InputActionsTreeViewItem : VisualElement
private const string kRenameTextField = "rename-text-field";
public event EventCallback<string> EditTextFinished;

// for testing purposes to know if the item is focused to accept input
internal bool IsFocused { get; private set; } = false;

private bool m_IsEditing;
private static InputActionsTreeViewItem s_EditingItem = null;

Expand All @@ -34,9 +37,13 @@ public InputActionsTreeViewItem()
renameTextfield.selectAllOnFocus = true;
renameTextfield.selectAllOnMouseUp = false;


RegisterCallback<MouseDownEvent>(OnMouseDownEventForRename);
renameTextfield.RegisterCallback<FocusOutEvent>(e => OnEditTextFinished());
renameTextfield.RegisterCallback<FocusInEvent>(e => IsFocused = true);
renameTextfield.RegisterCallback<FocusOutEvent>(e =>
{
OnEditTextFinished();
IsFocused = false;
});
}

public Label label => this.Q<Label>();
Expand All @@ -51,28 +58,38 @@ public void UnregisterInputField()
renameTextfield.UnregisterCallback<FocusOutEvent>(e => OnEditTextFinished());
}

private float lastSingleClick;
private double lastSingleClick;
private static InputActionsTreeViewItem selected;

private void OnMouseDownEventForRename(MouseDownEvent e)
{
if (e.clickCount != 1 || e.button != (int)MouseButton.LeftMouse || e.target == null)
return;

if (selected == this && Time.time - lastSingleClick < 3f)
var now = EditorApplication.timeSinceStartup;
if (selected == this && now - lastSingleClick < 3)
{
FocusOnRenameTextField();
e.StopImmediatePropagation();
lastSingleClick = 0;
return;
}
lastSingleClick = Time.time;
lastSingleClick = now;
selected = this;
}

public void Reset()
{
if (m_IsEditing)
{
lastSingleClick = 0;
delegatesFocus = false;

renameTextfield.AddToClassList(InputActionsEditorConstants.HiddenStyleClassName);
label.RemoveFromClassList(InputActionsEditorConstants.HiddenStyleClassName);
s_EditingItem = null;
m_IsEditing = false;
}
EditTextFinished = null;
m_IsEditing = false;
}

public void FocusOnRenameTextField()
Expand All @@ -87,7 +104,7 @@ public void FocusOnRenameTextField()

//a bit hacky - e.StopImmediatePropagation() for events does not work like expected on ListViewItems or TreeViewItems because
//the listView/treeView reclaims the focus - this is a workaround with less overhead than rewriting the events
DelayCall();
schedule.Execute(() => renameTextfield.Q<TextField>().Focus()).StartingIn(120);
renameTextfield.SelectAll();

s_EditingItem = this;
Expand All @@ -99,12 +116,6 @@ public static void CancelRename()
s_EditingItem?.OnEditTextFinished();
}

async void DelayCall()
{
await Task.Delay(120);
renameTextfield.Q<TextField>().Focus();
}

private void OnEditTextFinished()
{
if (!m_IsEditing)
Expand All @@ -123,6 +134,7 @@ private void OnEditTextFinished()
renameTextfield.schedule.Execute(() => renameTextfield.SetValueWithoutNotify(text));
return;
}

EditTextFinished?.Invoke(text);
}
}
Expand Down
Loading