Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
64 changes: 63 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].IsTextFieldFocused == isActive)
{
return true;
}
return false;
}, $"WaitForActionRename {index} {isActive}", timeoutSecs);
}

#endregion

[Test]
Expand Down Expand Up @@ -173,5 +189,51 @@ 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]
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.

great to see a test for this! There were a lot of cases on renaming and focus issues already.

{
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].IsTextFieldFocused)
{
SimulateClickOn(actionItem[1]);
}

yield return WaitForActionRename(1, isActive: true);

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

// Wait for rename to end
yield return WaitForActionRename(1, isActive: false);

// 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 @@ -22,6 +22,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed missing documentation for source generated Input Action Assets. This is now generated as part of the source code generation step when "Generate C# Class" is checked in the importer inspector settings.
- Fixed pasting into an empty map list raising an exception. [ISXB-1150](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1150)
- Fixed pasting bindings into empty Input Action asset. [ISXB-1180](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1180)
- Fixed arrow key navigation of Input Actions after Action rename. [ISXB-1024](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1024)

### Changed
- Added back the InputManager to InputSystem project-wide asset migration code with performance improvement (ISX-2086).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ internal class ActionsTreeView : ViewBase<ActionsTreeView.ViewState>
private readonly ScrollView m_PropertiesScrollview;

private bool m_RenameOnActionAdded;
private bool m_FocusOnRenameActionFinish;
private readonly CollectionViewSelectionChangeFilter m_ActionsTreeViewSelectionChangeFilter;

//save TreeView element id's of individual input actions and bindings to ensure saving of expanded state
Expand Down Expand Up @@ -215,6 +216,8 @@ public override void RedrawUI(ViewState viewState)

// Don't want to show action properties if there's no actions.
m_PropertiesScrollview.visible = m_ActionsTreeView.GetTreeCount() > 0;

FinishActionRename(viewState.newElementID);
}

private void OnDraggedItem(DragPerformEvent evt)
Expand Down Expand Up @@ -308,6 +311,17 @@ internal void RenameActionItem(int index)
m_ActionsTreeView.GetRootElementForIndex(index)?.Q<InputActionsTreeViewItem>()?.FocusOnRenameTextField();
}

private void FinishActionRename(int id)
{
if (!m_FocusOnRenameActionFinish || id == -1)
return;
m_ActionsTreeView.ScrollToItemById(id);
var treeViewItem = m_ActionsTreeView.GetRootElementForId(id)?.Q<InputActionsTreeViewItem>();
treeViewItem?.FocusOnRenameFinish();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it really needs to use this new function ?
the Id here might not be the expected one. If during the rename you click on another element "id" will be the selected id and not the one that was renamed.

Locally it seems to work well with treeViewItem?.FocusOnRenameFinish(); replaced by treeViewItem?.Focus();


m_FocusOnRenameActionFinish = false;
}

internal void AddAction()
{
Dispatch(Commands.AddAction());
Expand Down Expand Up @@ -369,6 +383,8 @@ private void ChangeActionOrCompositName(ActionOrBindingData data, string newName
Dispatch(Commands.ChangeActionName(data.actionMapIndex, data.name, newName));
else if (data.isComposite)
Dispatch(Commands.ChangeCompositeName(data.actionMapIndex, data.bindingIndex, newName));

m_FocusOnRenameActionFinish = true;
}

internal int GetMapCount()
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 IsTextFieldFocused { 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 => IsTextFieldFocused = true);
renameTextfield.RegisterCallback<FocusOutEvent>(e =>
{
OnEditTextFinished();
IsTextFieldFocused = false;
});
}

public Label label => this.Q<Label>();
Expand Down Expand Up @@ -99,6 +106,19 @@ public static void CancelRename()
s_EditingItem?.OnEditTextFinished();
}

public void FocusOnRenameFinish()
{
if (m_IsEditing)
return;

//FocusOnRenameTextField() changes the focus to the renameTextfield explicitly, the focus needs to
//get moved again using a similar workaround when finished editing text
//Everything else has already been taken restored in OnEditTextFinished() but this has to happen after
//listView/treeView reclaims the focus in RedrawUI
label.Q<Label>().Focus();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some concern on this line.
If the rename end by a clic in an other area, like action maps list, does it steal the focus?

IsTextFieldFocused = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

IsTextFieldFocused should not be changed here. It should be driven by ui toolkit when the item lose focus. Is it necessary?

}

async void DelayCall()
{
await Task.Delay(120);
Expand Down