Load images into ImageCache & Use non-async method in ImageCache #3898
Conversation
|
🥷 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:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes image loading performance by preloading constant images into ImageCache as frozen resources and removes async operations from ImageCache. The changes simplify the codebase by eliminating the need to wait for async image saving operations during app lifecycle events.
- Preloads constant images (
ImageIcon,LoadingImgIcon) into ImageCache as frozen resources - Replaces async image cache operations with synchronous ones using Lock instead of SemaphoreSlim
- Removes
ImageLoader.WaitSaveAsync()calls from app restart and shutdown sequences
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| PublicAPIInstance.cs | Removes async from RestartApp() and replaces async image saving with synchronous Save() |
| MainWindow.xaml.cs | Removes ImageLoader using statement and WaitSaveAsync() call from shutdown |
| BinaryStorage.cs | Adds synchronous TryLoad() and Save() methods, improves error handling |
| ImageLoader.cs | Replaces async operations with sync, preloads constant images, uses Lock instead of SemaphoreSlim |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📝 WalkthroughWalkthroughCentralizes image sources in ImageCache, reworks ImageLoader initialization and concurrency to use synchronous storage load/save with locks, expands preloaded icons, and removes async wait/save from shutdown/restart. Adds synchronous load/save/clear helpers to BinaryStorage and updates shutdown/restart call sites accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Img as ImageLoader
participant Store as BinaryStorage
participant Cache as ImageCache
App->>Img: InitializeAsync()
activate Img
Img->>Store: TryLoad(defaultUsage)
Store-->>Img: usageData
Img->>Store: ClearData()
Img->>Cache: Initialize(usageData)
Img->>Cache: Preload(DefaultIcon, ImageIcon, MissingImgIcon, LoadingImgIcon)
Img-->>App: Initialized
deactivate Img
sequenceDiagram
participant UI as UI/Shutdown/Restart
participant API as PublicAPIInstance
participant Img as ImageLoader
participant Store as BinaryStorage
UI->>API: SaveAppAllSettings()
API->>Img: Save() (synchronous)
Img->>Store: Save(data)
Store-->>Img: Saved
API-->>UI: Done
UI->>API: RestartApp()
API-->>UI: Restart (no await on image save)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (1)
15-15: Fix typo in documentation comment.There's a typo in the XML documentation comment.
- /// Stroage object using binary data + /// Storage object using binary data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs(5 hunks)Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs(3 hunks)Flow.Launcher/MainWindow.xaml.cs(0 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher/MainWindow.xaml.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.361Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
📚 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.Infrastructure/Image/ImageLoader.cs
🧬 Code Graph Analysis (3)
Flow.Launcher/PublicAPIInstance.cs (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
RestartApp(34-34)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
RestartApp(27-30)Flow.Launcher.Infrastructure/Image/ImageLoader.cs (2)
ImageLoader(17-411)Save(72-85)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (6)
Flow.Launcher.Infrastructure/Helper.cs (1)
T(12-22)Flow.Launcher.Infrastructure/Logger/Log.cs (5)
Log(13-168)Log(19-64)Error(140-143)Info(159-162)Exception(97-107)Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
Save(72-85)Flow.Launcher.Infrastructure/Storage/FlowLauncherJsonStorage.cs (1)
Save(24-34)Flow.Launcher.Infrastructure/Storage/PluginBinaryStorage.cs (1)
Save(22-32)Flow.Launcher.Plugin/Interfaces/ISavable.cs (1)
Save(19-19)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (3)
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (6)
BinaryStorage(21-169)BinaryStorage(34-36)BinaryStorage(38-44)ClearData(165-168)Save(135-138)Save(140-147)Flow.Launcher.Infrastructure/Image/ImageCache.cs (1)
ImageCache(10-79)Flow.Launcher.Infrastructure/Constant.cs (1)
Constant(7-59)
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs
[warning] 15-16: Unrecognized spelling: 'Stroage'.
Flow.Launcher.Infrastructure/Image/ImageLoader.cs
[warning] 406-406: Unrecognized spelling: 'Pbgra'.
⏰ 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 workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (14)
Flow.Launcher/PublicAPIInstance.cs (2)
77-90: LGTM! Synchronous restart flow is appropriate here.The removal of
asyncfromRestartApp()is correct since the method performs synchronous operations and callsEnvironment.Exit(0)viaUpdateManager.RestartApp(). The synchronousSaveAppAllSettings()call ensures all settings are persisted before the application terminates.
108-117: LGTM! Proper synchronization of save operations.The addition of
ImageLoader.Save()ensures image cache is persisted along with other application settings. The lock properly synchronizes all save operations to prevent concurrent modifications.Flow.Launcher.Infrastructure/Storage/BinaryStorage.cs (6)
56-79: LGTM! Well-structured synchronous load method with proper error handling.The
TryLoadmethod implementation correctly handles all edge cases:
- Returns cached data if already loaded
- Handles zero-length files with appropriate logging and recovery
- Creates default data for missing files
- Properly deserializes existing files with error recovery
81-93: LGTM! Robust deserialization with proper error recovery.The private
Deserializemethod properly handles deserialization errors by logging the exception and returning the default data as a fallback, ensuring the application continues to function even with corrupted cache files.
121-133: LGTM! Consistent async deserialization implementation.The async deserialization helper maintains consistency with the synchronous version, properly handling errors and returning default data as fallback.
135-138: LGTM! Proper delegation to parameterized Save method.The parameterless
Save()method correctly delegates toSave(T data)usingNonNull()extension to ensure data is not null.
140-147: LGTM! Clean synchronous save implementation.The new
Save(T data)method provides a clean synchronous API that validates the directory exists and serializes the data efficiently using MemoryPack.
163-168: LGTM! Useful utility for clearing in-memory cache.The
ClearData()method provides a clean way to reset the in-memory cache, which is particularly useful for the ImageCache implementation that needs to manage memory efficiently.Flow.Launcher.Infrastructure/Image/ImageLoader.cs (6)
22-22: LGTM! Appropriate switch to Lock for synchronous operations.The change from
SemaphoreSlimtoLockis appropriate since all storage operations are now synchronous. TheLocktype provides better performance for synchronous scenarios.
27-29: LGTM! Clever centralization of image sources in ImageCache.Moving the image sources (Image, MissingImage, LoadingImage) to be computed from ImageCache ensures these resources are properly frozen and managed centrally, improving performance and consistency.
39-57: LGTM! Well-structured initialization with proper preloading.The initialization properly:
- Sets up storage in a background task
- Loads existing cache entries
- Clears storage data to free memory
- Initializes ImageCache
- Preloads essential icons as frozen resources
This approach ensures optimal memory usage and performance.
72-85: LGTM! Clean synchronous save implementation with proper error handling.The synchronous
Save()method properly:
- Acquires the lock to ensure thread safety
- Saves the current cache entries using LINQ to extract keys
- Handles exceptions gracefully with logging
87-93: LGTM! Consistent synchronous storage loading.The change from async to synchronous loading aligns with the overall refactoring and properly uses the lock for thread safety.
125-127: LGTM! Consistent use of MissingImage property.All references to missing image icons now consistently use the
MissingImageproperty instead of direct access, ensuring centralized management through ImageCache.Also applies to: 166-168, 265-266
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
Load images into ImageCache
Load
Constant.ImageIcon&Constant.LoadingImgIconso that they can be freeze resources which can improve performance.Use non-async method in ImageCache
Add non-async method in
BinaryStorageso thatImageCachecan use it to remove async methods.And we can remove
ImageLoader.WaitSaveAsync()to improve code qualityTest