Fix Wallpaper File Lock #3986
Fix Wallpaper File Lock #3986
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes file locking issues with wallpaper file handling by implementing proper resource management and error handling. The changes ensure wallpaper files are not locked after access and prevent potential memory leaks.
Key changes:
- Replaced
objectwithLockfor thread synchronization and proper resource disposal - Implemented proper file stream disposal using explicit
usingblocks - Added registry key disposal to prevent memory leaks
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplace exclusive wallpaper file access with non-locking reads, add thread-safe freezing and cache eviction, improve registry color parsing and error handling, and tighten image loader return types and a hash-generator field type. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Settings UI (Appearance)
participant WP as WallpaperPathRetrieval
participant FS as File System
participant REG as Registry
rect rgb(250,250,255)
Note over UI,WP: Request wallpaper preview
end
UI->>WP: GetImageBrushForPath(path)
alt Cache hit (enabled)
WP->>WP: lock CacheLock\nreturn frozen ImageBrush
WP-->>UI: ImageBrush (frozen)
else Cache miss / disabled
WP->>FS: File.OpenRead(path) (using)
FS-->>WP: Stream
WP->>WP: Read image first-frame dims
alt dims == 0
WP->>REG: GetWallpaperColor()
REG-->>WP: colorString or null
WP-->>UI: SolidColorBrush (fallback)
else dims > 0
WP->>WP: Create BitmapImage (OnLoad, IgnoreColorProfile)\nbitmap.Freeze()
WP->>WP: Create & Freeze ImageBrush
WP->>WP: lock CacheLock\nmaybe evict oldest\nadd entry
WP-->>UI: ImageBrush (frozen)
end
end
alt Exception
WP->>WP: App.API.LogException(...)
WP-->>UI: Transparent / color-derived Brush
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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 (6)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs (6)
36-38: Freeze fallback brushes for thread-safety and consistency.Return values are Freezables but not frozen, unlike the image brush path. Freezing avoids accidental cross-thread usage issues and reduces overhead.
- var wallpaperColor = GetWallpaperColor(); - return new SolidColorBrush(wallpaperColor); + var wallpaperColor = GetWallpaperColor(); + var brush = new SolidColorBrush(wallpaperColor); + brush.Freeze(); + return brush;- var wallpaperColor = GetWallpaperColor(); - return new SolidColorBrush(wallpaperColor); + var wallpaperColor = GetWallpaperColor(); + var brush = new SolidColorBrush(wallpaperColor); + brush.Freeze(); + return brush;- return new SolidColorBrush(Colors.Transparent); + var transparent = new SolidColorBrush(Colors.Transparent); + transparent.Freeze(); + return transparent;Also applies to: 64-67, 109-109
42-42: Prefer UTC timestamps for cache keys and eviction ordering.Using local time can behave oddly across DST/clock changes. UTC avoids surprises.
- var dateModified = File.GetLastWriteTime(wallpaperPath); + var dateModified = File.GetLastWriteTimeUtc(wallpaperPath);Also applies to: 95-95
35-35: Severity nit: consider Warn for “invalid” wallpaper path.On some setups (solid color, slideshow, policy restrictions), an “invalid” path may be expected. Logging as Error could be noisy.
- App.API.LogError(ClassName, $"Wallpaper path is invalid: {wallpaperPath}"); + App.API.LogWarn(ClassName, $"Wallpaper path is invalid or unavailable: {wallpaperPath}");
79-79: Comment typo.It’s
OnLoad, not “OnLoaded”.- bitmap.CacheOption = BitmapCacheOption.OnLoad; // Use OnLoaded to ensure the wallpaper file is not locked + bitmap.CacheOption = BitmapCacheOption.OnLoad; // Use OnLoad to ensure the wallpaper file is not locked
121-122: Make registry color parsing robust (multiple spaces, invalid values).Current split can produce empty tokens on double spaces and will throw. Use
RemoveEmptyEntriesandTryParse.- var parts = strResult.Trim().Split([' '], 3).Select(byte.Parse).ToList(); - return Color.FromRgb(parts[0], parts[1], parts[2]); + var parts = strResult.Split(' ', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + if (parts.Length == 3 + && byte.TryParse(parts[0], out var r) + && byte.TryParse(parts[1], out var g) + && byte.TryParse(parts[2], out var b)) + { + return Color.FromRgb(r, g, b); + } + App.API.LogWarn(ClassName, $"Unexpected wallpaper color format: '{strResult}'");
69-75: Edge-case: avoid aspect-ratio mismatch if the file changes between dimension read and decode.If the file is updated between the decoder probe and the
BitmapImageload, setting both decode dimensions may stretch. Setting only one decode dimension lets WPF preserve aspect ratio automatically.- var decodedPixelWidth = (int)(originalWidth * scaleFactor); - var decodedPixelHeight = (int)(originalHeight * scaleFactor); + var decodedPixelWidth = (int)(originalWidth * scaleFactor);- bitmap.DecodePixelWidth = decodedPixelWidth; - bitmap.DecodePixelHeight = decodedPixelHeight; + bitmap.DecodePixelWidth = decodedPixelWidth; // Let WPF preserve aspect ratioAlso applies to: 82-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-28T07:47:24.148Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3279
File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46
Timestamp: 2025-02-28T07:47:24.148Z
Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
Applied to files:
Flow.Launcher/Helper/WallpaperPathRetrieval.cs
🧬 Code graph analysis (1)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs (1)
Flow.Launcher/Msg.xaml.cs (2)
System(43-46)System(62-91)
⏰ 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). (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs (4)
52-60: LGTM: early stream disposal to avoid file locking.Reading dimensions via a short-lived stream and disposing before creating the
BitmapImagehelps ensure the file isn’t held. This complementsBitmapCacheOption.OnLoad.
76-88: LGTM:OnLoad+Freezepattern to avoid file locks and enable cross-thread use.Using
BitmapCacheOption.OnLoad, setting theUriSource, thenFreeze()is the right recipe to prevent lingering file handles in WPF. This also avoids the MemoryStream pitfalls we previously observed in this code area.
81-81: Verify need forIgnoreColorProfile.Disabling color profiles can shift colors on wide‑gamut/managed displays. Keep it only if you measured perf gains here.
Would you like me to run a quick profiling plan comparing with/without
IgnoreColorProfileon common image types?
20-20: Incorrect: System.Threading.Lock is used correctly here — no change required.Repo targets net9.0-windows and the file imports System.Threading; CacheLock is a System.Threading.Lock (declared at Flow.Launcher/Helper/WallpaperPathRetrieval.cs line 20) and
lock (CacheLock)is compiled tousing (CacheLock.EnterScope())(no boxing or Monitor-based locking); readonly is fine (Lock is a reference type; EnterScope returns a ref‑struct). (learn.microsoft.com)Locations: Flow.Launcher/Helper/WallpaperPathRetrieval.cs — declaration ≈line 20, usages ≈lines 43 and 90.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
367-410: Dispose FileSvgReader — wrap in using to avoid file locks.FileSvgReader implements IDisposable (SharpVectors 1.8+); wrap its creation in a using block or call Dispose so file handles are released. Update Flow.Launcher.Infrastructure/Image/ImageLoader.cs (lines 367–410). Example: using (var reader = new FileSvgReader(drawingSettings)) { var drawing = reader.Read(new Uri(path)); }
🧹 Nitpick comments (5)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (5)
154-161: Retry path runs on caller thread; can block UI.Second attempt should also be offloaded to the thread pool.
- imageResult = GetThumbnailResult(ref path, loadFullImage); + imageResult = await Task.Run(() => GetThumbnailResult(ref path, loadFullImage));
175-196: Align remote image decode options and re-validate MemoryStream lifetime.Add IgnoreColorProfile for parity with local path loads. Also, past learning notes an issue with BitmapImage + MemoryStream even with OnLoad; please re-test remote images for file/stream lifetime issues.
var image = new BitmapImage(); image.BeginInit(); image.CacheOption = BitmapCacheOption.OnLoad; + image.CreateOptions = BitmapCreateOptions.IgnoreColorProfile; if (!loadFullImage) { image.DecodePixelHeight = SmallIconSize; image.DecodePixelWidth = SmallIconSize; }Test: load a remote image, GC collect, and verify no stream/file handle remains referenced.
215-232: Broaden exception handling around LoadFullImage.new Uri(path) and decoding can throw UriFormatException/FileFormatException; currently only NotSupportedException is caught.
- catch (NotSupportedException ex) + catch (Exception ex) when (ex is NotSupportedException or UriFormatException or FileFormatException) { image = Image; type = ImageType.Error; Log.Exception(ClassName, $"Failed to load image file from path {path}: {ex.Message}", ex); }
330-365: Avoid double I/O by choosing decode size before first load.Current flow loads full-size, then may reload with DecodePixelWidth/Height. Use a BitmapDecoder to read dimensions first, then build a single BitmapImage with the right DecodePixel*.
I can draft a decoder-based version if you want it in this PR.
24-24: Name nit: GuidToKey isn’t GUID-based.It stores image hashes (not GUIDs). Consider HashToKey to avoid confusion.
Also applies to: 308-317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs(4 hunks)Flow.Launcher/Helper/WallpaperPathRetrieval.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Helper/WallpaperPathRetrieval.cs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3279
File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46
Timestamp: 2025-02-28T07:47:24.148Z
Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
⏰ 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: build
🔇 Additional comments (2)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (2)
34-34: No change required — repo targets net9.0 so C# 12 collection expressions are supportedFlow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj (and other project files) contain net9.0-windows, so the array-literal/spread syntax is supported by the toolchain; no fallback needed. Add an explicit or revert only if CI uses an older SDK.
22-22: Fix incorrect use of System.Threading.Lock (won’t compile with lock statement).System.Threading.Lock is a value type; C# lock requires a reference-type monitor. Replace the lock(...) usage — either use Lock.EnterScope() correctly or switch to an object monitor.
File: Flow.Launcher.Infrastructure/Image/ImageLoader.cs — declaration at line 22 and lock blocks at lines ~74–85 and ~89–92.
Option A — keep System.Threading.Lock:
- private static Lock storageLock { get; } = new(); + private static readonly System.Threading.Lock storageLock = new(); public static void Save() { - lock (storageLock) - { + using var _ = storageLock.EnterScope(); + try + { try { _storage.Save([.. ImageCache.EnumerateEntries().Select(x => x.Key)]); } catch (System.Exception e) { Log.Exception(ClassName, "Failed to save image cache to file", e); } - } + } } private static List<(string, bool)> LoadStorageToConcurrentDictionary() { - lock (storageLock) - { - return _storage.TryLoad([]); - } + using var _ = storageLock.EnterScope(); + return _storage.TryLoad([]); }Option B — revert to object monitor:
- private static Lock storageLock { get; } = new(); + private static readonly object storageLock = new();⛔ Skipped due to learnings
Learnt from: Jack251970 PR: Flow-Launcher/Flow.Launcher#3279 File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46 Timestamp: 2025-02-28T07:47:24.148Z Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
This reverts commit 9a597f2.
|
🥷 Code experts: onesounds Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Tested and good to go. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs (1)
79-82: Correct use ofBitmapCacheOption.OnLoadto avoid file locks.Setting
OnLoadwithUriSourceensures the decoder reads and releases the handle atEndInit.IgnoreColorProfileis fine given perf goals.
🧹 Nitpick comments (4)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs (4)
35-38: Freeze the fallback brush to avoid UI-thread-only affinity and reduce overhead.The created
SolidColorBrushis mutable and not frozen. Freezing improves perf and avoids accidental cross-thread usage.Apply this diff:
- var wallpaperColor = GetWallpaperColor(); - return new SolidColorBrush(wallpaperColor); + var wallpaperColor = GetWallpaperColor(); + var brush = new SolidColorBrush(wallpaperColor); + brush.Freeze(); + return brush;Optional: consider downgrading to
LogWarningfor “invalid wallpaper path” to reduce noise if this can occur during normal user actions (e.g., temporary file swaps). No code change required now.
52-60: Comment is misleading re:using varvsusing (...) {}scope.It’s about scoping/lifetime, not the keyword. Both forms dispose at scope end; here you’re intentionally constraining the stream’s lifetime to this block so the file handle closes before the
BitmapImageloads viaUriSource.Apply this diff to clarify:
- // Use `using ()` instead of `using var` sentence here to ensure the wallpaper file is not locked + // Limit FileStream lifetime to this block so the handle closes before BitmapImage loads via UriSource. + // (Either `using var` or `using (...) {}` works if scoped appropriately.)
64-67: Freeze the “zero-size fallback” brush too.Same reason as above; freeze for safety/perf.
Apply this diff:
- var wallpaperColor = GetWallpaperColor(); - return new SolidColorBrush(wallpaperColor); + var wallpaperColor = GetWallpaperColor(); + var brush = new SolidColorBrush(wallpaperColor); + brush.Freeze(); + return brush;Additionally (outside this hunk), consider freezing the brush in the catch path:
var brush = new SolidColorBrush(Colors.Transparent); brush.Freeze(); return brush;
121-123: Harden registry color parsing; apply Split+TryParse fix.
- Replace the snippet in Flow.Launcher/Helper/WallpaperPathRetrieval.cs (≈lines 121–123) with the proposed diff: use Split(' ', StringSplitOptions.RemoveEmptyEntries) and byte.TryParse to avoid exceptions from multiple spaces or malformed data.
- The [' '] collection-expression is a C# 12 feature; this repo targets net9.0 (defaults to C# 13) so the syntax is allowed, but prefer the explicit char overload for clarity. (learn.microsoft.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-28T07:47:24.148Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3279
File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46
Timestamp: 2025-02-28T07:47:24.148Z
Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
Applied to files:
Flow.Launcher/Helper/WallpaperPathRetrieval.cs
⏰ 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: build
🔇 Additional comments (1)
Flow.Launcher/Helper/WallpaperPathRetrieval.cs (1)
115-115: Good: ensure registry key is disposed.
using varon the registry key prevents handle leaks. LGTM.
Fix Wallpaper File Lock
Fix Wallpaper File Lock
LockforCacheLockLogErrorinstead ofLogInfousing ()sentence so that file is not lockedOnLoadedto ensure the wallpaper file is not lockedusingsentence for registry key to fix possible memory leakFix #3983
Test
Open Flow Launcher -> Open setting window -> Navigate to appearance page -> Wallpaper file is not locked