Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ 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 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).

### 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 @@ -79,14 +79,16 @@ public class ButtonWithOneModifier : InputBindingComposite<float>
/// still trigger. Default is false.
/// </summary>
/// <remarks>
/// By default, <see cref="modifier"/> is required to be in pressed state before or at the same time that <see cref="button"/>
/// By default, if the setting <see cref="InputSettings.shortcutKeysConsumeInput"/> is enabled,
/// <see cref="modifier"/> is 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>Shift+B</c>,
/// the <c>shift</c> key has to be pressed before pressing the <c>B</c> key. This is the behavior usually expected with
/// keyboard shortcuts.
///
/// 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")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

public bool overrideModifiersNeedToBePressedFirst;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,16 @@ public class ButtonWithTwoModifiers : InputBindingComposite<float>
/// and the composite will still trigger. Default is false.
/// </summary>
/// <remarks>
/// By default, <see cref="modifier1"/> and <see cref="modifier2"/> are required to be in pressed state before or at the same
/// 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.
/// This is the behavior usually expected with keyboard shortcuts.
///
/// This parameter can be used to bypass this behavior and allow any timing between <see cref="modifier1"/>, <see cref="modifier2"/>,
/// and <see cref="button"/>. The only requirement is for all of them to concurrently be in pressed state.
/// </remarks>
[Tooltip("If checked, it will bypass the InputSettings.shortcutKeysConsumeInput setting and the modifiers can be pressed after and the composite will still trigger")]
public bool overrideModifiersNeedToBePressedFirst;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ public class OneModifierComposite : InputBindingComposite
/// Default value is <c>false</c>.
/// </summary>
/// <remarks>
/// By default, if <see cref="binding"/> is bound to only <see cref="Controls.ButtonControl"/>s, then the composite requires
/// By default, if the setting <see cref="InputSettings.shortcutKeysConsumeInput"/> is enabled,
/// if <see cref="binding"/> is bound to only <see cref="Controls.ButtonControl"/>s, then the composite requires
/// <see cref="modifier"/> to be pressed <em>before</em> pressing <see cref="binding"/>. This means that binding to, for example,
/// <c>Ctrl+B</c>, the <c>ctrl</c> keys have to be pressed before pressing the <c>B</c> key. This is the behavior usually expected
/// with keyboard shortcuts.
Expand All @@ -101,6 +102,7 @@ public class OneModifierComposite : InputBindingComposite
/// is bound to, any press sequence is acceptable. For the example binding to <c>Ctrl+B</c>, it would mean that pressing <c>B</c> and
/// only then pressing <c>Ctrl</c> will still trigger the binding.
/// </remarks>
[Tooltip("If checked, it will bypass the InputSettings.shortcutKeysConsumeInput setting and the modifier can be pressed after and the composite will still trigger")]
public bool overrideModifiersNeedToBePressedFirst;

private int m_ValueSizeInBytes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ public class TwoModifiersComposite : InputBindingComposite
/// Default value is <c>false</c>.
/// </summary>
/// <remarks>
/// By default, if <see cref="binding"/> is bound to only <see cref="Controls.ButtonControl"/>s, then the composite requires
/// By default, if the setting <see cref="InputSettings.shortcutKeysConsumeInput"/> is enabled,
/// if <see cref="binding"/> is bound to only <see cref="Controls.ButtonControl"/>s, then the composite requires
/// both <see cref="modifier1"/> and <see cref="modifier2"/> to be pressed <em>before</em> pressing <see cref="binding"/>.
/// This means that binding to, for example, <c>Ctrl+Shift+B</c>, the <c>ctrl</c> and <c>shift</c> keys have to be pressed
/// before pressing the <c>B</c> key. This is the behavior usually expected with keyboard shortcuts.
Expand All @@ -103,6 +104,7 @@ public class TwoModifiersComposite : InputBindingComposite
/// is bound to, any press sequence is acceptable. For the example binding to <c>Ctrl+Shift+B</c>, it would mean that pressing
/// <c>B</c> and only then pressing <c>Ctrl</c> and <c>Shift</c> will still trigger the binding.
/// </remarks>
[Tooltip("If checked, it will bypass the InputSettings.shortcutKeysConsumeInput setting and the modifiers can be pressed after and the composite will still trigger")]
public bool overrideModifiersNeedToBePressedFirst;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,46 +280,47 @@ void OnEditEnd()
{
var intValue = parameter.value.value.ToInt32();
var field = new DropdownField(label.text, parameter.enumNames.Select(x => x.text).ToList(), intValue);
field.tooltip = label.tooltip;
field.RegisterValueChangedCallback(evt => OnValueChanged(ref parameter, field.index, closedIndex));
field.RegisterCallback<BlurEvent>(_ => OnEditEnd());
root.Add(field);
}
else if (parameter.value.type == TypeCode.Int64 || parameter.value.type == TypeCode.UInt64)
{
var longValue = parameter.value.value.ToInt64();
var field = new LongField(label.text) { value = longValue };
var field = new LongField(label.text) { value = longValue, tooltip = label.tooltip };
field.RegisterValueChangedCallback(evt => OnValueChanged(ref parameter, evt.newValue, closedIndex));
field.RegisterCallback<BlurEvent>(_ => OnEditEnd());
root.Add(field);
}
else if (parameter.value.type.IsInt())
{
var intValue = parameter.value.value.ToInt32();
var field = new IntegerField(label.text) { value = intValue };
var field = new IntegerField(label.text) { value = intValue, tooltip = label.tooltip };
field.RegisterValueChangedCallback(evt => OnValueChanged(ref parameter, evt.newValue, closedIndex));
field.RegisterCallback<BlurEvent>(_ => OnEditEnd());
root.Add(field);
}
else if (parameter.value.type == TypeCode.Single)
{
var floatValue = parameter.value.value.ToSingle();
var field = new FloatField(label.text) { value = floatValue };
var field = new FloatField(label.text) { value = floatValue, tooltip = label.tooltip };
field.RegisterValueChangedCallback(evt => OnValueChanged(ref parameter, evt.newValue, closedIndex));
field.RegisterCallback<BlurEvent>(_ => OnEditEnd());
root.Add(field);
}
else if (parameter.value.type == TypeCode.Double)
{
var floatValue = parameter.value.value.ToDouble();
var field = new DoubleField(label.text) { value = floatValue };
var field = new DoubleField(label.text) { value = floatValue, tooltip = label.tooltip };
field.RegisterValueChangedCallback(evt => OnValueChanged(ref parameter, evt.newValue, closedIndex));
field.RegisterCallback<BlurEvent>(_ => OnEditEnd());
root.Add(field);
}
else if (parameter.value.type == TypeCode.Boolean)
{
var boolValue = parameter.value.value.ToBoolean();
var field = new Toggle(label.text) { value = boolValue };
var field = new Toggle(label.text) { value = boolValue, tooltip = label.tooltip };
field.RegisterValueChangedCallback(evt => OnValueChanged(ref parameter, evt.newValue, closedIndex));
field.RegisterValueChangedCallback(_ => OnEditEnd());
root.Add(field);
Expand Down