Conversation
oscartbeaumont
changed the title
WalkthroughAdded multi-path, high-resolution Windows icon extraction in win.rs: new private helpers for shell and executable icon enumeration, larger size threshold, expanded HICON→PNG sizing strategy, and reordered fallbacks. Also enabled additional WinRT graphics features in scap-targets/Cargo.toml. Public API unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as WindowImpl::app_icon
participant S as Shell (SHGetFileInfoW)
participant E as Executable (ExtractIconExW / PrivateExtractIconsW)
participant R as Renderer (hicon_to_png)
participant F as Legacy Fallbacks
rect rgba(200,230,255,0.25)
note over W,S: Method 1 — Shell high-res attempt
W->>S: SHGetFileInfoW (large/small)
S-->>W: HICON (maybe)
alt HICON returned
W->>R: Convert HICON→PNG (high-res sizes)
R-->>W: PNG (accept if >= threshold)
end
end
rect rgba(200,255,200,0.25)
note over W,E: Method 2 — Executable icon enumeration
W->>E: ExtractIconExW / PrivateExtractIconsW (enumerate sizes)
E-->>W: HICON list
loop evaluate icons
W->>R: Convert candidate HICON→PNG
R-->>W: PNG (measure/choose best)
end
end
rect rgba(255,245,200,0.25)
note over W,F: Methods 3–6 — Prior fallback paths
W->>F: Try legacy large/default icon extraction
F-->>W: HICON/PNG if available
end
opt No satisfactory icon
W-->>W: Return best available PNG or None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
crates/scap-targets/src/platform/win.rs (2)
566-607: Drop the redundant “Method 4” block.This re-implements a subset of extract_executable_icons_high_res logic (index 0 only), duplicating work and increasing latency.
Apply this diff to remove the duplicate block:
- // Method 4: Try executable file extraction (fallback to original method) - if let Some(exe_path) = self.get_executable_path() { - let wide_path: Vec<u16> = - exe_path.encode_utf16().chain(std::iter::once(0)).collect(); - - let mut large_icon: HICON = HICON::default(); - let mut small_icon: HICON = HICON::default(); - - let extracted = ExtractIconExW( - PCWSTR(wide_path.as_ptr()), - 0, // Only try the first (main) icon - Some(&mut large_icon), - Some(&mut small_icon), - 1, - ); - - if extracted > 0 { - // Try large icon first - if !large_icon.is_invalid() { - if let Some(result) = self.hicon_to_png_bytes_optimized(large_icon) { - let _ = DestroyIcon(large_icon); - if !small_icon.is_invalid() { - let _ = DestroyIcon(small_icon); - } - // Return immediately if we got a good quality icon - if result.1 >= GOOD_SIZE_THRESHOLD { - return Some(result.0); - } - } - let _ = DestroyIcon(large_icon); - } - - // Try small icon as fallback - if !small_icon.is_invalid() { - if let Some(result) = self.hicon_to_png_bytes_optimized(small_icon) { - let _ = DestroyIcon(small_icon); - return Some(result.0); - } - let _ = DestroyIcon(small_icon); - } - } - }
816-835: Don’t try upscaling first; attempt native size before larger targets.This reduces work and avoids returning upscaled bitmaps when higher-res isn’t available.
Apply this diff:
- let target_sizes = if let Some((width, height)) = native_size { - let native_dim = width.max(height); - if native_dim >= 512 { - vec![native_dim, 512, 256, 128] // Ultra high-res icon - } else if native_dim >= 256 { - vec![512, native_dim, 256, 128] // Try upscaling first, then native - } else if native_dim >= 128 { - vec![512, 256, native_dim, 128] // Try higher res first - } else if native_dim >= 64 { - vec![256, 128, native_dim, 64] // Medium-res icon - } else if native_dim >= 32 { - vec![128, 64, native_dim, 32] // Standard icon - } else { - vec![64, 32, 16] // Small icon, try reasonable sizes - } - } else { - // No native size info, try high-res defaults first - vec![512, 256, 128, 64, 32, 16] - }; + let target_sizes = if let Some((width, height)) = native_size { + let native_dim = width.max(height); + if native_dim >= 512 { + vec![native_dim, 256, 128, 64, 32, 16] + } else if native_dim >= 256 { + vec![native_dim, 256, 128, 64, 32, 16] + } else if native_dim >= 128 { + vec![native_dim, 128, 64, 32, 16] + } else if native_dim >= 64 { + vec![native_dim, 64, 32, 16] + } else if native_dim >= 32 { + vec![native_dim, 32, 16] + } else { + vec![16] + } + } else { + vec![256, 128, 64, 32, 16] + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/scap-targets/src/platform/win.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/scap-targets/src/platform/win.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/scap-targets/src/platform/win.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/scap-targets/src/platform/win.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/scap-targets/src/platform/win.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
crates/scap-targets/src/platform/win.rs (2)
19-21: Imports look correct and necessary.These are used by get_file_description and the new shell icon path.
34-37: Shell imports LGTM.Needed for SHGetFileInfoW and ExtractIconExW paths.
| } | ||
|
|
||
| // Method 3: Try small window icon as fallback | ||
| // Method 5: Try small window icon as fallback |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove added inline comment (“Method 5”).
Per repo guidelines, don’t add inline comments in Rust files.
- // Method 5: Try small window icon as fallback📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Method 5: Try small window icon as fallback |
🤖 Prompt for AI Agents
In crates/scap-targets/src/platform/win.rs around line 609, there is an inline
comment "// Method 5: Try small window icon as fallback" which violates the repo
guideline prohibiting inline comments in Rust files; remove that comment line
(or move any necessary context to the surrounding function-level documentation
or commit message) so the code contains no inline comment at that location.
| } | ||
|
|
||
| // Method 4: Try class icon as last resort | ||
| // Method 6: Try class icon as last resort |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove added inline comment (“Method 6”).
Per repo guidelines, don’t add inline comments in Rust files.
- // Method 6: Try class icon as last resort📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Method 6: Try class icon as last resort |
🤖 Prompt for AI Agents
In crates/scap-targets/src/platform/win.rs around line 623, remove the newly
added inline comment text "Method 6: Try class icon as last resort" so the Rust
file contains no inline comment per repo guidelines; simply delete that comment
line (or convert it into a non-inline doc or external note if context must be
preserved) and ensure formatting/lints remain clean.
Switches from ExtractIconExW to PrivateExtractIconsW for better icon quality and size selection. Refactors icon extraction and conversion routines to prioritize higher resolution icons and optimize PNG output, improving reliability and visual fidelity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
crates/scap-targets/src/platform/win.rs (5)
559-564: Critical: Use native HICON dimensions, not rendered draw size, as quality threshold.Lines 561 and 592 compare
result.1(the rendered/requested draw size) againstGOOD_SIZE_THRESHOLD. This accepts upscaled icons as high-quality. A 32×32 native icon rendered at 256×256 will pass the check, defeating the PR's quality goal.Call
get_icon_size(hicon)before conversion and compare the native dimension against the threshold instead.if large_icon.0 != 0 { - if let Some(result) = self.hicon_to_png_bytes_optimized(HICON(large_icon.0 as _)) { - // If we got a good quality icon, return it immediately - if result.1 >= GOOD_SIZE_THRESHOLD { - return Some(result.0); + let hicon = HICON(large_icon.0 as _); + let native_dim = self.get_icon_size(hicon) + .map(|(w, h)| w.max(h)) + .unwrap_or(0); + if let Some((bytes, _)) = self.hicon_to_png_bytes_optimized(hicon) { + if native_dim >= GOOD_SIZE_THRESHOLD { + return Some(bytes); } } }Apply similar fixes to line 592 and the new extraction methods at lines 659, 725.
Also applies to: 586-594
536-536: Remove all inline comments per repository guidelines.The repository's coding guidelines explicitly prohibit inline, block, or docstring comments in Rust files. The code must be self-explanatory. Remove all newly added comment lines including "Method 1" through "Method 6" annotations and other inline comments throughout the new extraction methods.
Based on coding guidelines.
Also applies to: 543-543, 550-550, 567-567, 610-610, 624-624, 640-640, 660-660
536-548: Cacheget_executable_path()result to avoid repeated calls.The method
get_executable_path()is called three times (lines 537, 544, 568). Cache the result once and reuse it across all three extraction attempts.Apply this diff:
- // Method 1: Try shell icon extraction for highest quality - if let Some(exe_path) = self.get_executable_path() { - if let Some(icon_data) = self.extract_shell_icon_high_res(&exe_path, 512) { + let exe_path = self.get_executable_path(); + if let Some(ref exe_path) = exe_path { + if let Some(icon_data) = self.extract_shell_icon_high_res(exe_path, 512) { return Some(icon_data); } } - // Method 2: Try executable file extraction with multiple icon sizes - if let Some(exe_path) = self.get_executable_path() { - if let Some(icon_data) = self.extract_executable_icons_high_res(&exe_path) { + if let Some(ref exe_path) = exe_path { + if let Some(icon_data) = self.extract_executable_icons_high_res(exe_path) { return Some(icon_data); } } - // Method 3: Try to get the window's large icon + let large_icon = SendMessageW( self.0, WM_GETICON, @@ -564,8 +562,7 @@ } } - // Method 4: Try executable file extraction (fallback to original method) - if let Some(exe_path) = self.get_executable_path() { + if let Some(ref exe_path) = exe_path { let wide_path: Vec<u16> = exe_path.encode_utf16().chain(std::iter::once(0)).collect();Also applies to: 567-608
636-670: Critical: Double-destroy of HICON and quality proxy issue.Two critical bugs in this method:
Double-destroy: When conversion succeeds but the size check fails (line 659), the HICON is destroyed at line 658, then destroyed again at line 664, causing undefined behavior.
Quality proxy: Line 659 checks
result.1(rendered draw size) instead of native HICON dimensions, accepting upscaled icons.Apply this fix:
fn extract_shell_icon_high_res(&self, exe_path: &str, target_size: i32) -> Option<Vec<u8>> { unsafe { let wide_path: Vec<u16> = exe_path.encode_utf16().chain(std::iter::once(0)).collect(); - // Try different shell icon sizes let icon_flags = [ - SHGFI_ICON | SHGFI_LARGEICON, // Large system icon - SHGFI_ICON | SHGFI_SMALLICON, // Small system icon as fallback + SHGFI_ICON | SHGFI_LARGEICON, + SHGFI_ICON | SHGFI_SMALLICON, ]; for flags in icon_flags { let mut file_info = SHFILEINFOW::default(); let result = SHGetFileInfoW( windows::core::PCWSTR(wide_path.as_ptr()), FILE_FLAGS_AND_ATTRIBUTES(0), Some(&mut file_info), std::mem::size_of::<SHFILEINFOW>() as u32, flags, ); if result != 0 && !file_info.hIcon.is_invalid() { - if let Some(result) = self.hicon_to_png_bytes_optimized(file_info.hIcon) { - let _ = DestroyIcon(file_info.hIcon); - if result.1 >= target_size / 2 { - // Accept if at least half target size - return Some(result.0); - } - } - let _ = DestroyIcon(file_info.hIcon); + let hicon = file_info.hIcon; + let native_dim = self.get_icon_size(hicon) + .map(|(w, h)| w.max(h)) + .unwrap_or(0); + let converted = self.hicon_to_png_bytes_optimized(hicon); + let _ = DestroyIcon(hicon); + if let Some((bytes, _)) = converted { + if native_dim >= target_size / 2 { + return Some(bytes); + } + } } } None } }
672-735: Critical: Best icon selection uses rendered size instead of native dimensions.Lines 721-725 track
best_sizeusingrealized_sizefrom the tuple (the rendered draw size), not the native HICON bitmap dimensions. This causes the method to select upscaled icons over genuinely high-resolution ones.Use
get_icon_size(icon_handle)to determine native dimensions for comparison and early-exit logic.fn extract_executable_icons_high_res(&self, exe_path: &str) -> Option<Vec<u8>> { unsafe { let wide_path: Vec<u16> = exe_path.encode_utf16().chain(std::iter::once(0)).collect(); let mut path_buffer = [0u16; 260]; let copy_len = wide_path.len().min(path_buffer.len()); path_buffer[..copy_len].copy_from_slice(&wide_path[..copy_len]); let icon_count = ExtractIconExW(PCWSTR(wide_path.as_ptr()), -1, None, None, 0); let total_icons = if icon_count > 0 { icon_count as usize } else { 1 }; let max_icons_to_try = total_icons.min(8); let size_candidates: [i32; 12] = [512, 400, 256, 192, 128, 96, 72, 64, 48, 32, 24, 16]; let mut best_icon: Option<Vec<u8>> = None; - let mut best_size: i32 = 0; + let mut best_native: i32 = 0; for &size in &size_candidates { for index in 0..max_icons_to_try { let mut icon_slot = [HICON::default(); 1]; let extracted = PrivateExtractIconsW( &path_buffer, index as i32, size, size, Some(&mut icon_slot), None, 0, ); if extracted == 0 { continue; } let icon_handle = icon_slot[0]; if icon_handle.is_invalid() { continue; } + let native_dim = self.get_icon_size(icon_handle) + .map(|(w, h)| w.max(h)) + .unwrap_or(0); let icon_result = self.hicon_to_png_bytes_optimized(icon_handle); let _ = DestroyIcon(icon_handle); - if let Some((png_data, realized_size)) = icon_result { - if realized_size > best_size { - best_size = realized_size; + if let Some((png_data, _)) = icon_result { + if native_dim > best_native { + best_native = native_dim; best_icon = Some(png_data); - if best_size >= 256 { + if best_native >= 256 { return best_icon; } } } } } best_icon } }
🧹 Nitpick comments (1)
crates/scap-targets/src/platform/win.rs (1)
796-856: Return native dimensions in tuple to fix quality proxy issue throughout codebase.The method returns
(Vec<u8>, i32)where thei32is the requested/rendered draw size (line 845), not the native HICON bitmap dimensions. This propagates the quality proxy bug to all callers.Consider returning
(Vec<u8>, i32, i32)where the third element is the native dimension, or restructure to return a struct with named fields for clarity.Example signature change:
fn hicon_to_png_bytes_optimized(&self, icon: HICON) -> Option<(Vec<u8>, i32, i32)> { // ... // return Some((png_data, requested_size, native_dim)) }Then update all call sites to use the third element for quality checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/scap-targets/Cargo.toml(1 hunks)crates/scap-targets/src/platform/win.rs(12 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/scap-targets/src/platform/win.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/scap-targets/src/platform/win.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/scap-targets/src/platform/win.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/scap-targets/src/platform/win.rs
🧬 Code graph analysis (1)
crates/scap-targets/src/platform/win.rs (1)
crates/recording/src/cursor.rs (3)
std(236-236)std(266-266)std(295-295)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
crates/scap-targets/Cargo.toml (1)
27-29: LGTM — WinRT features added to support enhanced icon extraction.The three new Windows features enable the richer Shell/FileInfo handling and graphics capabilities used by the updated icon extraction logic in win.rs.
crates/scap-targets/src/platform/win.rs (1)
19-21: LGTM — imports added for high-resolution icon extraction.The new Shell and FileSystem imports support the multi-path icon extraction logic introduced in this PR.
Also applies to: 34-37, 44-46
2 participants
This needs more testing because I am concerned it's going to slow down the logo collection process again but I think it'll be alright.
Summary by CodeRabbit
New Features
Bug Fixes
Chores