X Tutup
Skip to content

FIX: UI inconsistencies - Single helpbox for repetitive default settings mentions#2372

Open
josepmariapujol-unity wants to merge 10 commits intodevelopfrom
input/properties-input-actions
Open

FIX: UI inconsistencies - Single helpbox for repetitive default settings mentions#2372
josepmariapujol-unity wants to merge 10 commits intodevelopfrom
input/properties-input-actions

Conversation

@josepmariapujol-unity
Copy link
Collaborator

@josepmariapujol-unity josepmariapujol-unity commented Mar 9, 2026

Description

Previously, the interface displayed repetitive messages along with multiple links to the settings, which made the UI more cluttered and harder to read. This update replaces those repeated messages with a single HelpBox that clearly explains the default behavior and provides the relevant guidance in one place.

Changes
• Reduced UI clutter and improved readability.
• Kept the link to the relevant settings within the unified HelpBox.

Result
• Clearer and more maintainable UI messaging.
• A more consistent user experience with less duplicated information.

screenshot-5

Before:
Before

After:
After

https://jira.unity3d.com/browse/ISX-2340

Testing status & QA

Toggle and untoggle the buttons and check the names in the helpbox to make sure it works.

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.

  • Complexity: 1
  • Halo Effect: 2

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

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.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • 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.

@josepmariapujol-unity josepmariapujol-unity self-assigned this Mar 9, 2026
@josepmariapujol-unity josepmariapujol-unity changed the title Combining names for helpbox FIX: UI inconsistencies - Single helpbox for repetitive default settings mentions Mar 9, 2026
@josepmariapujol-unity josepmariapujol-unity marked this pull request as ready for review March 9, 2026 15:57
Copy link
Contributor

@u-pr u-pr bot left a comment

Choose a reason for hiding this comment

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

Great
The PR quality is great. I found a few minor issues regarding code structure and potential dead code.

  • Moving shared footer logic to a non-generic base class can reduce metadata bloat.
  • Using events instead of single callbacks prevents overwriting.
  • Clean up unused parameters and potentially dead member variables.

🤖 Helpful? 👍/👎

m_OnUseDefaultChanged = callback;
}

