FIX: Fixed documentation of bindings with modifiers and UI Toolkit tooltips in the asset editor (ISBX-806) #2048

bmalrat · 2024-11-13T16:54:58Z

Description

The documentation of overrideModifiersNeedToBePressedFirst was not very clear and lead to miss configuration. https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-806

I added a Tooltip in the UI and fixed the Tooltips support for UI Toolkit version of the Input Actions Asset editor.
Please note that there is currently a limitation within UI Toolkit that doesn't display tooltip if the label of the field is truncated.

Testing status & QA

Tested on windows 6000.0.26f1

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:
Halo Effect:

Comments to reviewers

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

Checklist

Before review:

[X ] 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.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

…s in the asset editor - Fixed tooltip support in the UI Toolkit version of the Input Actions Asset editor. - Fixed documentation to clarify bindings with modifiers `overrideModifiersNeedToBePressedFirst` configuration [ISXB-806](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-806).

lyndon-unity · 2024-11-13T17:53:21Z

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/ButtonWithOneModifier.cs

        /// This parameter can be used to bypass this behavior and allow any timing between <see cref="modifier"/> and <see cref="button"/>.
        /// The only requirement is for them both to concurrently be in pressed state.
        /// </remarks>
+        [Tooltip("If checked, it will bypass the InputSettings.shortcutKeysConsumeInput setting and the modifier can be pressed after and the composite will still trigger")]


Is that correct? It sounds like the reverse logic to my reading of the variable.

I think this is the state:

// Ordering important
overrideModifiersNeedToBePressedFirst = true

// Ordering not important
overrideModifiersNeedToBePressedFirst = false

If that's not the case then the variable and text next to the checkbox would also need changing.

Current state is :
if InputSettings.shortcutKeysConsumeInput is false (the default)
// Ordering not important in all case

if InputSettings.shortcutKeysConsumeInput is true
// Ordering important
overrideModifiersNeedToBePressedFirst = false

// Ordering not important
overrideModifiersNeedToBePressedFirst = true

the name overrideModifiersNeedToBePressedFirst override the setting and doesn't really describe the composite behavior.

Wow that's confusing. Making it clearer with the tooltip is helpful but I'd prefer if we tidied this up further to make it much more intuitive.

Pauliusd01 · 2024-11-14T13:08:17Z

I see the tooltips just fine on one and two modifiers but I'm not even sure I understand the tooltip correctly it seems confusing. Maybe adding Ben is a good idea to proof read?

My suggestion would be to change it to:
"If enabled, this will override the Input Consumption setting, allowing the modifier key to be pressed while still triggering the composite action." (Maybe even mentioning where to find the setting in parentheses is a good idea)

I renamed the setting from a script one to the settings one, that I assume will be more familiar to people and rephrased it a bit.

Pauliusd01

Just updating status that I checked it, I can approve if my suggestion isn't good just let me know

lyndon-unity

Generally I approve this change.

Couple of minor typos to fix.
Would be useful to update the description image

Packages/com.unity.inputsystem/CHANGELOG.md

lyndon-unity · 2024-11-19T10:54:44Z

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/ButtonWithOneModifier.cs

+                // Legacy. We need to reference the obsolete member here so temporarily
+                // turn off the warning.
+#pragma warning disable CS0618
+                if (overrideModifiersNeedToBePressedFirst)


Looks like we are preserving old behaviour which is great to see.

lyndon-unity · 2024-11-19T10:56:35Z

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/ButtonWithOneModifier.cs

+#pragma warning restore CS0618
+                    modifiersOrder = ModifiersOrder.Unordered;
+                else
+                    modifiersOrder = InputSystem.settings.shortcutKeysConsumeInput ? ModifiersOrder.Ordered : ModifiersOrder.Unordered;


Is this changing the stored value from default to a specific value?
I'm thinking this should be on reading the value rather than changing what's stored.

That said this code was making a change directly before so its not altering behaviour

It doesn't changed the stored value. The class is not serialized within the classic unity serialization.
It's serialized in the inputaction asset in json.
like

{ "name": "Two Modifiers", "id": "9796175a-7495-4ded-9b01-e17eafba2eaf", "path": "TwoModifiers(overrideModifiersNeedToBePressedFirst=true,modifiersOrder=1)", "interactions": "", "processors": "", "groups": "", "action": "New action", "isComposite": true, "isPartOfComposite": false },

The value is only changed a runtime when the binding is setup

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/TwoModifiersComposite.cs

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/ButtonWithOneModifier.cs

