Input/settings uitoolkit josep#2389
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces several significant issues that could lead to crashes or broken functionality in the Input System settings UI. Specifically, there are high-importance risks related to NullReferenceException when settings assets are missing and state synchronization issues in the new UIToolkit-based UI.
Summary of findings:
- 3 High importance bugs related to null references and stale data in UI Toolkit.
- 1 Medium importance issue regarding a redundant UI container.
- 3 Low importance suggestions for code cleanup, performance, and portability.
🤖 Helpful? 👍/👎
| private void DrawSupportedDevicesGUI() | ||
| { | ||
| InitializeWithCurrentSettingsIfNecessary(); | ||
|
|
||
| using (new EditorGUI.DisabledScope(m_AvailableInputSettingsAssets.Length == 0)) | ||
| { | ||
| Debug.Assert(m_Settings != null); | ||
| m_SupportedDevices.DoLayoutList(); | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two critical issues in this extracted method:
- NullReferenceException when no settings exist: If
m_Settingsis null,m_SupportedDeviceswill also be null, causing aNullReferenceExceptionatm_SupportedDevices.DoLayoutList(). An early returnif (m_Settings == null) return;is required. - Missing Change Checks: In the fallback IMGUI path (
DrawSettingsGUI(false)),m_SupportedDevices.DoLayoutList()correctly falls inside theEditorGUI.BeginChangeCheck()andEditorGUI.EndChangeCheck()blocks, triggeringApply()upon modification. However, these change checks were omitted here. Without them, modifying the Supported Devices list in UIToolkit mode will not callm_Settings.OnChange(), so the live Input System will not reload the changes.
Suggested fix:
private void DrawSupportedDevicesGUI()
{
InitializeWithCurrentSettingsIfNecessary();
if (m_Settings == null) return;
using (new EditorGUI.DisabledScope(m_AvailableInputSettingsAssets.Length == 0))
{
m_SettingsObject.Update();
EditorGUI.BeginChangeCheck();
m_SupportedDevices.DoLayoutList();
if (EditorGUI.EndChangeCheck())
Apply();
}
}🤖 Helpful? 👍/👎
| m_iOSProvider.CreateGUI(lowerSectionsContainer, () => | ||
| { | ||
| Apply(); | ||
| RefreshUIToolkitHeaderState(); | ||
| }); |
There was a problem hiding this comment.
If m_Settings is null (e.g., in a project where an Input Settings asset hasn't been created yet), InitializeWithCurrentSettingsIfNecessary() will exit early without instantiating m_iOSProvider. This leads to a NullReferenceException at m_iOSProvider.CreateGUI(...), completely breaking the Settings UI.
Furthermore, simply adding a null-conditional operator (m_iOSProvider?.CreateGUI) is not a complete fix. Because BuildUI() is only invoked once during OnActivate(), if a user creates a settings asset later, the iOS UI elements will remain missing from the visual tree until the settings window is reopened.
To fix this properly, InputSettingsiOSProvider should be instantiated regardless of whether m_Settings is null, and its Update(SerializedObject) method should gracefully handle a null parent. This ensures the VisualElements are always built, added to the UI tree, and then properly bound when settings are created.
🤖 Helpful? 👍/👎
| if (m_HeaderContainer == null) | ||
| return; | ||
|
|
||
| var hasSettings = m_SettingsObject != null; | ||
| var hasSettingsAsset = m_AvailableInputSettingsAssets != null && m_AvailableInputSettingsAssets.Length != 0; |
There was a problem hiding this comment.
Because the UIToolkit fields are manually updated by reading from SerializedProperty (e.g., property.floatValue in UpdateFloatField) rather than using data binding (Bind()), you must call m_SettingsObject?.Update(); here before reading the properties.
Without it, if the settings are modified externally (e.g., via script) or reversed via Undo/Redo, the SerializedObject will retain stale data. Consequently, RefreshUIToolkitHeaderState() will populate the UI with outdated values. Adding m_SettingsObject?.Update(); ensures the UI reflects the most recent state of the underlying InputSettings object.
🤖 Helpful? 👍/👎
| HelpBoxMessageType.None); | ||
| lowerSectionsContainer.Add(m_ShortcutKeysConsumeInputsHelpBox); | ||
|
|
||
| m_IMGUIContainer = new IMGUIContainer(() => DrawSettingsGUI(includeUIToolkitHeader: true)); |
There was a problem hiding this comment.
This IMGUIContainer appears to be redundant. It calls DrawSettingsGUI(includeUIToolkitHeader: true), which skips all its internal drawing and change-checking logic due to the !includeUIToolkitHeader guards. This makes the container a no-op that still consumes resources during UI layout and repaint cycles.
🤖 Helpful? 👍/👎
| } | ||
| } | ||
|
|
||
| private void DrawSettingsGUI(bool includeUIToolkitHeader) |
There was a problem hiding this comment.
The DrawSettingsGUI method is now fragmented with multiple if (!includeUIToolkitHeader) blocks that wrap almost the entire body. Since the method effectively performs no rendering or logic when the flag is true, it would be much cleaner to use an early return at the start of the method or handle the mode switching entirely at the call sites.
🤖 Helpful? 👍/👎
| return; | ||
| } | ||
|
|
||
| dropdown.choices = property.enumDisplayNames.ToList(); |
There was a problem hiding this comment.
Calling property.enumDisplayNames.ToList() in UpdateDropdownChoices creates a new list allocation every time the UI is refreshed (which happens on every settings change, undo, or redo). To reduce GC pressure, consider caching the list or only updating it if the underlying property's enum names have actually changed.
🤖 Helpful? 👍/👎
| private static void OpenUpdateModeDocumentation() | ||
| { | ||
| var link = new Uri(InputSystem.kDocUrl + "/manual/Settings.html#update-mode"); | ||
| System.Diagnostics.Process.Start(link.AbsoluteUri); |
There was a problem hiding this comment.
Have you considered using Application.OpenURL(link.AbsoluteUri) instead of System.Diagnostics.Process.Start? Application.OpenURL is the standard Unity API for opening external links and is more portable across different editor environments (Windows, macOS, Linux).
🤖 Helpful? 👍/👎
1 participant
Description
Please fill this section with a description what the pull request is trying to address and what changes were made.
Testing status & QA
Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.