Skip to content

FIX: Avoid errors logged when opening settings menu (ISX-1721) #1825

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Feb 2, 2024
Merged

FIX: Avoid errors logged when opening settings menu (ISX-1721) #1825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
762b5aa
FIX: Avoid errors logged when opening settings menu (ISX-1721)
graham-huws Jan 29, 2024
4d2c0c6
Address feedback: Consolidate root element for views and bugfix logic.
graham-huws Feb 1, 2024
7c4a085
Merge branch 'develop' into ISX-1721-settings-error-in-console
graham-huws Feb 1, 2024
ba14a8c
Rename after merging develop branch.
graham-huws Feb 1, 2024
96fa3a2
Fix scrollview access sometimes failing.
graham-huws Feb 1, 2024
020f837
Match existing styles, including having readonly element variables ra…
graham-huws Feb 1, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed missing name in window title for Input Action assets.
- Fixed showing action properties view when there were no actions.
- Fixed "Listen" functionality for selecting an input sometimes expecting the wrong input type.
- Fixed console errors that can be produced when opening input package settings from the Inspector.
- Fixed InputManager.asset file growing in size on each Reset call.

## [1.8.0-pre.2] - 2023-11-09
Expand Down
17 changes: 7 additions & 10 deletions Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionMapsView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@ namespace UnityEngine.InputSystem.Editor
internal class ActionMapsView : ViewBase<ActionMapsView.ViewState>
{
public ActionMapsView(VisualElement root, StateContainer stateContainer)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;

m_ListView = m_Root?.Q<ListView>("action-maps-list-view");
m_ListView = root.Q<ListView>("action-maps-list-view");
m_ListView.selectionType = UIElements.SelectionType.Single;

m_ListViewSelectionChangeFilter = new CollectionViewSelectionChangeFilter(m_ListView);
Expand Down Expand Up @@ -61,12 +59,11 @@ public ActionMapsView(VisualElement root, StateContainer stateContainer)
CreateSelector(s => new ViewStateCollection<string>(Selectors.GetActionMapNames(s)),
(actionMapNames, state) => new ViewState(Selectors.GetSelectedActionMap(state), actionMapNames));

addActionMapButton.clicked += AddActionMap;
m_AddActionMapButton = root.Q<Button>("add-new-action-map-button");
m_AddActionMapButton.clicked += AddActionMap;
ContextMenu.GetContextMenuForActionMapListView(this, m_ListView.parent);
}

private Button addActionMapButton => m_Root?.Q<Button>("add-new-action-map-button");

