Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the performance of the sidebar and list controls in the Context Menu Manager application by implementing asynchronous loading for shell items and refactoring the sidebar rendering approach.
Changes:
- Implemented asynchronous loading for ShellList items using Task.Run with cancellation support
- Added a new ShellItem constructor that accepts pre-computed data to avoid redundant registry reads
- Refactored MySideBar rendering from panel-based to direct painting approach
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 40 comments.
| File | Description |
|---|---|
| ContextMenuManager/MainForm.cs | Added ItemsLoaded event handler to coordinate search filtering with async list loading |
| ContextMenuManager/Controls/ShellList.cs | Implemented async loading with CancellationTokenSource, added helper methods to pre-compute item data, and introduced ItemsLoaded event |
| ContextMenuManager/Controls/ShellItem.cs | Added optimized constructor accepting pre-computed text, image, and multi-item flag to bypass expensive registry operations |
| ContextMenuManager/BluePointLilac.Controls/MySideBar.cs | Refactored from panel-based rendering to OnPaint-based approach, consolidated code, and abbreviated variable names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private bool isMultiItem(string regPath) // helper | ||
| { | ||
| return GetIsMultiItem(regPath); | ||
| } | ||
|
|
There was a problem hiding this comment.
The helper method isMultiItem is redundant and adds no value - it simply calls GetIsMultiItem(regPath). This wrapper method should be removed and callers should directly use GetIsMultiItem instead. This eliminates unnecessary indirection and reduces maintainability burden.
| private bool isMultiItem(string regPath) // helper | |
| { | |
| return GetIsMultiItem(regPath); | |
| } |
| private readonly Timer animTimer = new() { Interval = 16 }; | ||
| private string[] itemNames; | ||
| private int itemHeight = 36; | ||
| private int selectIndex = -1; | ||
| private int hoverIndex = -1; | ||
|
|
||
| private int animationTargetIndex = -1; | ||
| private int animationCurrentIndex = -1; | ||
| private float animationProgress = 0f; | ||
| private const float ANIMATION_SPEED = 0.25f; | ||
| private int itemHeight = 36, selectIndex = -1, hoverIndex = -1; | ||
| private int animTarget = -1, animCurrent = -1; | ||
| private float animProgress = 0f, curSelTop = -1; |
There was a problem hiding this comment.
Poor variable naming: Variable names have been excessively abbreviated (e.g., animTimer instead of animationTimer, animTarget/animCurrent instead of animationTargetIndex/animationCurrentIndex, curSelTop instead of currentSelectionTop). While shorter names can sometimes improve readability, these abbreviations make the code harder to understand, especially for developers unfamiliar with the codebase. The original names were clearer and self-documenting. Consider reverting to more descriptive names or at least using less cryptic abbreviations.
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -15); | ||
| } | ||
| else if (hasLUAShield) | ||
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -78); | ||
| else | ||
| icon = ResourceIcon.GetIcon(iconPath = itemFilePath, iconIndex = 0); | ||
|
|
||
| if (icon == null) | ||
| icon = ResourceIcon.GetExtensionIcon(iconPath = itemFilePath) ?? ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -2); |
There was a problem hiding this comment.
This assignment to iconPath is useless, since its value is never read.
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -15); | |
| } | |
| else if (hasLUAShield) | |
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -78); | |
| else | |
| icon = ResourceIcon.GetIcon(iconPath = itemFilePath, iconIndex = 0); | |
| if (icon == null) | |
| icon = ResourceIcon.GetExtensionIcon(iconPath = itemFilePath) ?? ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -2); | |
| icon = ResourceIcon.GetIcon("imageres.dll", iconIndex: -15); | |
| } | |
| else if (hasLUAShield) | |
| icon = ResourceIcon.GetIcon("imageres.dll", iconIndex: -78); | |
| else | |
| icon = ResourceIcon.GetIcon(itemFilePath, iconIndex: 0); | |
| if (icon == null) | |
| icon = ResourceIcon.GetExtensionIcon(itemFilePath) ?? ResourceIcon.GetIcon("imageres.dll", iconIndex: -2); |
| else if (hasLUAShield) | ||
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -78); | ||
| else | ||
| icon = ResourceIcon.GetIcon(iconPath = itemFilePath, iconIndex = 0); |
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -15); | ||
| } | ||
| else if (hasLUAShield) | ||
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -78); | ||
| else | ||
| icon = ResourceIcon.GetIcon(iconPath = itemFilePath, iconIndex = 0); | ||
|
|
||
| if (icon == null) | ||
| icon = ResourceIcon.GetExtensionIcon(iconPath = itemFilePath) ?? ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -2); |
There was a problem hiding this comment.
This assignment to iconPath is useless, since its value is never read.
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -15); | |
| } | |
| else if (hasLUAShield) | |
| icon = ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -78); | |
| else | |
| icon = ResourceIcon.GetIcon(iconPath = itemFilePath, iconIndex = 0); | |
| if (icon == null) | |
| icon = ResourceIcon.GetExtensionIcon(iconPath = itemFilePath) ?? ResourceIcon.GetIcon(iconPath = "imageres.dll", iconIndex = -2); | |
| icon = ResourceIcon.GetIcon("imageres.dll", iconIndex: -15); | |
| } | |
| else if (hasLUAShield) | |
| icon = ResourceIcon.GetIcon("imageres.dll", iconIndex: -78); | |
| else | |
| icon = ResourceIcon.GetIcon(itemFilePath, iconIndex: 0); | |
| if (icon == null) | |
| icon = ResourceIcon.GetExtensionIcon(itemFilePath) ?? ResourceIcon.GetIcon("imageres.dll", iconIndex: -2); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
No description provided.