-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix incorrect localization causing crashes in RegisterCommandHandler and KeyGestures #919
Conversation
Removing the "LOC CHECKIN" prefix from the title. This is not a conventional loc-checkin. |
Can you summarize (in a table, perhaps?) what strings are being removed from resx files? |
It was hard to keep it all in my head to determine, so I'll ask, are there any duplicate strings being defined in multiple files that could be hoisted up into a common file? If so, please do so. |
...rosoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/FileClassifier.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Resources/Strings.resx
Show resolved
Hide resolved
...Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/FlowDocumentReader.cs
Outdated
Show resolved
Hide resolved
Removed string resources could have been used by others in the past. Are we worried about those no longer being accessible? |
What's a concrete scenario that might break? |
Someone could be looking directly at the string resources and either displaying them or parsing them out for their own needs. I don't have a specific example for sure, but resources are publicly available at runtime and now these are not a part of that set. |
Retaining the old strings seems like a costly (i.e, confusion and bug-prone) effort on our side. We'd end up with two sets of strings (resources and const strings) - which would require careful documentation to ensure that we only use the const-strings in code and never use the resource strings. Localization will continue happening (and we'd continue having to ensure that these strings remain locked, despite the fact that we don't them, nor are we sure anyone else uses them). On balance, I think we should just take this change now as a clean-break. Anyone depending on these resources will just have to adapt a bit. |
That's fair enough, anyone accessing these should really be going through the various RoutedUICommands anyway. |
Resource IDs moved to const strings from strings.resx filesPresentationBuildTasks
PresentationCore
PresentationFramework
|
src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/UidManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/UidManager.cs
Outdated
Show resolved
Hide resolved
...rosoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/FileClassifier.cs
Outdated
Show resolved
Hide resolved
In your tables, I can't tell whether the list consists of strings, or resource names. It would be best if the table were structured as a {resource name, string value} pair. |
Removing a string from .resx removes that string's corresponding generated SRID. The table contains resource IDs, which were used as const string names where possible. |
...rosoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/FileClassifier.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on our conversation, you are going to change these to nameof(FileClassifier)
etc., right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the table as a doc markdown file under documentation\
. lgtm.
Reviewed key strings, updated strings.resx files, and updated xlf files.
Confirmed updates fix NuGet Package Explorer and loc test application provided by @Lakritzator. (Fixes #913.)