Refactor: Improve architecture, performance, and class organization

[T5ive]

This commit addresses a broad set of issues aimed at improving your application's performance, reorganizing its architecture, and clarifying class responsibilities.

Key Architectural Changes:

• Eliminated the global PublicClass to reduce tight coupling and improve testability.
• Introduced SettingsService to manage loading and saving of application settings from/to JSON, centralizing settings management.
• Introduced ImageCacheService to handle loading and caching of all images and icons, significantly improving UI rendering performance for elements with icons.
• Refactored ItemsManager into an instance-based class, separating its data manipulation logic from UI concerns. It now uses events to notify the UI of changes.
• Created TreeViewHelper to encapsulate TreeView-specific UI logic, previously mixed in Utility or ItemsManager.

Performance Enhancements:

• Implemented image caching via ImageCacheService, reducing redundant file I/O and icon extraction operations.
• Applied SuspendLayout and ResumeLayout to FrmMain's context menu population and TreeViewHelper's tree loading, improving UI update performance and reducing flicker.

Code Organization:

• Restructured the project by creating Models and UI/Helpers folders for better class categorization.
• Moved data model classes (ListInfo, GroupInfo, etc.) to the Models namespace/folder.
• Moved TreeViewHelper to the UI.Helpers namespace/folder.
• Standardized namespaces for utility classes.

Note: Direct performance testing and full build verification were conducted based on code analysis due to limitations in the execution environment. The changes are expected to yield significant improvements in maintainability and UI responsiveness.

[gemini-code-assist]

Hello @T5ive, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Gemini here, providing a summary of this pull request authored by T5ive.

This PR focuses on a significant refactor aimed at improving the application's architecture, performance, and overall code organization. The core changes involve moving away from a global state (PublicClass) towards a more service-oriented and instance-based design. New services (SettingsService, ImageCacheService) are introduced to handle specific concerns like settings management and image loading/caching, while the ItemsManager is refactored into an instance class responsible solely for data manipulation.

UI logic previously mixed in data or utility classes has been extracted into a dedicated helper (TreeViewHelper). Performance enhancements include implementing image caching to reduce file I/O and using SuspendLayout/ResumeLayout for smoother UI updates. The project structure is also improved with new folders and standardized namespaces for better clarity and maintainability.

Highlights

• Architectural Refactor: Eliminated the global PublicClass dependency, promoting better encapsulation and testability.
• New Services Introduced: Added SettingsService for centralized JSON settings loading/saving and ImageCacheService for efficient image/icon handling with caching.
• ItemsManager Refactored: Converted ItemsManager from a static helper to an instance class, separating data logic from UI concerns and using events (ItemsChanged) to notify the UI.
• UI Logic Extraction: Created TreeViewHelper to consolidate TreeView-specific UI manipulation logic, removing it from Utility and ItemsManager.
• Performance Improvements: Implemented image caching via ImageCacheService and applied SuspendLayout/ResumeLayout to UI updates for better responsiveness.
• Code Organization: Restructured the project with Models and UI/Helpers folders and standardized namespaces.
• Dependency Injection: Updated form constructors (FrmMain, FrmSetting) to accept service dependencies.

Changelog

Click here to see the changelog

• Toolkit Launcher/Form/FrmMain.cs
  • Added using statements for Models and Services.
  • Added _settingsService and _imageCacheService readonly fields.
  • Modified constructor to accept SettingsService and ImageCacheService via DI (lines 13-18).
  • Removed old static initialization and loading methods (InitializeList, LoadFile, LoadJson) (lines 36-44).
  • Updated FrmMain_Load to remove calls to old initialization methods (lines 36-37).
  • Updated AddItems to use _settingsService.CurrentSettings and added SuspendLayout/ResumeLayout (lines 48-51).
  • Modified FrmSetting instantiation in AddSetting to pass service instances (line 65).
  • Updated AddAll methods to use _imageCacheService.GetImage for loading icons and added null/default image checks (lines 119-127, 173-179).
• Toolkit Launcher/Form/FrmSetting.cs
  • Added various using statements for new/moved namespaces (UI.Helpers, Models, Services, ItemManager, Utilities) (lines 1-11).
  • Replaced static _editor with instance field (line 17).
  • Added _settingsService, _imageCacheService, _itemsManager instance fields (lines 18-20).
  • Modified constructor to accept services via DI, initialize _itemsManager, and subscribe to ItemsChanged (lines 23-40).
  • Added ItemsManager_ItemsChanged event handler to reload the TreeView and restore selection (lines 42-50).
  • Updated FrmSetting_Load to use TreeViewHelper.LoadAll and _settingsService.TempSettings (lines 53-63).
  • Refactored SaveFile to use _settingsService.SaveSettings (lines 67-86).
  • Refactored Add Item methods (btnAddHeader_Click, etc.) to use a common HandleAddItem method which calls _itemsManager.AddGroup and TreeViewHelper.SelectNodeByPath (lines 90-120).
  • Updated treeOption_AfterSelect to use _itemsManager methods for getting type and detail (lines 123-134).
  • Updated EnableType logic based on selection type and enabled/disabled context menu items (lines 137-151).
  • Refactored GetDetail to use _itemsManager.GetGroupInfoByPath and handle null cases (lines 155-194).
  • Updated treeOption_MouseDown to handle right-click selection and call MenuController (lines 211-230).
  • Updated MenuController logic based on node selection and type (lines 232-260).
  • Updated treeOption_KeyDown to use _itemsManager.RemoveGroup (lines 264-281).
  • Updated treeOption_AfterLabelEdit to use _itemsManager.RenameGroup (lines 283-326).
  • Refactored context menu add clicks to ContextMenuAdd_Click (lines 352-364).
  • Updated detail panel button clicks (btnName_Click, btnPath_Click, btnIcon_Click) to use _itemsManager methods (lines 376-452).
  • Refactored detail panel text/checkbox changes to call SaveTempAdvanceInfo which uses _itemsManager.UpdateGroupAdvanceInfo (lines 469-491).
  • Updated btnCancel_Click to call _settingsService.ReloadTempSettings (lines 454-457).
  • Added FrmSetting_FormClosing to unsubscribe from the ItemsChanged event (lines 494-501).
• Toolkit Launcher/ItemManager/ItemsManager.cs
  • Changed namespace from Toolkit_Launcher.Helper to Toolkit_Launcher.ItemManager (line 2).
  • Changed from static class to instance class ItemsManager (line 12).
  • Added _listInfo instance field and constructor to initialize it (lines 14-23).
  • Added ItemsChanged event and OnItemsChanged method (lines 17-28).
  • Converted all methods from static to instance.
  • Removed TreeView parameters from methods that previously manipulated the UI directly.
  • Modified methods (AddGroup, RemoveGroup, MoveGroup, RenameGroup, UpdateGroupValuePaths, UpdateGroupAdvanceInfo) to modify the internal _listInfo and call OnItemsChanged.
  • Renamed and refactored methods for getting group info by path (GetParentGroupInfoByPath, GetGroupInfoByPath, GetGroupInfoByPathRecursive) (lines 114-148).
  • Renamed GetType to GetGroupType (lines 152-156).
  • Removed GetGroupDetail.
  • Added UpdateChildFullPaths helper for renaming logic (lines 271-289).
  • Updated GetNewIndex methods to work with lists of GroupInfo (lines 105-111).
• Toolkit Launcher/Models/ListInfo.cs
  • Added using Toolkit_Launcher; for MyEnums (line 3).
• Toolkit Launcher/Program.cs
  • Instantiated SettingsService and ImageCacheService (lines 10-11).
  • Modified FrmMain instantiation to pass service instances (line 13).
  • Note: The Application.Run line appears duplicated in the patch (lines 13-14).
• Toolkit Launcher/Services/ImageCacheService.cs
  • New file created in Toolkit_Launcher.Services namespace (lines 1-120).
  • Includes _imageList and _fileImageCache fields.
  • Provides GetImage overloads for MyEnums.GroupType and file paths.
  • Implements caching for file-based images.
  • Includes IconFromFilePath helper for extracting icons.
  • Provides GetSystemImageList for UI controls.
• Toolkit Launcher/Services/SettingsService.cs
  • New file created in Toolkit_Launcher.Services namespace (lines 1-62).
  • Manages loading (LoadSettingsFromFile) and saving (SaveSettings) of ListInfo from/to JSON.
  • Uses CurrentSettings and TempSettings properties.
  • Provides ReloadTempSettings method.
  • Uses Newtonsoft.Json.
• Toolkit Launcher/UI/Helpers/TreeViewHelper.cs
  • New file created in Toolkit_Launcher.UI.Helpers namespace (lines 1-145).
  • Contains static methods for TreeView manipulation (LoadAll, CreateTreeNode, GetImageIndex, SelectNodeByPath, FindNodeRecursive, AddTreeNode).
  • Methods accept ListInfo and ImageCacheService parameters.
  • Uses TreeNode.Tag to store the full path.
• Toolkit Launcher/Utilities/Utility.cs
  • Changed namespace from Toolkit_Launcher to Toolkit_Launcher.Utilities (line 7).
  • Removed TreeView-specific methods (AddGroup, AddSubGroup, LoadAll, SelectNode, SelectNodeChild) (lines 17-18).
  • Removed image loading methods (IconFromFilePath, GetImage extensions) (lines 52-53).
  • Added null/empty checks in GetMissingNumber, FindIndex, and GetTarget (lines 24, 37, 43, 54).
  • Added ToLowerInvariant in GetTarget switch (line 63).
  • Added basic validation in IsTarget (line 73).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces significant and positive architectural changes, moving towards a more decoupled, service-oriented design. The introduction of SettingsService, ImageCacheService, an instance-based ItemsManager, and TreeViewHelper are commendable. The elimination of global state via PublicClass is a key improvement. The changes should also lead to better performance due to image caching and UI update optimizations.

While the refactoring is extensive and well-executed in many areas, there are a few points that need attention, including one critical issue in Program.cs.

Summary of Findings

• Critical: Duplicate Application Run: In Program.cs, Application.Run() is called twice, which will lead to the main form running sequentially two times. This needs to be corrected.
• High: Item Selection After Rename: In FrmSetting.cs, after an item is renamed via inline TreeView edit, the selection logic in ItemsManager_ItemsChanged uses the old item path, potentially leading to lost or incorrect selection. This impacts user experience.
• Medium: Null-Forgiving Operator Usage: The null-forgiving operator (!) is used with _settingsService.CurrentSettings in FrmMain.cs. Given the initialization logic, CurrentSettings should not be null. Consider removing the operator or clarifying the nullability scenario.
• Medium: Initialization Logic in FrmSetting_Load: The check for _settingsService.TempSettings == null in FrmSetting_Load seems defensive, as TempSettings should be initialized by the SettingsService constructor. Clarification on this scenario is needed.
• Medium: Placeholder Default Images: ImageCacheService uses placeholder Bitmap(16,16) for default images. These should be replaced with actual icons.
• Medium: Service Error Handling: SettingsService uses Console.WriteLine for error handling. A more user-facing approach or robust logging is recommended for file I/O errors.
• Medium: Legacy ImageID Property: The ImageID property in ValueInfo (set by ItemsManager) might be a remnant from older logic. Its current necessity should be reviewed given the new image handling mechanisms.

Merge Readiness

This pull request represents a significant improvement to the codebase's architecture and maintainability. However, due to the critical issue identified in Program.cs (duplicate Application.Run) and the high-severity issue regarding item re-selection after renaming in FrmSetting.cs, I recommend that these changes not be merged until these issues are addressed. The medium-severity items should also be considered for improvement. I am not authorized to approve pull requests, so please ensure these points are resolved and further review is conducted before merging.

[gemini-code-assist]

When an item is renamed using the TreeView's inline editing (treeOption_AfterLabelEdit), the ItemsManager_ItemsChanged event is triggered, which reloads the TreeView. However, ItemsManager_ItemsChanged attempts to re-select the node using previouslySelectedPath, which is the old path of the item before renaming.

This means that after a successful rename, the selection might be lost or jump to an unexpected node if the old path no longer exists or if another item now coincidentally has that old path (unlikely but possible).

Have you considered how to ensure the newly renamed item is selected after the TreeView reloads? This might involve ItemsManager.RenameGroup returning the new path, or ItemsManager raising a more specific event like ItemRenamed(string oldPath, string newPath) that FrmSetting can use to update its selection target.

[gemini-code-assist]

Is the null-forgiving operator (!) necessary here for _settingsService.CurrentSettings? The SettingsService constructor initializes CurrentSettings by calling LoadSettingsFromFile(), which returns a new ListInfo() if the file doesn't exist or if an error occurs during loading. This suggests CurrentSettings should always be non-null after SettingsService instantiation.

If CurrentSettings can indeed be null under some circumstance not immediately apparent, could we add a comment explaining that scenario or handle the null case more explicitly rather than suppressing a potential warning?

[gemini-code-assist]

These lines for adding a menu item and returning were commented out:

// menu.Items.Add(group.Type.ToString(), image); 
// return; 

Was the early return intentional in the previous logic? If so, removing it changes the behavior of how items are added when a specific icon path isn't found or is equivalent to the 'Null' type image. Could you clarify if this change in behavior (processing subsequent items instead of an early exit) is intended as part of the refactor?

[gemini-code-assist]

In FrmSetting_Load, there's a check if (_settingsService.TempSettings == null). The SettingsService constructor initializes TempSettings with a deep copy of CurrentSettings, and CurrentSettings is initialized (even to an empty ListInfo) during SettingsService construction.

Could you clarify the scenario where _settingsService.TempSettings might be null at this point? If it's not expected to be null, this block might be unnecessary. If it can be null, perhaps the initialization logic in SettingsService needs to be revisited to ensure TempSettings is always initialized.

[gemini-code-assist]

The default images initialized in InitializeDefaultImages are currently new Bitmap(16,16). While fine for placeholders during development, for the actual application, should these be replaced with meaningful default icons (e.g., loaded from embedded resources or specific files) to provide a better user experience when an icon cannot be found or for default types?

[gemini-code-assist]

Error handling during settings load/save currently uses Console.WriteLine. For errors like a corrupted settings file or inability to write the file, this might not be visible or helpful to the end-user.

Should critical errors during these operations (especially loading) be communicated to the user via MyMessage.ShowError or a similar mechanism? For non-critical issues, a more robust logging framework (e.g., Serilog, NLog) would be beneficial over Console.WriteLine in a production application.