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.

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

Jump to bottom

FIX: Scheme Name in Control Scheme editor menu that gets reset when editing devices (ISXB-763) #1941

Merged
merged 4 commits into from
May 28, 2024
Merged

FIX: Scheme Name in Control Scheme editor menu that gets reset when editing devices (ISXB-763) #1941

merged 4 commits into from
May 28, 2024

Conversation

bmalrat
Copy link
Collaborator

@bmalrat bmalrat commented May 27, 2024

Description

Fixed Scheme Name in Control Scheme editor menu that gets reset when editing devices

Changes made

In ControlSchemesView

  • Fixed RedrawUI to use the control scheme name or new/renamed one currently in edition
  • Fixed control scheme name text box change callback to now support :
  1. Trim the name to prevent having space at the begin/end of the name
  2. Prevent renaming the scheme if you change the name and put back the original name in the same edition without saving
  3. Write back in the UI if a unique name has been generated

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.

@bmalrat bmalrat changed the title Fixed Scheme Name in Control Scheme editor menu that gets reset when editing devices (ISXB-763) FIX: Scheme Name in Control Scheme editor menu that gets reset when editing devices (ISXB-763) May 27, 2024
@bmalrat bmalrat requested a review from Pauliusd01 May 27, 2024 20:52
ekcoh
ekcoh approved these changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

Basically this fix looks good to me, good work, but I had some minor tweaks I think could be adressed. Minor though so approving already now but let you consider if it makes sense to update based on comments.

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ControlSchemesView.cs Outdated Show resolved Hide resolved
Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/ControlSchemesView.cs
((TextField)evt.currentTarget).value);
// If the name is the same as the current name, don't change it
var newName = ((TextField)evt.currentTarget).value.Trim();
if (string.IsNullOrEmpty(newName) || String.Compare(newName, state.selectedControlScheme.name) == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: I think you can remove the string.IsNullOrEmpty since it cannot be null from Trim() right?
In addition String.Compare will be 0 if newName is empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it can't be null but IsEmpty doesn't exist.
String.Compare() will not be 0 if the string is empty, except if both are empty

Pauliusd01
Pauliusd01 approved these changes May 28, 2024
View reviewed changes
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.

LGTM, checked changing scheme names when making a new one/editing and adding new devices at various times.

bmalrat added 2 commits May 28, 2024 08:42
@bmalrat
Merge remote-tracking branch 'origin/develop' into isxb-763-fix-for-s…
215dc58 
…cheme-name-gets-reset-when-editing

# Conflicts:
#	Packages/com.unity.inputsystem/CHANGELOG.md
@bmalrat
replace "" by string.empty
aa114fa
@bmalrat bmalrat merged commit 2cca081 into develop May 28, 2024
77 of 79 checks passed
@bmalrat bmalrat deleted the isxb-763-fix-for-scheme-name-gets-reset-when-editing branch May 28, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pauliusd01 Pauliusd01 Pauliusd01 approved these changes

@ekcoh ekcoh ekcoh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bmalrat @ekcoh @Pauliusd01