…s-doc-and-tooltips

Packages/com.unity.inputsystem/CHANGELOG.md

Pauliusd01 · 2024-11-20T07:44:53Z

By the way, I see Publish to Internal Registry job is failing, probably means you will need to up the next version? @ekcoh

ekcoh

LGTM, since we any way make modifications in the area we might as well also be clear regarding any expected or rather non-expected ordering of modifiers. E.g. for Ctrl+Shift+B, is Ctrl expected to be pressed before Shift in any scenario or is the new setting applying to Modifier-group vs Trigger only. Also consider removing the obsolete member for any major revision forward ports.

ekcoh · 2024-11-22T07:58:45Z

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/ButtonWithTwoModifiers.cs

+            /// By default, if the setting <see cref="InputSettings.shortcutKeysConsumeInput"/> is enabled,
+            /// <see cref="modifier1"/> and <see cref="modifier2"/> are required to be in pressed state before or at the same
+            /// time that <see cref="button"/> goes into pressed state for the composite as a whole to trigger. This means that binding to,
+            /// for example, <c>Ctrl+Shift+B</c>, the <c>ctrl</c> <c>shift</c> keys have to be pressed before pressing the <c>B</c> key.


Great you are clarifying this, but still unclear whether ctrl and shift is ordered or not. I suggest either adding this information, e.g. if its a sequence or a set of pressed buttons.

ctrl and shift doesn't need to be in order. I will precise it.

Packages/com.unity.inputsystem/InputSystem/Actions/Composites/OneModifierComposite.cs

…omposites-with-modifiers-doc-and-tooltips # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md

ekcoh · 2024-11-27T16:32:21Z

By the way, I see Publish to Internal Registry job is failing, probably means you will need to up the next version? @ekcoh

Yes, it needs to be a minor version bump due to introducing API

…omposites-with-modifiers-doc-and-tooltips # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md

bmalrat requested review from Pauliusd01 and ekcoh November 13, 2024 16:56

lyndon-unity reviewed Nov 13, 2024

View reviewed changes

Pauliusd01 requested changes Nov 15, 2024

View reviewed changes

bmalrat added 3 commits November 15, 2024 12:15

Add modifiersOrder

39263a0

update changelog

a665583

fixed doc

70f1d9d

lyndon-unity approved these changes Nov 19, 2024

View reviewed changes

bmalrat added 2 commits November 19, 2024 09:11

fixed docs and spelling

3bb2c48

fixing documentation

1717281

Pauliusd01 self-requested a review November 20, 2024 06:33

Merge branch 'develop' into isxb-806-fix-for-composites-with-modifier…

7030469

…s-doc-and-tooltips

Pauliusd01 reviewed Nov 20, 2024

View reviewed changes

Packages/com.unity.inputsystem/CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

2c30068

Pauliusd01 self-requested a review November 20, 2024 06:43

Pauliusd01 approved these changes Nov 20, 2024

View reviewed changes

ekcoh approved these changes Nov 22, 2024

View reviewed changes

bmalrat added 3 commits November 22, 2024 11:06

improve documentation

c7e4482

Merge remote-tracking branch 'origin/develop' into isxb-806-fix-for-c…

a01fd2c

…omposites-with-modifiers-doc-and-tooltips # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md

fix format

39e0ed9

bmalrat added 2 commits November 27, 2024 12:42

Merge remote-tracking branch 'origin/develop' into isxb-806-fix-for-c…

9dbc242

…omposites-with-modifiers-doc-and-tooltips # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md

CHANGE: Bump version to 1.12.0

9f892fe

bmalrat requested a review from Pauliusd01 November 27, 2024 20:46

Pauliusd01 approved these changes Nov 28, 2024

View reviewed changes

Pauliusd01 merged commit be8287f into develop Nov 28, 2024
77 checks passed

Pauliusd01 deleted the isxb-806-fix-for-composites-with-modifiers-doc-and-tooltips branch November 28, 2024 07:49

FIX: Fixed documentation of bindings with modifiers and UI Toolkit tooltips in the asset editor (ISBX-806) #2048

FIX: Fixed documentation of bindings with modifiers and UI Toolkit tooltips in the asset editor (ISBX-806) #2048

Conversation

bmalrat commented Nov 13, 2024

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pauliusd01 commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pauliusd01 left a comment

Choose a reason for hiding this comment

Uh oh!

lyndon-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pauliusd01 commented Nov 20, 2024

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekcoh commented Nov 27, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Pauliusd01 commented Nov 14, 2024 •

edited

Loading