internal static void AddSharedDefaultSettingsFooter(VisualElement root,
Copy link
Contributor

Choose a reason for hiding this comment

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

medium
The AddSharedDefaultSettingsFooter method is a static method defined within a nested class of the generic InputParameterEditor<TValue>. Since the logic for creating and refreshing the footer does not depend on the generic type parameter TValue, this results in redundant code generation and metadata for every distinct type of interaction or processor that uses it. Consider moving CustomOrDefaultSetting (and this helper method) to the non-generic InputParameterEditor base class to improve maintainability and reduce assembly size.

🤖 Helpful? 👍/👎

@codecov-github-com
Copy link

codecov-github-com bot commented Mar 9, 2026

Codecov Report

Attention: Patch coverage is 0% with 47 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...tsystem/InputSystem/Editor/InputParameterEditor.cs 0.00% 40 Missing ⚠️
...nputSystem/Actions/Interactions/HoldInteraction.cs 0.00% 1 Missing ⚠️
...System/Actions/Interactions/MultiTapInteraction.cs 0.00% 1 Missing ⚠️
...putSystem/Actions/Interactions/PressInteraction.cs 0.00% 1 Missing ⚠️
...tSystem/Actions/Interactions/SlowTapInteraction.cs 0.00% 1 Missing ⚠️
...InputSystem/Actions/Interactions/TapInteraction.cs 0.00% 1 Missing ⚠️
...ystem/Controls/Processors/AxisDeadzoneProcessor.cs 0.00% 1 Missing ⚠️
...stem/Controls/Processors/StickDeadzoneProcessor.cs 0.00% 1 Missing ⚠️
@@             Coverage Diff             @@
##           develop    #2372      +/-   ##
===========================================
- Coverage    77.90%   77.89%   -0.01%     
===========================================
  Files          476      481       +5     
  Lines        97613    97673      +60     
===========================================
+ Hits         76048    76086      +38     
- Misses       21565    21587      +22     
Files with missing lines Coverage Δ
...nputSystem/Actions/Interactions/HoldInteraction.cs 58.49% <0.00%> (-1.13%) ⬇️
...System/Actions/Interactions/MultiTapInteraction.cs 60.74% <0.00%> (-0.46%) ⬇️
...putSystem/Actions/Interactions/PressInteraction.cs 65.44% <0.00%> (-0.49%) ⬇️
...tSystem/Actions/Interactions/SlowTapInteraction.cs 44.89% <0.00%> (-0.94%) ⬇️
...InputSystem/Actions/Interactions/TapInteraction.cs 62.31% <0.00%> (-0.92%) ⬇️
...ystem/Controls/Processors/AxisDeadzoneProcessor.cs 42.55% <0.00%> (-0.93%) ⬇️
...stem/Controls/Processors/StickDeadzoneProcessor.cs 52.63% <0.00%> (-0.94%) ⬇️
...tsystem/InputSystem/Editor/InputParameterEditor.cs 16.52% <0.00%> (-0.44%) ⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@josepmariapujol-unity
Copy link
Collaborator Author

/review

@josepmariapujol-unity
Copy link
Collaborator Author

/crc

@u-pr
Copy link
Contributor

u-pr bot commented Mar 10, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

ISX-2340 - Partially compliant

Compliant requirements:

  • Consolidate repetitive default-setting explanation/messages and links into a single HelpBox.

Non-compliant requirements:

  • Fix Auto-Save toggle padding issues (remove unnecessary competing inline styles; parent toggle width 68px; right padding 4px; label min-width 120px).
  • Add “Shadow” section headers using component-like headers that allow for buttons.
  • Use an add icon for Add buttons (and “add more” icon when multiple add actions exist).
  • Use toolbar style for toolbar (regular 12pt font, height 20px).
  • Use reorderable lists for action properties (so remove/move options come from the list).

Requires further human verification:

  • Validate the unified HelpBox behavior and wording across all affected inspectors (interactions/processors) in the Editor UI, including toggling defaults on/off and ensuring the settings link opens the correct page.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

The change is conceptually small but touches shared editor UI plumbing across multiple interactions/processors and introduces event-driven UI behavior that needs careful validation in Editor.

🏅 Score: 84

Solid consolidation that reduces duplication, but the new shared footer/event wiring needs extra scrutiny for lifecycle/unsubscribe safety and null-safety around callbacks.

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Lifecycle Leak

AddSharedDefaultSettingsFooter subscribes to onUseDefaultChanged for each setting but never unsubscribes; if these UIElements/settings are recreated frequently, this can retain references and cause leaks or duplicate refresh calls—consider unsubscribing on detach or using a disposable pattern tied to footerContainer lifecycle.

internal event Action onUseDefaultChanged;

internal static void AddSharedDefaultSettingsFooter(VisualElement root,
    IReadOnlyList<CustomOrDefaultSetting> settings)
{
    if (settings == null || settings.Count == 0)
        return;

    var footerContainer = new VisualElement();
    var helpBox = new HelpBox("", HelpBoxMessageType.None);
    var buttonContainer = new VisualElement { style = { flexDirection = FlexDirection.RowReverse } };
    var openInputSettingsButton = new Button(InputSettingsProvider.Open)
    {
        text = EditorGUIUtility.TrTextContent("Open Input Settings").text
    };
    openInputSettingsButton.AddToClassList("open-settings-button");
    buttonContainer.Add(openInputSettingsButton);
    footerContainer.Add(helpBox);
    footerContainer.Add(buttonContainer);

    void RefreshFooter()
    {
        var namesInUse = new List<string>();
        foreach (var s in settings)
        {
            if (s.UseDefaultValue && s.DefaultComesFromInputSettings)
                namesInUse.Add(s.DefaultName);
        }
        if (namesInUse.Count > 0)
        {
            var quotedNames = new List<string>(namesInUse.Count);
            foreach (var n in namesInUse)
                quotedNames.Add($"\"{n}\"");
            var combinedNames = string.Join(", ", quotedNames);
            helpBox.text = EditorGUIUtility.TrTextContent(
                $"Uses {combinedNames} set in project-wide input settings.").text;
            footerContainer.style.display = DisplayStyle.Flex;
        }
        else
        {
            footerContainer.style.display = DisplayStyle.None;
        }
    }

    foreach (var s in settings)
        s.onUseDefaultChanged += RefreshFooter;

    RefreshFooter();
    root.Add(footerContainer);
Null Safety

ToggleUseDefaultValue calls onChangedCallback.Invoke() without a null-check; if any call sites pass null, this will throw at runtime—consider using onChangedCallback?.Invoke() (consistent with other code paths).

private void ToggleUseDefaultValue(ChangeEvent<bool> evt, Action onChangedCallback)
{
    if (evt.newValue != m_UseDefaultValue)
    {
        m_SetValue(!evt.newValue ? m_GetDefaultValue() : m_DefaultInitializedValue);
        onChangedCallback.Invoke();
    }

    m_UseDefaultValue = evt.newValue;
    m_FloatField?.SetEnabled(!m_UseDefaultValue);
    onUseDefaultChanged?.Invoke();
}
Changelog Wording

The added entry reads awkwardly (“Fixed simplified…”); consider rephrasing to something like “Simplified default-setting messaging by consolidating repetitive messages into a single HelpBox.”

- Fixed simplified default setting messaging by consolidating repetitive messages into a single HelpBox.
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

Copy link
Contributor

@u-pr u-pr bot left a comment

Choose a reason for hiding this comment

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

Great
The review highlights a regression in IMGUI feature parity. While consolidating default-setting messaging for UIElements is beneficial, equivalent functionality should be maintained for IMGUI fallback paths to ensure consistency across the editor.

🤖 Helpful? 👍/👎

Comment on lines +416 to 418
if (useDefaultChanged)
onUseDefaultChanged?.Invoke();
EditorGUILayout.EndHorizontal();
Copy link
Contributor

Choose a reason for hiding this comment

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

medium
By removing the HelpBox and the "Open Input Settings" button from OnGUI(), this messaging is completely lost for IMGUI users.

Since AddSharedDefaultSettingsFooter is strictly for UIElements, any custom parameter editors or third-party code that still rely on OnGUI() (which the Input System still supports as a fallback) will no longer display the help text explaining what the default value is, nor provide the button to open the settings.

Have you considered adding a shared footer equivalent for IMGUI, or preserving the individual messaging here so that the IMGUI fallback retains feature parity?

🤖 Helpful? 👍/👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

X Tutup