FIX: Use the invariant culture for capitalization when generating code #2156

K-Tone · 2025-03-27T09:39:52Z

Description

A customer with the Turkish locale set, noticed that when we're generating function names for Input Actions, we're using the wrong function. This way, the "info" input action results in Onİnfo method name (notice the dot above the I, it's apparently how the capital for i looks like in Turkish).

Testing status & QA

Pending a QA pass, local testing by dev, a new regression test added

Overall Product Risks

Complexity: Medium
Halo Effect: Medium

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.
- JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
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.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

ekcoh · 2025-04-02T06:50:04Z

Glanced at the draft PR and I would just like to suggest to (if possible) get some unit test in that could catch any problems with culture info.

K-Tone · 2025-04-02T09:46:50Z

Also I see that some tests are failing due to compilation issues apparently, fixing that in a moment.

ritamerkl

The changes look good to me. Many thanks for going through all the cases!

The PR is still a draft, so this is probably still in work, but just a changelog and a link to a Jira ticket is missing in my POV.

K-Tone · 2025-04-08T12:18:11Z

Moving this out of the draft stage:

Added a new editor regression test precisely for the user issue (Hope Håkan is ok with it)
Looked through the current coverage on this branch and made sure that most of the changed lines in this PR are covered by existing tests

ekcoh

Great to get this fix in, I am approving but had some minor comments I let you decide whether they needs to be addressed or not.

ekcoh · 2025-04-09T06:41:42Z

Assets/Editor/AddScenesToBuild.cs

        foreach (string folder in excludedFolders)
        {
-            if (path.ToLower().Contains(folder.ToLower()))
+            if (path.Contains(folder, StringComparison.InvariantCultureIgnoreCase))


Great to fix case issues here, but I wonder if this wasn't incorrect to begin with. Shouldn't this be using https://learn.microsoft.com/en-us/dotnet/api/system.io.path?view=net-9.0 which should take platform-specific behavior of underlying file systems into account instead? Its probably fine, but it is a bit of a warning flag in my eyes when code tries to "outsmart" the filesystem implementation. Its also interesting previous code was doing ToLower() on a constant that is already in lowercase (its not an arbitrary input). The point I am after is that on some OS you may have both a XR, Xr, xR and xr directory and they are all different. (e.g. linux)

I guess for paths, we should probably use https://github.com/lucasmeijer/NiceIO because it makes pathing so easily and cool. For this instance, I would agree it might have been somewhat incorrect on case-sensitive file systems - which are normally Linux and the default Mac. Won't target changing behaviour here though.

Assets/Tests/InputSystem.Editor/InputActionAssetManagerTests.cs

ekcoh · 2025-04-09T06:47:43Z

Packages/com.unity.inputsystem/InputSystem/Editor/InputSystemPluginControl.cs

        {
            var registeredPackages = UnityEditor.PackageManager.PackageInfo.GetAllRegisteredPackages();
-            var plugInName = PlugInName + EditorUserBuildSettings.activeBuildTarget.ToString().ToLower();
+            var plugInName = PlugInName + EditorUserBuildSettings.activeBuildTarget.ToString();


It looks like you fixed a separate issue here right? Or are active build targets package names ToUpper? I am confused around how this previously was supposed to work. The new code looks solid though, effectively ignoring case.

ekcoh · 2025-04-09T06:56:16Z

Packages/com.unity.inputsystem/InputSystem/Utilities/InternedString.cs

        {
-            return string.Compare(a.ToLower(CultureInfo.InvariantCulture), b.m_StringLowerCase,
-                StringComparison.InvariantCultureIgnoreCase) == 0;
+            return string.Compare(a, b.m_StringLowerCase, StringComparison.InvariantCultureIgnoreCase) == 0;


Minor: One could call the other operator, e.g. return (b == a) to avoid the redundant code

ekcoh · 2025-04-09T06:56:53Z

Packages/com.unity.inputsystem/InputSystem/Utilities/InternedString.cs

        {
-            return string.Compare(a.ToLower(CultureInfo.InvariantCulture), b.m_StringLowerCase,
-                StringComparison.InvariantCultureIgnoreCase) != 0;
+            return string.Compare(a, b.m_StringLowerCase, StringComparison.InvariantCultureIgnoreCase) != 0;


Minor: One could call the other operator, e.g. return (b != a)

…ration

… the current culture

Pauliusd01 · 2025-04-10T11:12:37Z

Did not forget about this one, will check today/tomorrow

Pauliusd01

LGTM, I checked generating and using input actions with non latin characters (Chinese, Russian, Japanese, Arabic, Hebrew, Korean, Thai, Vietnamese, Turkish) on Windows and Mac with Chinese, English and Turkish locales.

But I have to note that I wasn't able to repro the original turkish i character issue on my end. The latin letters i and I stayed the same after generating them on a different locale

ritamerkl approved these changes Apr 2, 2025

View reviewed changes

K-Tone force-pushed the anthony/ixsb-1406-fix-actions-capitalization branch from 377cefa to 96e30ba Compare April 3, 2025 14:31

K-Tone marked this pull request as ready for review April 8, 2025 12:16

K-Tone requested a review from Pauliusd01 April 8, 2025 13:41

K-Tone force-pushed the anthony/ixsb-1406-fix-actions-capitalization branch from 58720fc to e1de946 Compare April 9, 2025 06:01

ekcoh approved these changes Apr 9, 2025

View reviewed changes

K-Tone added 9 commits April 10, 2025 13:28

use invariant culture when capitalizing for the purposes of code gene…

3d05994

…ration

use invariant string operations where possible

12d1278

some more lowercase fun

72ee68e

fix up a compilation issue

052c5dd

format code

09987dc

update changelog

5d88031

Added a regression test for type names that priorly were dependant on…

e9f8a63

… the current culture

fix formatting

9f8740c

some final tweaks after code review

ee83350

K-Tone force-pushed the anthony/ixsb-1406-fix-actions-capitalization branch from 21c0680 to ee83350 Compare April 10, 2025 10:28

Pauliusd01 approved these changes Apr 11, 2025

View reviewed changes

K-Tone merged commit bb6a72d into develop Apr 11, 2025
110 checks passed

K-Tone deleted the anthony/ixsb-1406-fix-actions-capitalization branch April 11, 2025 09:27

FIX: Use the invariant culture for capitalization when generating code #2156

FIX: Use the invariant culture for capitalization when generating code #2156

Uh oh!

Conversation

K-Tone commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing status & QA

Overall Product Risks

Checklist

Uh oh!

ekcoh commented Apr 2, 2025

Uh oh!

K-Tone commented Apr 2, 2025

Uh oh!

ritamerkl left a comment

Choose a reason for hiding this comment

Uh oh!

K-Tone commented Apr 8, 2025 • 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!

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

K-Tone Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Pauliusd01 commented Apr 10, 2025

Uh oh!

Pauliusd01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

K-Tone commented Mar 27, 2025 •

edited

Loading

K-Tone commented Apr 8, 2025 •

edited

Loading