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

graham-huws · 2024-01-30T15:10:24Z

Description

It's possible for RedrawUI to be called when m_Root is empty, throwing errors in the console.
This can happen when the view is discarded immediately after being loaded, i.e. jumping to the input settings
page in Preferences when the last window shown there was the main view, which the editor (briefly) loads first. (ISX-1721)

Changes made

Compare against the number of m_ChildViews as a cheap way to know we should skip redrawing.

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.
- FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
- FogBugz is marked as "Resolved" with next release version correctly set.
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.

graham-huws · 2024-01-30T15:54:28Z

@ekcoh - I don't think this fix is "ideal", but it's a small change and it's a very specific set of circumstances required to make the errors occur, and this catches that. If you've got any thoughts on improving though, I'm interested!

If there's a way to check for children by name in m_Root that doesn't produce errors, that's probably better 🤔

ekcoh

Thanks for providing this fix. I think it can be improved by moving all of it into ViewBase#UpdateView method.

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionsTreeView.cs

...ges/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/InputActionsEditorView.cs

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ViewBase.cs

graham-huws · 2024-02-01T11:02:17Z

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionsTreeView.cs

        private readonly TreeView m_ActionsTreeView;
-        private Button addActionButton => m_Root?.Q<Button>("add-new-action-button");
+        private readonly Button m_AddActionButton;



@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.")

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.

Pauliusd01

LGTM, tried opening/editing different settings assets both active and inactive ones and wasn't able to repro the error anymore.

ekcoh · 2024-02-01T11:13:08Z

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionMapsView.cs

        }

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


Is there any reason for querying this lazily? If not I would advise initializing the reference as readonly in the constructor using the passed root argument instead of querying it on each call.

Right now its adressed differently in ActionMapsView vs ActionPropertiesView.

ekcoh · 2024-02-01T11:15:41Z

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionsTreeView.cs

        private readonly TreeView m_ActionsTreeView;
-        private Button addActionButton => m_Root?.Q<Button>("add-new-action-button");
+        private readonly Button m_AddActionButton;



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.

ekcoh · 2024-02-01T11:16:16Z

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ActionMapsView.cs

-            m_Root = root;
-
-            m_ListView = m_Root?.Q<ListView>("action-maps-list-view");
+            m_ListView = rootElement?.Q<ListView>("action-maps-list-view");


Why use rootElement when you already have root passed as arg?

This is a pattern in all the constructors. It probably doesn't matter at the moment but if accessing base within constructor (while constructing the object) can be avoided its usually good to avoid. (This exists in all affected classes but haven't commented on all of them).

This is a pattern in all the constructors. It probably doesn't matter at the moment but if accessing base within constructor (while constructing the object) can be avoided its usually good to avoid.

Interesting, is that a style thing? I assumed performance-wise it would be the same; my thinking was that it's better to have everything accessing rootElement since then it's consistent across the files (eg. it's more clear that the root in the constructor and in RedrawUI is the same). Happy to change, either way!

Its probably just in the category nit-picking and could be ignored, but since there is already a local (potentially) in a register, why call into a getter that needs to fetch the reference from base?

…ther than accessed on demand.

ekcoh

LGTM

Pauliusd01

Don't see the difference from when I last checked, lgtm

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

762b5aa

graham-huws force-pushed the ISX-1721-settings-error-in-console branch from 3d9af80 to 762b5aa Compare January 30, 2024 15:48

graham-huws requested a review from ekcoh January 30, 2024 15:50

graham-huws marked this pull request as ready for review January 30, 2024 15:50

ekcoh requested changes Jan 30, 2024

View reviewed changes

Address feedback: Consolidate root element for views and bugfix logic.

4d2c0c6

graham-huws commented Feb 1, 2024

View reviewed changes

Merge branch 'develop' into ISX-1721-settings-error-in-console

7c4a085

graham-huws requested a review from ekcoh February 1, 2024 11:04

Rename after merging develop branch.

ba14a8c

graham-huws requested a review from Pauliusd01 February 1, 2024 11:15

Fix scrollview access sometimes failing.

96fa3a2

Pauliusd01 approved these changes Feb 1, 2024

View reviewed changes

ekcoh requested changes Feb 1, 2024

View reviewed changes

Match existing styles, including having readonly element variables ra…

020f837

…ther than accessed on demand.

ekcoh approved these changes Feb 1, 2024

View reviewed changes

graham-huws requested a review from Pauliusd01 February 1, 2024 15:48

Pauliusd01 approved these changes Feb 2, 2024

View reviewed changes

ritamerkl approved these changes Feb 2, 2024

View reviewed changes

graham-huws merged commit e58f599 into develop Feb 2, 2024

graham-huws deleted the ISX-1721-settings-error-in-console branch February 2, 2024 09:45

Pauliusd01 restored the ISX-1721-settings-error-in-console branch February 2, 2024 14:31

Pauliusd01 deleted the ISX-1721-settings-error-in-console branch February 2, 2024 14:36

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

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

Uh oh!

Conversation

graham-huws commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes made

Checklist

Uh oh!

graham-huws commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Pauliusd01 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Pauliusd01 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

graham-huws commented Jan 30, 2024 •

edited

Loading

graham-huws commented Jan 30, 2024 •

edited

Loading