Skip to content
Open
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
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Removed 32-bit compilation check for HID on Windows players, which had no impact anymore. (ISX-2543)
- Migrated sample scenes to use Universal Render Pipeline (URP) with Built-in Render Pipeline fallback shaders. The URP package is now required to run the samples. (ISX-2343)
- Changed the UI for `Actions.inputactions` asset to use UI Toolkit framework.
- Changed the UI for `InputSystem.inputsettings` asset to use UI Toolkit framework.

### Added

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System;
using System.IO;
using UnityEditor;
using UnityEngine.UIElements;

namespace UnityEngine.InputSystem.Editor
{
Expand All @@ -19,7 +20,7 @@
InvalidPath,

/// <summary>
/// The dialog was cancelled by the user and the path is invalid.
/// The dialog was canceled by the user and the path is invalid.
Comment thread
josepmariapujol-unity marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low
The spelling in this comment was changed to 'canceled' (American English), but the corresponding enum member Cancelled (line 25) retains the British English spelling. To maintain consistency within the codebase, it is recommended to use the same spelling for both.

Suggested change
/// The dialog was canceled by the user and the path is invalid.
/// The dialog was cancelled by the user and the path is invalid.

🤖 Helpful? 👍/👎

/// </summary>
Cancelled,

Expand Down Expand Up @@ -82,12 +83,22 @@
return asset;
}

public static void DrawMakeActiveGui<T>(T current, T target, string targetName, string entity, Action<T> apply, bool allowAssignActive = true)
public static VisualElement CreateMakeActiveGui<T>(T current, T target, string targetName, string entity, Action<T> apply, bool allowAssignActive = true)
Comment thread
josepmariapujol-unity marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low
The targetName parameter is accepted by CreateMakeActiveGui but is not used within the method body or passed to PopulateMakeActiveGui. Since this is an internal utility class, consider removing it from the signature to simplify the API.

🤖 Helpful? 👍/👎

where T : ScriptableObject
{
var container = new VisualElement();
PopulateMakeActiveGui(container, current, target, entity, apply, allowAssignActive);
return container;
}

Check warning on line 92 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L89-L92

Added lines #L89 - L92 were not covered by tests

private static void PopulateMakeActiveGui<T>(VisualElement container, T current, T target, string entity, Action<T> apply, bool allowAssignActive)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium
The transition from IMGUI to UI Toolkit introduces a behavior regression where the UI remains stale if the active asset (e.g., InputSystem.settings) is changed from another window (like Project Settings) or via an Undo operation.

The previous IMGUI implementation re-evaluated InputSystem.settings on every repaint, ensuring the UI always reflected the current state. In the new UI Toolkit version, CreateInspectorGUI is called once, and the state only updates locally when the "Assign" button is clicked. If the setting is changed elsewhere, this inspector will continue to show incorrect information until the selection is changed and returned to.

Consider implementing a simple poll using container.schedule.Execute(...) or subscribing to an event (if available) to ensure the UI stays in sync with the global state.

🤖 Helpful? 👍/👎

where T : ScriptableObject
{
container.Clear();

Check warning on line 97 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L96-L97

Added lines #L96 - L97 were not covered by tests

if (current == target)
{
EditorGUILayout.HelpBox($"These actions are assigned as the {entity}.", MessageType.Info);
container.Add(new HelpBox($"These actions are assigned as the {entity}.", HelpBoxMessageType.Info));

Check warning on line 101 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L101

Added line #L101 was not covered by tests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high
The HelpBox VisualElement was introduced in Unity 2021.1. If the Input System package is intended to support older Unity versions (such as 2019.4 or 2020.3 LTS), this change will introduce compilation errors. Have you verified the minimum supported Unity version for this PR? If older versions must be supported, consider using an IMGUIContainer with EditorGUILayout.HelpBox or a conditional compilation check.

🤖 Helpful? 👍/👎

return;
}

Expand All @@ -96,11 +107,21 @@
currentlyActiveAssetsPath = AssetDatabase.GetAssetPath(current);
if (!string.IsNullOrEmpty(currentlyActiveAssetsPath))
currentlyActiveAssetsPath = $" The actions currently assigned as the {entity} are: {currentlyActiveAssetsPath}. ";
EditorGUILayout.HelpBox($"These actions are not assigned as the {entity} for the Input System. {currentlyActiveAssetsPath??""}", MessageType.Warning);
GUI.enabled = allowAssignActive;
if (GUILayout.Button($"Assign as the {entity}", EditorStyles.miniButton))

container.Add(new HelpBox(

Check warning on line 111 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L111

Added line #L111 was not covered by tests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium
For long asset paths, the HelpBox text might be clipped if it does not wrap. Consider setting the style's whiteSpace property to WhiteSpace.Normal to ensure the text wraps correctly, similar to the styling applied to the button in InputSettingsEditor.cs.

🤖 Helpful? 👍/👎

$"These actions are not assigned as the {entity} for the Input System. {currentlyActiveAssetsPath ?? ""}",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low
The current string construction in lines 109 and 112 results in redundant spaces in the displayed message (e.g., a double space after the first sentence and a trailing space at the end). Trimming these strings or adjusting the interpolation would provide a cleaner and more polished user interface.

🤖 Helpful? 👍/👎

HelpBoxMessageType.Warning));

var assignButton = new Button(() =>
{

Check warning on line 116 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L115-L116

Added lines #L115 - L116 were not covered by tests
apply(target);
GUI.enabled = true;
PopulateMakeActiveGui(container, target, target, entity, apply, allowAssignActive);
})

Check warning on line 119 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L118-L119

Added lines #L118 - L119 were not covered by tests
{
text = $"Assign as the {entity}"
};
assignButton.SetEnabled(allowAssignActive);
container.Add(assignButton);

Check warning on line 124 in Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/InputAssetEditorUtils.cs#L123-L124

Added lines #L123 - L124 were not covered by tests
}

public static bool IsValidFileExtension(string path)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -489,17 +489,21 @@
[CustomEditor(typeof(InputSettings))]
internal class InputSettingsEditor : UnityEditor.Editor
{
public override void OnInspectorGUI()
public override VisualElement CreateInspectorGUI()
{
EditorGUILayout.Space();
var root = new VisualElement();

Check warning on line 494 in Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs#L494

Added line #L494 was not covered by tests

if (GUILayout.Button("Open Input Settings Window", GUILayout.Height(30)))
InputSettingsProvider.Open();
var openButton = new Button(() => InputSettingsProvider.Open())

Check warning on line 496 in Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs#L496

Added line #L496 was not covered by tests
{
text = "Open Input Settings Window",
style = { minHeight = 30, whiteSpace = WhiteSpace.Normal }
};
root.Add(openButton);

Check warning on line 501 in Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs#L501

Added line #L501 was not covered by tests

EditorGUILayout.Space();
root.Add(InputAssetEditorUtils.CreateMakeActiveGui(InputSystem.settings, target as InputSettings,
target.name, "settings", (value) => InputSystem.settings = value));

Check warning on line 504 in Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs#L503-L504

Added lines #L503 - L504 were not covered by tests

InputAssetEditorUtils.DrawMakeActiveGui(InputSystem.settings, target as InputSettings,
target.name, "settings", (value) => InputSystem.settings = value);
return root;

Check warning on line 506 in Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs#L506

Added line #L506 was not covered by tests
}

protected override bool ShouldHideOpenButton()
Expand Down
Loading