Conversation
DaLaw2
commented
Jul 31, 2025
DaLaw2
commented
- feat: Add icon and update dependent
- fix: Type error on release mode and refactor README.md
…eamline dependency installation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds application icon support and refactors the codebase to improve error handling and code organization. It introduces a new assets module for icon loading, updates several components with better error handling patterns, and includes build configuration for Windows resource compilation.
- Adds icon loading capability using rust-embed for application branding
- Refactors error handling throughout the codebase by simplifying map_err closures
- Updates UI components with improved variable naming and configuration management
Reviewed Changes
Copilot reviewed 35 out of 38 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/assets.rs | New module for loading embedded assets, specifically app icon |
| src/core/gui_manager.rs | Integrates icon loading into application window setup |
| src/ui/schedule_page.rs | Extensive refactoring with improved error handling and variable naming |
| src/ui/execution_page.rs | Similar refactoring with better error handling patterns |
| src/platform/windows/file_system.rs | Simplified error handling and removed unused methods |
| build.rs | New build script for Windows resource compilation |
| Cargo.toml | Updated dependencies and version bump |
| tokio::fs::symlink(&source_link, destination_link) | ||
| .await | ||
| .map_err(|_| IOError::CreateSymbolLinkFailed { | ||
| src: source_link.clone(), | ||
| dst: destination_link.clone(), | ||
| .map_err(|err| { |
There was a problem hiding this comment.
The symlink function is called with source_link instead of link_target. This will create a symlink pointing to the source symlink path rather than following the original target. Should be tokio::fs::symlink(&link_target, destination_link).
| let c_path = CString::new(path.to_string_lossy().as_bytes()) | ||
| .map_err(|_| IOError::SetMetadataFailed)?; | ||
| let c_path: CString = CString::new(path.to_string_lossy().as_bytes()) | ||
| .map_err(|err| Error::IO(IOError::SetMetadataFailed(path.clone(), err)))?; |
There was a problem hiding this comment.
The error wrapping is inconsistent with the pattern used elsewhere in this function. Should use the same pattern as other error mappings: .map_err(|err| IOError::SetMetadataFailed(path.clone(), err))?
| .map_err(|err| Error::IO(IOError::SetMetadataFailed(path.clone(), err)))?; | |
| .map_err(|err| IOError::SetMetadataFailed(path.clone(), err))?; |
| Err(IOError::SetMetadataFailed( | ||
| path.clone(), | ||
| "libc chmod failed", |
There was a problem hiding this comment.
The error message 'libc chmod failed' is not descriptive enough. It should include the actual error code or reason from libc, such as using io::Error::last_os_error() to get the system error.
| Err(IOError::SetMetadataFailed( | |
| path.clone(), | |
| "libc chmod failed", | |
| let os_error = std::io::Error::last_os_error(); | |
| Err(IOError::SetMetadataFailed( | |
| path.clone(), | |
| format!("libc chmod failed: {}", os_error), |
| if libc::chown(c_path.as_ptr(), permissions.uid, permissions.gid) != 0 { | ||
| return Err(IOError::SetMetadataFailed.into()); | ||
| return Err( | ||
| IOError::SetMetadataFailed(path.clone(), "libc chmod failed").into(), |
There was a problem hiding this comment.
The error message 'libc chmod failed' is not descriptive enough. It should include the actual error code or reason from libc, such as using io::Error::last_os_error() to get the system error.
| if libc::chmod(c_path.as_ptr(), mode as mode_t) != 0 { | ||
| return Err(IOError::SetMetadataFailed.into()); | ||
| return Err( | ||
| IOError::SetMetadataFailed(path.clone(), "libc chmod failed").into(), |
There was a problem hiding this comment.
The error message 'libc chmod failed' is not descriptive enough. It should include the actual error code or reason from libc, such as using io::Error::last_os_error() to get the system error.
| IOError::SetMetadataFailed(path.clone(), "libc chmod failed").into(), | |
| IOError::SetMetadataFailed( | |
| path.clone(), | |
| format!("libc chmod failed: {}", std::io::Error::last_os_error()) | |
| ).into(), |
| Err(IOError::SetMetadataFailed( | ||
| path.clone(), | ||
| "libc utimensat failed", |
There was a problem hiding this comment.
The error message 'libc utimensat failed' is not descriptive enough. It should include the actual error code or reason from libc, such as using io::Error::last_os_error() to get the system error.
| Err(IOError::SetMetadataFailed( | |
| path.clone(), | |
| "libc utimensat failed", | |
| let os_error = std::io::Error::last_os_error(); | |
| Err(IOError::SetMetadataFailed( | |
| path.clone(), | |
| format!("libc utimensat failed: {}", os_error), |
| wMinute: epoch.minute() as u16, | ||
| wSecond: epoch.second() as u16, | ||
| wMilliseconds: (duration.subsec_nanos() / 1_000_000) as u16, | ||
| wMilliseconds: duration.subsec_millis() as u16, |
There was a problem hiding this comment.
Using duration.subsec_millis() is more efficient than the previous calculation (duration.subsec_nanos() / 1_000_000), but this is a good improvement.
