Support Svg File Icon Loading & Local favicon for bookmark plugin #3361
Conversation
This comment has been minimized.
This comment has been minimized.
|
@onesounds Similar to #3079? Should we close it? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I made it to do that. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit cab0cb8.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![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 (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (2)
105-195: Well-structured favicon loading implementation.The new
LoadFaviconsFromDbmethod is well-structured with appropriate error handling at both the method and individual bookmark level. It properly creates a temporary copy of the favicon database to avoid lock issues, processes each bookmark's favicon, and cleans up resources.Consider implementing a cache invalidation mechanism to manage favicon cache growth over time, as mentioned in your PR description.
// Method to add to Main.cs public static void ClearFaviconCache() { if (Directory.Exists(_faviconCacheDir)) { try { foreach (var file in Directory.GetFiles(_faviconCacheDir, "firefox_*.*")) { File.Delete(file); } _context.API.LogInfo("BrowserBookmark", "Firefox favicon cache cleared"); } catch (Exception ex) { _context.API.LogException("BrowserBookmark", "Failed to clear favicon cache", ex); } } }
56-59: Consider using a more secure approach for temporary file handling.The current implementation creates temporary files with GUIDs, which is good for uniqueness, but you might want to consider using
Path.GetTempFileName()or implementing a cleanup mechanism in case exceptions occur during processing.-var tempDbPath = Path.Combine(_faviconCacheDir, $"tempplaces_{Guid.NewGuid()}.sqlite"); -File.Copy(placesPath, tempDbPath, true); +var tempDbPath = Path.GetTempFileName(); +File.Copy(placesPath, tempDbPath, true);Also applies to: 87-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (4)
Main(16-255)List(56-116)List(205-235)RegisterBookmarkFile(142-175)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Report (PR)
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (5)
12-19: Good class structure with proper initialization.The addition of a static class name for logging and a properly initialized favicon cache directory through constructor injection are both good practices. This makes the code more maintainable and follows appropriate logging patterns.
35-103: Significantly improved error handling and resource management.The refactored
GetBookmarksFromPathmethod now includes proper error handling with nested try-catch blocks, uses a temporary database file to avoid lock issues, and correctly cleans up resources. The method signature changed from static to instance, which is appropriate given the instance field_faviconCacheDiris now used.The approach of copying the database file to a temporary path to avoid lock conflicts is well-implemented, with proper error handling and resource cleanup.
197-207: Good utility method with proper error handling.The
SaveBitmapDatamethod is well-implemented with appropriate error handling and logging.
209-216: Effective SVG detection implementation.The
IsSvgDatamethod uses a good approach to detect SVG content by checking for both standard SVG tags and XML declarations, with proper bounds checking.
232-232:❓ Verification inconclusive
Changed property modifier from instance to static.
The
PlacesPathproperty has been changed from instance to static. This is appropriate since the method doesn't use any instance state, but ensure that any code that previously accessed this property as an instance member is updated accordingly.
🏁 Script executed:
#!/bin/bash # Check for any references to PlacesPath as an instance member rg -l "\.PlacesPath" --type cs | rg -v "FirefoxBookmarkLoader\.cs"Length of output: 68
ACTION: Confirm Static Access for
PlacesPathThe change to make
PlacesPathstatic is appropriate since it doesn’t depend on instance state. Our initial search for instance-specific references (e.g., usingthis.PlacesPathor instantiatingFirefoxBookmarkLoaderto accessPlacesPath) produced no results outside the defining file. However, because the verification output was minimal, please manually verify that all calls toPlacesPathnow reference it statically (using the class name) rather than via an instance.
- File to verify:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(Line 232)- Action required: Check that any external callers refer to
PlacesPathin a static context.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (2)
383-426: 🛠️ Refactor suggestionAdd bitmap.Freeze() call to SVG loading implementation.
The SVG loading method is missing the
Freeze()call on the bitmap before returning it, which is inconsistent with other image handling in the codebase. Freezing the bitmap is important for thread safety.bitmap.Render(drawingVisual); + bitmap.Freeze(); // Freeze to make it thread-safe, consistent with other image handling return bitmap;
383-386: 🛠️ Refactor suggestionAdd null check for path parameter.
The SVG loading method should validate that the path parameter is not null or empty before attempting to use it.
private static ImageSource LoadSvgImage(string path, bool loadFullImage = false) { + if (string.IsNullOrEmpty(path)) + { + throw new ArgumentNullException(nameof(path), "SVG file path cannot be null or empty"); + } // Set up drawing settings var desiredHeight = loadFullImage ? FullImageSize : SmallIconSize;
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (1)
383-397: Improve path handling in SVG loading method.The SVG loading method creates a Uri from the path, which might cause issues with some file paths that contain special characters. Consider using the overload of
Readthat accepts a file path directly.// Load and render the SVG var converter = new FileSvgReader(drawingSettings); - var drawing = converter.Read(new Uri(path)); + var drawing = converter.Read(path);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LogException(272-272)Flow.Launcher/PublicAPIInstance.cs (1)
LogException(217-218)Flow.Launcher/Msg.xaml.cs (1)
System(44-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs (4)
13-14: Import and constant additions look good!The new imports for SVG handling and the new constants for image sizes are well-structured and appropriately named.
Also applies to: 37-37, 40-40
242-247: Good improvement to exception handling!The updated exception handling uses a specific exception type and provides better error information in the log message.
259-272: Good SVG handling implementation with proper error logging.The SVG handling section is well-structured with appropriate error handling and detailed logging for failed operations.
346-346: Appropriate use of constants for image resizing.The method return type change to
ImageSourceis appropriate for supporting SVG images, and the use of theFullImageSizeconstant instead of hardcoded values improves maintainability.Also applies to: 355-372
Jack251970
changed the title
There was a problem hiding this comment.
Copilot reviewed 30 out of 34 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj: Language not supported
- Flow.Launcher.Infrastructure/NativeMethods.txt: Language not supported
- Flow.Launcher.Plugin/NativeMethods.txt: Language not supported
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj: Language not supported
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 30 out of 34 changed files in this pull request and generated 1 comment.
Files not reviewed (4)
- Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj: Language not supported
- Flow.Launcher.Infrastructure/NativeMethods.txt: Language not supported
- Flow.Launcher.Plugin/NativeMethods.txt: Language not supported
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj: Language not supported
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 30 out of 34 changed files in this pull request and generated no comments.
Files not reviewed (4)
- Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj: Language not supported
- Flow.Launcher.Infrastructure/NativeMethods.txt: Language not supported
- Flow.Launcher.Plugin/NativeMethods.txt: Language not supported
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj: Language not supported
Comments suppressed due to low confidence (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs:130
- Deleting the temporary favicon DB file in the finally block immediately after copying will remove the file before it is used to open the SQLite connection. Consider moving the deletion to after the connection has been processed to avoid file not found errors.
finally { File.Delete(tempDbPath); }
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:234
- Changing the 'PlacesPath' property to static may affect support for multiple Firefox profiles by sharing the same value across instances. Please verify if this change is intentional.
private static string PlacesPath
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (3)
44-87: Consider using try-finally for temporary file cleanup.The error handling and temporary file approach is good for avoiding lock issues with the original database. However, if an exception occurs between creating and deleting the temporary file, it could leave orphaned files in the cache directory.
- var tempDbPath = Path.Combine(_faviconCacheDir, $"tempplaces_{Guid.NewGuid()}.sqlite"); - - try + var tempDbPath = Path.Combine(_faviconCacheDir, $"tempplaces_{Guid.NewGuid()}.sqlite"); + try + { + // Try to register file monitoring + try + { + Main.RegisterBookmarkFile(placesPath); + } + catch (Exception ex) + { + Main._context.API.LogException(ClassName, $"Failed to register Firefox bookmark file monitoring: {placesPath}", ex); + } + + // Use a copy to avoid lock issues with the original file + File.Copy(placesPath, tempDbPath, true); + + // Connect to database and execute query + string dbPath = string.Format(DbPathFormat, tempDbPath); + using var dbConnection = new SqliteConnection(dbPath); + dbConnection.Open(); + var reader = new SqliteCommand(QueryAllBookmarks, dbConnection).ExecuteReader(); + + // Create bookmark list + bookmarks = reader + .Select( + x => new Bookmark( + x["title"] is DBNull ? string.Empty : x["title"].ToString(), + x["url"].ToString(), + "Firefox" + ) + ) + .ToList(); + + // Path to favicon database + var faviconDbPath = Path.Combine(Path.GetDirectoryName(placesPath), "favicons.sqlite"); + if (File.Exists(faviconDbPath)) + { + LoadFaviconsFromDb(faviconDbPath, bookmarks); + } + // https://github.com/dotnet/efcore/issues/26580 + SqliteConnection.ClearPool(dbConnection); + dbConnection.Close(); + } + catch (Exception ex) + { + Main._context.API.LogException(ClassName, $"Failed to load Firefox bookmarks: {placesPath}", ex); + } + finally + { + // Delete temporary file + try + { + if (File.Exists(tempDbPath)) + { + File.Delete(tempDbPath); + } + } + catch (Exception ex) + { + Main._context.API.LogException(ClassName, $"Failed to delete temporary favicon DB: {tempDbPath}", ex); + } + }
106-197: Consider enhancing the favicon loading process.The
LoadFaviconsFromDbmethod is comprehensive but could benefit from a few improvements:
- Use a try-finally block for temporary file cleanup
- Enhance the domain extraction logic to handle edge cases like IP addresses
- Consider caching results to avoid redundant database queries for the same domain
private void LoadFaviconsFromDb(string faviconDbPath, List<Bookmark> bookmarks) { var tempDbPath = Path.Combine(_faviconCacheDir, $"tempfavicons_{Guid.NewGuid()}.sqlite"); + var processedDomains = new Dictionary<string, string>(); try { // Use a copy to avoid lock issues with the original file File.Copy(faviconDbPath, tempDbPath, true); var defaultIconPath = Path.Combine( Path.GetDirectoryName(typeof(FirefoxBookmarkLoaderBase).Assembly.Location), "bookmark.png"); string dbPath = string.Format(DbPathFormat, tempDbPath); using var connection = new SqliteConnection(dbPath); connection.Open(); // Get favicons based on bookmark URLs foreach (var bookmark in bookmarks) { try { if (string.IsNullOrEmpty(bookmark.Url)) continue; // Extract domain from URL if (!Uri.TryCreate(bookmark.Url, UriKind.Absolute, out Uri uri)) continue; var domain = uri.Host; + + // Skip if we've already processed this domain + if (processedDomains.TryGetValue(domain, out string cachedPath)) + { + bookmark.FaviconPath = cachedPath; + continue; + } // Query for latest Firefox version favicon structure using var cmd = connection.CreateCommand(); cmd.CommandText = @" SELECT i.data FROM moz_icons i JOIN moz_icons_to_pages ip ON i.id = ip.icon_id JOIN moz_pages_w_icons p ON ip.page_id = p.id WHERE p.page_url LIKE @url AND i.data IS NOT NULL ORDER BY i.width DESC -- Select largest icon available LIMIT 1"; cmd.Parameters.AddWithValue("@url", $"%{domain}%"); using var reader = cmd.ExecuteReader(); if (!reader.Read() || reader.IsDBNull(0)) continue; var imageData = (byte[])reader["data"]; if (imageData is not { Length: > 0 }) continue; string faviconPath; if (IsSvgData(imageData)) { faviconPath = Path.Combine(_faviconCacheDir, $"firefox_{domain}.svg"); } else { faviconPath = Path.Combine(_faviconCacheDir, $"firefox_{domain}.png"); } SaveBitmapData(imageData, faviconPath); bookmark.FaviconPath = faviconPath; + processedDomains[domain] = faviconPath; } catch (Exception ex) { Main._context.API.LogException(ClassName, $"Failed to extract Firefox favicon: {bookmark.Url}", ex); } } // https://github.com/dotnet/efcore/issues/26580 SqliteConnection.ClearPool(connection); connection.Close(); } catch (Exception ex) { Main._context.API.LogException(ClassName, $"Failed to load Firefox favicon DB: {faviconDbPath}", ex); } + finally + { + // Delete temporary file + try + { + if (File.Exists(tempDbPath)) + { + File.Delete(tempDbPath); + } + } + catch (Exception ex) + { + Main._context.API.LogException(ClassName, $"Failed to delete temporary favicon DB: {tempDbPath}", ex); + } + } - - // Delete temporary file - try - { - File.Delete(tempDbPath); - } - catch (Exception ex) - { - Main._context.API.LogException(ClassName, $"Failed to delete temporary favicon DB: {tempDbPath}", ex); - } }
211-218: Consider enhancing SVG detection for edge cases.The current SVG detection logic is simple and effective for most cases, but it may not handle all SVG formats, especially those with unusual headers or encodings.
private static bool IsSvgData(byte[] data) { if (data.Length < 5) return false; string start = System.Text.Encoding.ASCII.GetString(data, 0, Math.Min(100, data.Length)); - return start.Contains("<svg") || - (start.StartsWith("<?xml") && start.Contains("<svg")); + // More comprehensive SVG detection + return start.Contains("<svg", StringComparison.OrdinalIgnoreCase) || + (start.StartsWith("<?xml", StringComparison.OrdinalIgnoreCase) && + start.Contains("<svg", StringComparison.OrdinalIgnoreCase)) || + start.Contains("image/svg+xml", StringComparison.OrdinalIgnoreCase); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (4)
Main(16-255)List(56-116)List(205-235)RegisterBookmarkFile(142-175)Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (3)
List(21-21)List(23-62)List(64-76)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Commands/BookmarkLoader.cs (1)
List(19-56)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (4)
12-19: Well-structured class initialization with proper logging identifier.The addition of the static
ClassNamefield and the constructor with favicon cache directory initialization follows good practices. This approach provides consistent logging identification and properly initializes the cache directory from a central location.
37-43: Improved start of the method with better variable management.Initializing the bookmarks list at the beginning and using it as the return value when the path check fails is a good practice. It makes the code more readable and maintainable by having a single return point.
199-209: Simple and effective bitmap saving method.This utility method is well-structured with proper error handling. It follows the single responsibility principle by focusing only on saving image data to a file.
234-234: Good change to static property.Making
PlacesPathstatic is appropriate since it doesn't access instance state and returns the same value regardless of the instance. This change also aligns with its usage pattern.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 30 out of 34 changed files in this pull request and generated no comments.
Files not reviewed (4)
- Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj: Language not supported
- Flow.Launcher.Infrastructure/NativeMethods.txt: Language not supported
- Flow.Launcher.Plugin/NativeMethods.txt: Language not supported
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj: Language not supported
Comments suppressed due to low confidence (2)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs:242
- Ensure that 'API' is defined and accessible in the ImageLoader class. If API is intended to reference the IPublicAPI instance, consider injecting it or using the appropriate static accessor to avoid compilation issues.
API.LogException(ClassName, "Failed to load image file from path " + path + ": " + ex.Message, ex);
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:234
- Changing the PlacesPath property to static may cause concurrency issues when processing multiple Firefox profiles. Consider reverting it to an instance property if multiple instances or profiles are expected.
private static string PlacesPath
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
What's the PR
IPublicAPIDocuments.Close #3132.
TODO
Test Cases
Known issue