Add a new TypeConverter for ToolStripMenuItem.ShortcutKeys that does not provide standard items
#13039
+78
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
PR Overview
This PR introduces a new ShortcutKeysConverter that disables the provision of standard shortcut key values for ToolStripMenuItem.ShortcutKeys, addressing an issue where standard values were erroneously reported. The changes include:
- Adding a new ShortcutKeysConverter class and corresponding tests.
- Updating ToolStripMenuItem to use the new converter and improving layout transaction calls.
- Adjusting test naming and minor cleanups in supporting files.
Reviewed Changes
| File | Description |
|---|---|
| tests/UnitTests/System/Windows/Forms/ShortcutKeysConverterTests.cs | Adds tests for the new converter ensuring no standard values are provided. |
| src/System/Windows/Forms/Controls/ToolStrips/ShortcutKeysConverter.cs | Introduces the new converter with empty standard values and GetStandardValuesSupported returning false. |
| tests/UnitTests/System/Windows/Forms/PropertyGridTests.cs | Adds a regression test to validate mouse wheel behavior on shortcut keys in the PropertyGrid. |
| src/System/Windows/Forms/Controls/ToolStrips/ToolStripMenuItem.cs | Applies the new converter attribute and improves tooltip layout calls using nameof(). |
| src/System.Private.Windows.Core/src/System/SpanHelpers.cs | Removes obsolete pragma warning directives around string termination. |
| tests/UnitTests/System/Windows/Forms/PropertyGridInternal/PropertyGridView.DropDownHolderTests.cs | Renames a test method for clarity. |
| src/System/Windows/Forms/Controls/PropertyGrid/PropertyGridInternal/PropertyGridView.cs | Reformats a conditional check for improved readability. |
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/System.Private.Windows.Core/src/System/SpanHelpers.cs:24
- [nitpick] The removal of #pragma warning disable/restore suggests that the assignment's potential warning is now expected; please add a comment clarifying that this assignment is intentional to prevent new IDE warnings.
destination[source.Length] = '\0';
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/PropertyGridTests.cs
Outdated
Show resolved
Hide resolved
Tanya-Solyanik
force-pushed
the
shortcut-converter
branch
from
662175b to
efa2102
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13039 +/- ##
===================================================
- Coverage 76.14135% 76.11217% -0.02918%
===================================================
Files 3275 3277 +2
Lines 643775 643809 +34
Branches 47441 47443 +2
===================================================
- Hits 490179 490017 -162
- Misses 150036 150237 +201
+ Partials 3560 3555 -5
Flags with carried forward coverage won't be shown. Click here to find out more. |
Tanya-Solyanik
force-pushed
the
shortcut-converter
branch
from
efa2102 to
6d6eba1
Compare
JeremyKuhne
reviewed
Feb 27, 2025
src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ShortcutKeysConverter.cs
Outdated
Show resolved
Hide resolved
…er does not provide standard items
Tanya-Solyanik
force-pushed
the
shortcut-converter
branch
from
6d6eba1 to
599e6d7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #12982
Proposed changes
ToolStripMenuItem.ShortcutKeysproperty type (Keys enum) has a TypeConverter that reports that it has a set of standard values. This is not entirely accurate.Keystype has a set of standard values, but theShortcutKeysproperty does not, even the ShortcutKeys type contains different key combinations from what can be generated by the shortcut key editor. Shortcuts are combinations of keys and modifiers, it's not practical to present all combinations for user to step through with up/down keys or with mouse wheel. To disable this behavior, we need to add a new TypeConverter that does not provide standard values.I would like to add a new TypeConverter to this property.
KeysConverteris defined on the Keys enum, the property does not have one right now. TypeDescriptor prioretizes attributese defined on properties over those defined on property types. This change is "borderline" allowed depending on whether this is considered a changed attribute or a new one https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md#attributes.We don't have any other public instance properties of type Keys.
This behavior regressed when we added Keys.None value to the list of standard values - #8401. This was done to localize "None" (as "(none)") in the property grid. When the current value of the shortcut is not one of the standard values, property grid entry does not enter the "scroll through the standard items" functionality. This is a beneficial change, I'd prefer to keep it.
Video.Project.3.mp4
Microsoft Reviewers: Open in CodeFlow