InputSystem icon indicating copy to clipboard operation
InputSystem copied to clipboard

Isx 1851 remove title from editor window rename - extended

Open ekcoh opened this issue 1 year ago • 4 comments

Description

Note that this branch is a sub-branch of https://github.com/Unity-Technologies/InputSystem/pull/1822 and hence it contains all changes from that PR but also contains additional fixes called out under Changes made below.

This PR is a tentative fix to the regressive behaviour reported in https://github.com/Unity-Technologies/InputSystem/pull/1822 and hence if verified to eliminate the problems mentioned in that PR and fulfilling the functionality intended on https://github.com/Unity-Technologies/InputSystem/pull/1822 this PR might be merged instead and the previous PR closed. If not successful this either needs more work or needs to be discarded while the original PR remains intact with the UI changes.

Changes made

Everything mentioned in https://github.com/Unity-Technologies/InputSystem/pull/1822

But also:

  • Modified RenameNewActionMaps to no longer be triggered from RedrawUI. Instead Rename is triggered as a shortcut command or via AddActionMap (indirectly through state not in sync with view/state). However this now happens as a continuation of AddActionMap if successful. This is likely the fix corrected the regression.
  • Modified RenameNewActionMap to scroll to target.
  • Extended StateContainer Dispatch to allow taking an optional action to be executed on the UI scheduler queue directly after the command if and only if the command is successful. The intention is increased exception safety.
  • Extended ViewBase with a isRedrawInProgress that may be used to discard any View to Model changes that would not lead to any change early. This basically just eliminates need to unhook/modify/hook patterns in RedrawUI since RedrawUI should only reflect the model/state to the view.
  • Made some minor improvements to CollectionViewSelectionChangeFilter so it uses less memory when disabled. Also fixed a compiler error when disabled (dev mode only).
  • Made a bunch of methods protected that were public for no reason (only internal code)
  • Made a bunch of fields readonly when intention is to not allow modification of them.

Notes

Additional issues found during development. Previously Action Map panel never scrolled to newly added items when list is larger than panel. This has been corrected logically but for some reason the list is always one item off. Unclear why.

Found an issue where adding a number of Actions and directly after exiting renaming mode with enter the up key is pressed causes an exception in UITK code. Is this a new bug?

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.
    • 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.

ekcoh avatar Feb 06 '24 18:02 ekcoh

I am happy to see lots of good changes. I left some comments, while my major concern is that we might miss selections if we interrupt the Selection Callback when a redraw is in process. Maybe I am overseeing something.

Not sure I follow here @ritamerkl could you elaborate a bit since it sounds like you have spotted something about this PR I might have overlooked?

ekcoh avatar Feb 12 '24 19:02 ekcoh

I am still reproducing #1822 (comment) is that expected?

I hoped this would eliminate that which is the whole point of this "sub-branch". Seems like I need to revisit on Windows again then.

ekcoh avatar Feb 12 '24 21:02 ekcoh

Not sure I follow here @ritamerkl could you elaborate a bit since it sounds like you have spotted something about this PR I might have overlooked?

I tried to bring a bit more clarity in my comments at the according threads

ritamerkl avatar Feb 13 '24 11:02 ritamerkl

I am still reproducing #1822 (comment) is that expected?

I hoped this would eliminate that which is the whole point of this "sub-branch". Seems like I need to revisit on Windows again then.

I was not able to reproduce this error in 2022.3.19 and the same editor version shown in the video. The first error seems to start the exception row, it would be interesting to see the whole log of this one. If you can reproduce this bug again @Pauliusd01, could you look if you can get the whole stack trace?

ritamerkl avatar Feb 15 '24 14:02 ritamerkl

I believe this PR is obsolete now due to other changes. Will close this PR but leave branch existing for a while longer if there is need to pick any cherries from it.

ekcoh avatar Feb 29 '24 21:02 ekcoh