Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Show favicons as Bookmarks icons #3079

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: Show favicons as Bookmarks icons #3079

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public List<Result> Query(Query query)
{
Title = c.Name,
SubTitle = c.Url,
IcoPath = @"Images\bookmark.png",
IcoPath = $"https://www.google.com/s2/favicons?domain={c.Url}&sz=64",
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider adding error handling and caching for favicon URLs

The direct use of bookmark URLs in the favicon API call has several potential issues:

  1. No fallback mechanism when the favicon API fails
  2. Potential security concerns with unencoded URLs
  3. Performance impact from making external API calls for each bookmark

Consider implementing these improvements:

-IcoPath = $"https://www.google.com/s2/favicons?domain={c.Url}&sz=64",
+IcoPath = GetFaviconPath(c.Url),

+private string GetFaviconPath(string url)
+{
+    try
+    {
+        var uri = new Uri(url);
+        var encodedDomain = Uri.EscapeDataString(uri.Host);
+        return $"https://www.google.com/s2/favicons?domain={encodedDomain}&sz=64";
+    }
+    catch (Exception ex)
+    {
+        Log.Exception("Main", "Failed to generate favicon URL", ex);
+        return @"Images\bookmark.png"; // Fallback to default icon
+    }
+}

Consider implementing a caching mechanism for favicons to:

  1. Reduce external API calls
  2. Handle offline scenarios
  3. Improve performance
    Would you like me to provide a caching implementation example?

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@z1nc0r3 I think this is worth addressing. Use the existing image as a fallback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I also think these calls to grab icons might impact performance, if there are a lot of bookmarks.

@coderabbitai provide a caching implementation example please.

@z1nc0r3 please test performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjw24, certainly! Here's an example of how you can implement caching for the favicons to improve performance:

First, add a dictionary to act as an in-memory cache:

private static Dictionary<string, string> faviconCache = new Dictionary<string, string>();

Next, update the GetFaviconPath method to utilize the cache:

private string GetFaviconPath(string url)
{
    try
    {
        var uri = new Uri(url);
        var domain = uri.Host;

        if (faviconCache.TryGetValue(domain, out var cachedFaviconUrl))
        {
            // Return the cached favicon URL if it exists
            return cachedFaviconUrl;
        }
        else
        {
            var encodedDomain = Uri.EscapeDataString(domain);
            var faviconUrl = $"https://www.google.com/s2/favicons?domain={encodedDomain}&sz=64";
            // Store the favicon URL in the cache
            faviconCache[domain] = faviconUrl;
            return faviconUrl;
        }
    }
    catch (Exception ex)
    {
        Log.Exception("Main", "Failed to generate favicon URL", ex);
        // Fallback to default icon
        return @"Images\bookmark.png";
    }
}

Then, update the IcoPath assignment in your Result objects to use this method:

IcoPath = GetFaviconPath(c.Url),

This implementation:

  • Caches favicon URLs based on the domain to avoid redundant API calls.
  • Reduces external requests, which improves performance, especially with a large number of bookmarks.
  • Handles exceptions and provides a fallback to the default icon if needed.

Please remember to test the performance with your set of bookmarks to ensure the caching mechanism effectively improves load times.

Let me know if you have any questions or need further assistance!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure @jjw24. I'll try these changes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjw24 I think performance wise it's fine. This pr only put the url as the image path, which will be loaded async from flow side. Because of this, caching doesn't really makes sense (it will be cached as the imagecache in flow if we do it correctly before).

Score = BookmarkLoader.MatchProgram(c, param).Score,
Action = _ =>
{
Expand All @@ -90,7 +90,7 @@ public List<Result> Query(Query query)
{
Title = c.Name,
SubTitle = c.Url,
IcoPath = @"Images\bookmark.png",
IcoPath = $"https://www.google.com/s2/favicons?domain={c.Url}&sz=64",
Score = 5,
Action = _ =>
{
Expand Down
Loading