Fix spurious folder icon overlay #1977
Conversation
WalkthroughAdds a Windows-specific icon extraction path to ImFileDialog: introduces an internal Image struct and GetFileIconWin32 helper that uses SHGetImageList + ImageList_GetIcon, converts icons to a pixel buffer, and creates a texture. Keeps the existing non-Windows fallback. Changes
Sequence Diagram(s)sequenceDiagram
participant ImFD as ImFileDialog
participant WinAPI as Windows API
participant ImgConv as Icon → Pixels
participant GPU as Texture Creator
ImFD->>ImFD: m_getIcon(request)
alt Windows path available
ImFD->>WinAPI: GetFileIconWin32(SHGetImageList, ImageList_GetIcon)
WinAPI-->>ImgConv: HICON
ImgConv-->>ImFD: Image{Pixels, W,H,Channels}
ImFD->>GPU: create texture from Image.Pixels
GPU-->>ImFD: Texture handle
else fallback
ImFD->>ImFD: existing GetIconInfo / GetBitmapBits path
ImFD->>GPU: create texture from fallback pixels
GPU-->>ImFD: Texture handle
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
third_party/ImFileDialog/ImFileDialog.cpp (1)
49-99: Solid implementation with proper resource management.The function correctly implements the fix by retrieving icons from the system image list, which should eliminate the overlay artifacts. Resource cleanup is properly handled with
DeleteObjectandDestroyIconcalled regardless of the success path.Optional: Overflow safety on line 79
The size calculation could theoretically overflow with very large dimensions:
if (const auto size = image.Width * image.Height * (ds.dsBm.bmBitsPixel / 8); size != 0)While unlikely with icon sizes, defensive coding could use checked multiplication or validate dimensions first. This is a low-priority refinement.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
third_party/ImFileDialog/ImFileDialog.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Code Health Review (main)
third_party/ImFileDialog/ImFileDialog.cpp
[warning] 49-99: ❌ New issue: Deep, Nested Complexity
GetFileIconWin32 has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
[notice] 818-823: ✅ Getting better: Complex Method
FileDialog::m_getIcon decreases in cyclomatic complexity from 17 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
⏰ 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). (10)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: build-openbios
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: asan
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (3)
third_party/ImFileDialog/ImFileDialog.cpp (3)
40-46: LGTM: Clean structure design.The
Imagestructure appropriately encapsulates icon pixel data with automatic memory management viastd::vector.
818-823: LGTM: Clean integration of the icon fix.The modification correctly uses the new
GetFileIconWin32helper and properly handles the optional return value. This change should resolve the spurious folder icon overlay issue described in the PR.
20-21: The review comment is based on incorrect information.The header
commoncontrols.his a standard Windows SDK header that exposes image-list COM interfaces (IImageList, IImageList2), ImageList_CoCreateInstance, and related structures. It is not redundant withcommctrl.h—the two headers serve complementary purposes: commctrl.h provides the classic Common Controls API (traditional control functions, messages, InitCommonControlsEx, and window-class names), while commoncontrols.h is focused on COM-style common-controls interfaces.Both headers are typically used together, with commoncontrols.h included when you need the COM interfaces alongside commctrl.h for traditional control APIs. The code is correct as-is and requires both headers.
Likely an incorrect or invalid review comment.
aybe
force-pushed
the
fix-icon-overlay
branch
from
b05d2b1 to
79de997
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
third_party/ImFileDialog/ImFileDialog.cpp (1)
49-103: Consider validating bitmap format and documenting color channel ordering.The function extracts bitmap bits directly without checking the format or performing color conversion. Windows bitmaps are typically in BGRA format, which might differ from what
CreateTextureexpects.Consider adding:
- Validation that
bmBitsPixelis 32 (expected for icons)- Documentation or assertion about expected color format (BGRA vs RGBA)
- Comment explaining why we use
SHGetImageList+ImageList_GetIconinstead of the icon fromSHGetFileInfoWdirectly (to avoid overlay issues)Example validation:
if (GetObject(info.hbmColor, sizeof(ds), &ds)) { const auto bm = ds.dsBm; + // Ensure we have a 32-bit BGRA bitmap + if (bm.bmBitsPixel != 32) + return std::nullopt; + const auto bw = bm.bmWidth; const auto bh = bm.bmHeight;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
third_party/ImFileDialog/ImFileDialog.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Code Health Review (main)
third_party/ImFileDialog/ImFileDialog.cpp
[warning] 49-103: ❌ New issue: Deep, Nested Complexity
GetFileIconWin32 has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
[notice] 822-827: ✅ Getting better: Complex Method
FileDialog::m_getIcon decreases in cyclomatic complexity from 17 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
⏰ 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). (9)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
🔇 Additional comments (2)
third_party/ImFileDialog/ImFileDialog.cpp (2)
822-827: LGTM! Clean integration of the new icon extraction approach.The integration properly handles the optional return value from
GetFileIconWin32and creates the texture with the extracted pixel data. This approach usingSHGetImageListshould resolve the spurious folder icon overlay issue.
20-24: The review comment is factually incorrect.
commoncontrols.his a valid standard Windows SDK header that exposes COM-based common-controls interfaces, and bothcommctrl.handcommoncontrols.hship in the Windows SDK. The assertion that<commoncontrols.h>is "not a standard Windows SDK header" is false.While the practical question of whether this code actually requires both headers could merit investigation, the review comment's technical premise is incorrect.
Likely an incorrect or invalid review comment.
2 participants
For some reason which I couldn't figure out, when clicking some folders such as AMD, an overlay would be shown on the icon.
This looked terrible because folders icons would constantly change according where you browse to.
Before:
After: