FileStatusList: Reuse context menu separators #11916
Conversation
| @@ -31,6 +33,8 @@ public sealed partial class FileStatusList : GitModuleControl | |||
| private readonly ToolStripItem _openInVisualStudioSeparator = new ToolStripSeparator(); | |||
| private readonly ToolStripItem _NO_TRANSLATE_openInVisualStudioMenuItem; | |||
| private readonly CancellationTokenSequence _reloadSequence = new(); | |||
| private readonly ToolStripItem _showAllDifferencesSeparator = new ToolStripSeparator() { Name = _showAllDifferencesItemName + "Separator" }; | |||
There was a problem hiding this comment.
| private readonly ToolStripItem _showAllDifferencesSeparator = new ToolStripSeparator() { Name = _showAllDifferencesItemName + "Separator" }; | |
| private readonly ToolStripItem _showAllDifferencesSeparator = new ToolStripSeparator() { Name = $"{_showAllDifferencesItemName}Separator" }; |
There was a problem hiding this comment.
Why?
It is compile-time constant.
And I would expect that a single concatenation of two (constant) strings is cheaper than string formatting.
There was a problem hiding this comment.
This seems like a premature micro optimisation. I trust the compiler to do the best here.
Interpolated strings are preferred to string concatenations.
There was a problem hiding this comment.
This seems like a premature micro optimisation. I trust the compiler to do the best here.
Interpolated strings are preferred to string concatenations.
- I have just moved that code.
- I was anticipating your performance argument with which you use to reason the switch to interpolated strings.
Only in case of a simple concatenation, the+is slightly clearer to me than interpolation.
Though in order to have a simple coding rule, I have changed it as requested.
There was a problem hiding this comment.
Interpolation with constants does not have runtime overhead.
| @@ -22,6 +22,8 @@ namespace GitUI | |||
| { | |||
| public sealed partial class FileStatusList : GitModuleControl | |||
| { | |||
| private const string _showAllDifferencesItemName = "ShowDiffForAllParentsText"; | |||
There was a problem hiding this comment.
| private const string _showAllDifferencesItemName = "ShowDiffForAllParentsText"; | |
| private const string ShowAllDifferencesItemName = nameof(ShowDiffForAllParentsText); |
There was a problem hiding this comment.
I have followed the naming conventions for private constants:
There is no such identifier ShowDiffForAllParentsText except in TranslatedStrings.
I have aligned the moved / new identifiers as well.
Fixes #11909
Proposed changes
There are used different ContextMenuStrips for files and for submodules.
The menu items are added on menu open - and automatically removed from the other menu.
When re-adding them, the separators must not be created anew (as it is done for
_openInVisualStudioSeparator).Set the visibility of
FormCommit.toolStripSeparatorScriptas in 844050a (done for all usages ofAddUserScripts).Screenshots
Before
see issue
After
no multiple separators
Test methodology
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.