Skip to content

ISX-1851 Remove title from editor window #1822

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

Closed
wants to merge 10 commits into from

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Jan 29, 2024

Description

Removes the header title from UITK editor window when used outside Project Settings.
Improves styling of header title in Project Settings.

Changes made

Removes the header title from editor window as called out in user feedback here: https://forum.unity.com/threads/project-wide-actions-1-8-0-pre-2-release-feedback-wanted-new-update-released.1488843/ by Walter_hulsebos.

Modifies header title styling to look more similar to other Settings. Not perfect but closer. Also alignes menu button with container window menu button.

Screenshot 2024-01-29 at 12 29 47

Screenshot 2024-01-29 at 12 16 04

Notes

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.

Copy link
Collaborator

@ritamerkl ritamerkl left a comment

Choose a reason for hiding this comment

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

LGTM

@ekcoh
Copy link
Collaborator Author

ekcoh commented Jan 29, 2024

Reverted the change to title I did while checking it, also replaced incorrect screenshot

Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Just needs formatting running on the commit, but looks good

@ekcoh
Copy link
Collaborator Author

ekcoh commented Jan 29, 2024

Just needs formatting running on the commit, but looks good

Should hopefully be fixed now.

@Pauliusd01
Copy link
Collaborator

Getting 2 different errors that don't repro on develop when adding new maps to fresh assets. Steps to repro: Create a new input asset -> make sure it is in auto save and add a new map -> get UITK error from Editor side -> delete that map and add one again -> get nullref from Input System side

Unity_2024-01-29_15-20-17.mp4

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

updating status

@ekcoh
Copy link
Collaborator Author

ekcoh commented Jan 30, 2024

updating status

@Pauliusd01 - Checked the following

  1. I merged latest changes from develop into this branch just to be sure its up-to-date.
  2. I tried the steps described on 2022.3.18f1 and I cannot replicate doing exactly the same as in the video.
  3. I tried the steps described on 2023.2.b04 and I cannot replicate doing exactly the same as in the video.

I noticed the exception happens in Unity native UI framework code. Do you happen to have a full stack trace. A video is great for reproduction but not very helpful to find the problem when I cannot replicate. Are you saying this exception only happens on this branch and not on develop using the same Unity version?

@ekcoh
Copy link
Collaborator Author

ekcoh commented Jan 31, 2024

For clarity, as discussed outside this PR, I failed to reproduce on macOS while @Pauliusd01 got the errors on Windows 10 so there is likely a platform-dependent issue, likely in UITK implementation and indirectly triggered by this PR.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 1, 2024

@Pauliusd01 I pushed a potential fix for the renaming problem that attempted to enter rename mode without a selection as I was able to reproduce the reported issue on Win10 but not on macOS so something regarding renaming is platform dependent. Feel free to retest.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 2, 2024

I dinged further into this yesterday and the problematic behavior is caused by renaming logic where there isn't a clean adoption of the model-view design in the view components. Hence there is work needed on renaming and selection logic (unrelated to changes on this branch).

@lyndon-unity
Copy link
Collaborator

I think the code in this PR supersedes this
#1834

Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

I've moved all key changes over into the other PR so this one shouldn't land.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 12, 2024

I've moved all key changes over into the other PR so this one shouldn't land.

@lyndon-unity What exactly was moved then? Does this imply the regression introduced by this branch and attempted to be solved in sub-branch #1833 was also ported into #1834? I believe it would be good to avoid porting that regression into 1834.

@lyndon-unity
Copy link
Collaborator

I've moved all key changes over into the other PR so this one shouldn't land.

@lyndon-unity What exactly was moved then? Does this imply the regression introduced by this branch and attempted to be solved in sub-branch #1833 was also ported into #1834? I believe it would be good to avoid porting that regression into 1834.

I haven't looked at the sub branch and the 'regression'.
The header changes in this PR are applied.
I did apply RenameNewActionMaps() too so if thats the regression area then the sub branch parts will need applying.
(https://github.com/Unity-Technologies/InputSystem/pull/1834/files#diff-2627872bea2748f8f1725742226018473e44f270300bd2cca84b782223b0057c )

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 13, 2024

@lyndon-unity the regression seems completely unrelated to the changes on this PR but for some reason these changes triggers a problem that seems to only be visible on Windows and not macOS. Likely related to focus/selection, hence the changes on the other branch. The RenameNewActionMaps changes where added as a way to mitigate the problem. The problem surfaced just by changing the UXML which was the odd part.

@ekcoh ekcoh added the work in progress Indicates that the PR is work in progress and any review efforts can be post-poned. label Feb 21, 2024
@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 29, 2024

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 ekcoh closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress Indicates that the PR is work in progress and any review efforts can be post-poned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants