Skip to content

Refactor: Improve architecture, performance, and class organization #1

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.

Sign up for GitHub

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

Open
T5ive wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor: Improve architecture, performance, and class organization #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 29 additions & 54 deletions Toolkit Launcher/Form/FrmMain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
using Toolkit_Launcher.Models;
using Toolkit_Launcher.Services;
namespace Toolkit_Launcher;

public partial class FrmMain : Form
private readonly SettingsService _settingsService;
private readonly ImageCacheService _imageCacheService;
{
public bool Leaver = true;
private static string[] _args = { };
private static MyEnums.TargetType _targetType;

public FrmMain(string[] args)
public FrmMain(string[] args, SettingsService settingsService, ImageCacheService imageCacheService)
{
if (args == null) throw new ArgumentNullException(nameof(args));
InitializeComponent();
_settingsService = settingsService ?? throw new ArgumentNullException(nameof(settingsService));
_imageCacheService = imageCacheService ?? throw new ArgumentNullException(nameof(imageCacheService));
#if DEBUG
args = new[]
{
Expand All @@ -27,58 +33,22 @@ public FrmMain(string[] args)

private void FrmMain_Load(object sender, EventArgs e)
{
PublicClass.ImageList = imageList1;
InitializeList();
// PublicClass.ImageList = imageList1; // Removed: ImageCacheService now handles images
// InitializeList(); // Removed: SettingsService handles loading
AddItems();
AddSetting();
AddOption();
contextMenuStrip1.Show(Cursor.Position);
}

private static void InitializeList()
{
if (!File.Exists(PublicClass.PATH)) return;
LoadFile(PublicClass.PATH);
}

private static void LoadFile(string fileName)
{
try
{
PublicClass.ListInfo = LoadJson<ListInfo>(fileName);
}
catch (Exception ex)
{
MyMessage.ShowError(ex.Message);
}
finally
{
PublicClass.TempListInfo = PublicClass.ListInfo;
}
}

private static T LoadJson<T>(string fileName)
{
if (fileName.IsEmpty()) return default!;

var objectOut = default(T);

try
{
objectOut = JsonConvert.DeserializeObject<T>(File.ReadAllText(fileName));
}
catch (Exception ex)
{
MyMessage.ShowError(ex.Message);
}

return objectOut!;
}
// Removed InitializeList, LoadFile, LoadJson as SettingsService handles this

private void AddItems()
{
if (PublicClass.ListInfo == null!) return;
AddAll(contextMenuStrip1, PublicClass.ListInfo);
if (_settingsService.CurrentSettings == null!) return; // Use SettingsService
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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?

contextMenuStrip1.SuspendLayout();
AddAll(contextMenuStrip1, _settingsService.CurrentSettings); // Use SettingsService
contextMenuStrip1.ResumeLayout(false);
}

private void AddSetting()
Expand All @@ -92,7 +62,7 @@ private void AddSetting()
{
Leaver = false;
contextMenuStrip1.Hide();
var frmSetting = new FrmSetting();
var frmSetting = new FrmSetting(_settingsService, _imageCacheService);
frmSetting.ShowDialog();
frmSetting.Dispose();
Application.Exit();
Expand Down Expand Up @@ -146,13 +116,16 @@ private void AddAll(ToolStrip menu, ListInfo listInfo)
default:
{
if (string.IsNullOrWhiteSpace(group.Value.FilePath)) continue;
var image = group.Value.IconPath.GetImage();
if (image == null!)
var image = _imageCacheService.GetImage(group.Value.IconPath); // Use ImageCacheService
if (image == null || image == _imageCacheService.GetImage(MyEnums.GroupType.Null)) // Check against default null image
{
image = group.Type.GetImage();
menu.Items.Add(group.Type.ToString(), image);
return;
image = _imageCacheService.GetImage(group.Type); // Use ImageCacheService
// menu.Items.Add(group.Type.ToString(), image); // This line seems to be an early exit, might need review
// return; // This line seems to be an early exit, might need review
Comment on lines +123 to +124
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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?

}
// Ensure image is not null before creating ToolStripMenuItem, or handle it
if (image == null) image = _imageCacheService.GetImage(MyEnums.GroupType.Null);


var result = new ToolStripMenuItem(group.Identify.Name, image);
result.Click += (_, _) =>
Expand Down Expand Up @@ -197,11 +170,13 @@ private List<ToolStripItem> AddAll(GroupInfo groupInfo)
default:
{
if (string.IsNullOrWhiteSpace(group.Value.FilePath)) continue;
var image = group.Value.IconPath.GetImage();
if (image == null!)
var image = _imageCacheService.GetImage(group.Value.IconPath); // Use ImageCacheService
if (image == null || image == _imageCacheService.GetImage(MyEnums.GroupType.Null)) // Check against default null image
{
image = group.Type.GetImage();
image = _imageCacheService.GetImage(group.Type); // Use ImageCacheService
}
// Ensure image is not null before creating ToolStripMenuItem, or handle it
if (image == null) image = _imageCacheService.GetImage(MyEnums.GroupType.Null);

var item = new ToolStripMenuItem(group.Identify.Name, image);
item.Click += (_, _) =>
Expand All @@ -226,4 +201,4 @@ private void FrmMain_Deactivate(object sender, EventArgs e)
if (!Leaver) return;
Application.Exit();
}
}
}
Loading
Loading