public override void RedrawUI(ViewState viewState)
{
m_ListView.itemsSource = viewState.actionMapNames?.ToList() ?? new List<string>();
Expand All @@ -81,7 +78,7 @@ public override void RedrawUI(ViewState viewState)

public override void DestroyView()
{
addActionMapButton.clicked -= AddActionMap;
m_AddActionMapButton.clicked -= AddActionMap;
}

private void RenameNewActionMaps()
Expand Down Expand Up @@ -183,8 +180,8 @@ private void OnValidateCommand(ValidateCommandEvent evt)

private readonly CollectionViewSelectionChangeFilter m_ListViewSelectionChangeFilter;
private bool m_EnterRenamingMode;
private readonly VisualElement m_Root;
private ListView m_ListView;
private readonly ListView m_ListView;
private readonly Button m_AddActionMapButton;

internal class ViewState
{
Expand Down
12 changes: 5 additions & 7 deletions ...es/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionPropertiesView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,11 @@ namespace UnityEngine.InputSystem.Editor
{
internal class ActionPropertiesView : ViewBase<(SerializedInputAction?, List<string>)>
{
private readonly VisualElement m_Root;
private readonly Foldout m_ParentFoldout;

public ActionPropertiesView(VisualElement root, Foldout foldout, StateContainer stateContainer)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;
m_ParentFoldout = foldout;

// TODO: Consider IEquatable<T> and how to compare selector data
Expand All @@ -37,7 +35,7 @@ public override void RedrawUI((SerializedInputAction ? , List<string>) viewState
m_ParentFoldout.text = "Action";
var inputAction = viewState.Item1.Value;

m_Root.Clear();
rootElement.Clear();

var actionType = new EnumField("Action Type", inputAction.type)
{
Expand All @@ -47,7 +45,7 @@ public override void RedrawUI((SerializedInputAction ? , List<string>) viewState
{
Dispatch(Commands.ChangeActionType(inputAction, (InputActionType)evt.newValue));
});
m_Root.Add(actionType);
rootElement.Add(actionType);

if (inputAction.type != InputActionType.Button)
{
Expand All @@ -65,7 +63,7 @@ public override void RedrawUI((SerializedInputAction ? , List<string>) viewState
{
Dispatch(Commands.ChangeActionControlType(inputAction, controlType.index));
});
m_Root.Add(controlType);
rootElement.Add(controlType);
}

if (inputAction.type != InputActionType.Value)
Expand All @@ -79,7 +77,7 @@ public override void RedrawUI((SerializedInputAction ? , List<string>) viewState
{
Dispatch(Commands.ChangeInitialStateCheck(inputAction, evt.newValue));
});
m_Root.Add(initialStateCheck);
rootElement.Add(initialStateCheck);
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionsTreeView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ namespace UnityEngine.InputSystem.Editor
/// </summary>
internal class ActionsTreeView : ViewBase<ActionsTreeView.ViewState>
{
private readonly VisualElement m_Root;
private readonly TreeView m_ActionsTreeView;
private Button addActionButton => m_Root?.Q<Button>("add-new-action-button");
private readonly Button m_AddActionButton;
private readonly ScrollView m_PropertiesScrollview;

Copy link
Collaborator Author

@graham-huws graham-huws Feb 1, 2024

Choose a reason for hiding this comment

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

@ritamerkl, is moving addActionButton around here okay? I wasn't sure why it was set up differently to eg. m_ActionsTreeView. I made the change because otherwise it seemed to be accessing the root too early when following the repro steps for the bug which this PR fixes. ("Expected a visual element called 'add-new-action-button' of type 'UnityEngine.UIElements.Button' to exist but none was found.")

Copy link
Collaborator

@ekcoh ekcoh Feb 1, 2024

Choose a reason for hiding this comment

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

To me the change makes sense but would be good to get info from @ritamerkl whether there was any reason to do this lazily via getter indirection. If not I propose we look it up once and case it as a readonly var as suggested by @graham-huws.

Copy link
Collaborator

@ritamerkl ritamerkl Feb 1, 2024

Choose a reason for hiding this comment

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

Ok for me!

private bool m_RenameOnActionAdded;
private readonly CollectionViewSelectionChangeFilter m_ActionsTreeViewSelectionChangeFilter;
Expand All @@ -27,11 +27,11 @@ internal class ActionsTreeView : ViewBase<ActionsTreeView.ViewState>
private Dictionary<Guid, int> m_GuidToTreeViewId;

public ActionsTreeView(VisualElement root, StateContainer stateContainer)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;

m_ActionsTreeView = m_Root.Q<TreeView>("actions-tree-view");
m_AddActionButton = root.Q<Button>("add-new-action-button");
m_PropertiesScrollview = root.Q<ScrollView>("properties-scrollview");
m_ActionsTreeView = root.Q<TreeView>("actions-tree-view");
//assign unique viewDataKey to store treeView states like expanded/collapsed items - make it unique to avoid conflicts with other TreeViews
m_ActionsTreeView.viewDataKey = "InputActionTreeView " + stateContainer.GetState().serializedObject.targetObject.GetInstanceID();
m_GuidToTreeViewId = new Dictionary<Guid, int>();
Expand Down Expand Up @@ -147,7 +147,7 @@ public ActionsTreeView(VisualElement root, StateContainer stateContainer)
};
});

addActionButton.clicked += AddAction;
m_AddActionButton.clicked += AddAction;
}

private int GetSelectedElementId(InputActionsEditorState state, List<TreeViewItemData<ActionOrBindingData>> treeData)
Expand Down Expand Up @@ -188,7 +188,7 @@ private int GetComponentOrBindingID(List<TreeViewItemData<ActionOrBindingData>>

public override void DestroyView()
{
addActionButton.clicked -= AddAction;
m_AddActionButton.clicked -= AddAction;
}

public override void RedrawUI(ViewState viewState)
Expand All @@ -202,10 +202,10 @@ public override void RedrawUI(ViewState viewState)
m_ActionsTreeView.ScrollToItemById(viewState.newElementID);
}
RenameNewAction(viewState.newElementID);;
addActionButton.SetEnabled(viewState.actionMapCount > 0);
m_AddActionButton.SetEnabled(viewState.actionMapCount > 0);

// Don't want to show action properties if there's no actions.
m_Root.Q<VisualElement>("properties-scrollview").visible = m_ActionsTreeView.GetTreeCount() > 0;
m_PropertiesScrollview.visible = m_ActionsTreeView.GetTreeCount() > 0;
}

private void RenameNewAction(int id)
Expand Down
16 changes: 7 additions & 9 deletions ...s/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/BindingPropertiesView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ namespace UnityEngine.InputSystem.Editor
{
internal class BindingPropertiesView : ViewBase<BindingPropertiesView.ViewState>
{
private readonly VisualElement m_Root;
private readonly Foldout m_ParentFoldout;
private CompositeBindingPropertiesView m_CompositeBindingPropertiesView;
private CompositePartBindingPropertiesView m_CompositePartBindingPropertiesView;

public BindingPropertiesView(VisualElement root, Foldout foldout, StateContainer stateContainer)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;
m_ParentFoldout = foldout;

CreateSelector(state => state.selectedBindingIndex,
Expand All @@ -37,7 +35,7 @@ public override void RedrawUI(ViewState viewState)
if (selectedBindingIndex == -1)
return;

m_Root.Clear();
rootElement.Clear();

var binding = viewState.selectedBinding;
if (!binding.HasValue)
Expand All @@ -47,11 +45,11 @@ public override void RedrawUI(ViewState viewState)
if (binding.Value.isComposite)
{
m_ParentFoldout.text = "Composite";
m_CompositeBindingPropertiesView = CreateChildView(new CompositeBindingPropertiesView(m_Root, stateContainer));
m_CompositeBindingPropertiesView = CreateChildView(new CompositeBindingPropertiesView(rootElement, stateContainer));
}
else if (binding.Value.isPartOfComposite)
{
m_CompositePartBindingPropertiesView = CreateChildView(new CompositePartBindingPropertiesView(m_Root, stateContainer));
m_CompositePartBindingPropertiesView = CreateChildView(new CompositePartBindingPropertiesView(rootElement, stateContainer));
DrawControlSchemeToggles(viewState, binding.Value);
}
else
Expand All @@ -64,7 +62,7 @@ public override void RedrawUI(ViewState viewState)
controlPathEditor.SetExpectedControlLayout(inputAction?.expectedControlType ?? "");

var controlPathContainer = new IMGUIContainer(controlPathEditor.OnGUI);
m_Root.Add(controlPathContainer);
rootElement.Add(controlPathContainer);

DrawControlSchemeToggles(viewState, binding.Value);
}
Expand All @@ -81,15 +79,15 @@ private void DrawControlSchemeToggles(ViewState viewState, SerializedInputBindin
if (!viewState.controlSchemes.Any()) return;

var useInControlSchemeLabel = new Label("Use in control scheme");
m_Root.Add(useInControlSchemeLabel);
rootElement.Add(useInControlSchemeLabel);

foreach (var controlScheme in viewState.controlSchemes)
{
var checkbox = new Toggle(controlScheme.name)
{
value = binding.controlSchemes.Any(scheme => controlScheme.name == scheme)
};
m_Root.Add(checkbox);
rootElement.Add(checkbox);
checkbox.RegisterValueChangedCallback(changeEvent =>
{
Dispatch(ControlSchemeCommands.ChangeSelectedBindingsControlSchemes(controlScheme.name, changeEvent.newValue));
Expand Down
8 changes: 3 additions & 5 deletions ...ty.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/CompositeBindingPropertiesView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ namespace UnityEngine.InputSystem.Editor
{
internal class CompositeBindingPropertiesView : ViewBase<CompositeBindingPropertiesView.ViewState>
{
private readonly VisualElement m_Root;
private readonly DropdownField m_CompositeTypeField;
private EventCallback<ChangeEvent<string>> m_CompositeTypeFieldChangedHandler;

Expand All @@ -20,12 +19,11 @@ internal class CompositeBindingPropertiesView : ViewBase<CompositeBindingPropert
InputActionsEditorConstants.CompositeBindingPropertiesViewUxml;

public CompositeBindingPropertiesView(VisualElement root, StateContainer stateContainer)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;
var visualTreeAsset = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(UxmlName);
var container = visualTreeAsset.CloneTree();
m_Root.Add(container);
rootElement.Add(container);

m_CompositeTypeField = container.Q<DropdownField>("composite-type-dropdown");

Expand All @@ -46,7 +44,7 @@ public override void RedrawUI(ViewState viewState)
{
Dispatch(Commands.UpdatePathNameAndValues(viewState.parameterListView.GetParameters(), viewState.selectedBindingPath));
};
viewState.parameterListView.OnDrawVisualElements(m_Root);
viewState.parameterListView.OnDrawVisualElements(rootElement);
}

public override void DestroyView()
Expand Down
6 changes: 2 additions & 4 deletions ...nputsystem/InputSystem/Editor/UITKAssetEditor/Views/CompositePartBindingPropertiesView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ namespace UnityEngine.InputSystem.Editor
{
internal class CompositePartBindingPropertiesView : ViewBase<CompositePartBindingPropertiesView.ViewState>
{
private readonly VisualElement m_Root;
private readonly DropdownField m_CompositePartField;
private readonly IMGUIContainer m_PathEditorContainer;

Expand All @@ -18,12 +17,11 @@ internal class CompositePartBindingPropertiesView : ViewBase<CompositePartBindin
InputActionsEditorConstants.CompositePartBindingPropertiesViewUxml;

public CompositePartBindingPropertiesView(VisualElement root, StateContainer stateContainer)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;
var visualTreeAsset = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(UxmlName);
var container = visualTreeAsset.CloneTree();
m_Root.Add(container);
rootElement.Add(container);

m_PathEditorContainer = container.Q<IMGUIContainer>("path-editor-container");
m_CompositePartField = container.Q<DropdownField>("composite-part-dropdown");
Expand Down
8 changes: 3 additions & 5 deletions ...ages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ControlSchemesView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ internal class ControlSchemesView : ViewBase<InputControlScheme>
public event Action<ViewBase<InputControlScheme>> OnClosing;

public ControlSchemesView(VisualElement root, StateContainer stateContainer, bool updateExisting = false)
: base(stateContainer)
: base(root, stateContainer)
{
m_Root = root;
m_UpdateExisting = updateExisting;

var controlSchemeEditor = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(
Expand Down Expand Up @@ -49,7 +48,7 @@ public ControlSchemesView(VisualElement root, StateContainer stateContainer, boo
};
popupWindow.contentContainer.Add(controlSchemeVisualElement);
m_ModalWindow.Add(popupWindow);
m_Root.Add(m_ModalWindow);
root.Add(m_ModalWindow);
m_ModalWindow.StretchToParentSize();
m_ModalWindow.RegisterCallback<ClickEvent>(evt => Close());
popupWindow.RegisterCallback<ClickEvent>(evt => evt.StopPropagation());
Expand Down Expand Up @@ -96,7 +95,7 @@ private void RemoveDeviceRequirement()

public override void RedrawUI(InputControlScheme viewState)
{
m_Root.Q<TextField>(kControlSchemeNameTextField).value = viewState.name;
rootElement.Q<TextField>(kControlSchemeNameTextField).value = viewState.name;

m_ListView.itemsSource?.Clear();
m_ListView.itemsSource = viewState.deviceRequirements.Count > 0 ?
Expand Down Expand Up @@ -161,7 +160,6 @@ private void BindDeviceTypeCell(VisualElement visualElement, int rowIndex)
((Label)visualElement).text = (((string, bool))m_ListView.itemsSource[rowIndex]).Item1;
}

private readonly VisualElement m_Root;
private readonly bool m_UpdateExisting;
private MultiColumnListView m_ListView;
private VisualElement m_ModalWindow;
Expand Down
Loading