Code cleanup: Flow.Launcher.Plugin.BrowserBookmark #2652

Yusyuriv · 2024-04-16T07:03:14Z

Mostly just moving from block-scoped namespaces to file-scoped, and renaming private properties to start with _ for consistency.

There are a few other minor changes that I will address in comments.

Since GitHub's diff view doesn't work well for migrating from block-scoped to file-scoped namespaces, and as it turns out, Rider also doesn't display it well when looking directly at the pull request, I suggest pulling this branch in Rider locally and looking at the diff there as it seems to work correctly when the diff is local.

Yusyuriv · 2024-04-16T07:05:44Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs

-        private BrowserType browserType = BrowserType.Chromium;
+    private string _name;
+    private string _dataDirectoryPath;
+    private BrowserType _browserType = BrowserType.Chromium;


Add _ to private property name to be consistent with other private property names

I suppose you want to say private field (they are different in c#).

Yeah, sorry, I meant field, not property.

Yusyuriv · 2024-04-16T07:07:21Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs

-                OnPropertyChanged(nameof(DataDirectoryPath));
-            }
+            _name = value;
+            OnPropertyChanged();


BaseModel class, which this class extends, has the [CallerMemberName] attribute on this argument, so there's no need to pass it manually.

Yusyuriv · 2024-04-16T07:08:00Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml

@@ -63,7 +63,6 @@
            <StackPanel Margin="26,0,26,0">
                <StackPanel Margin="0,0,0,12">
                    <TextBlock
-                        Grid.Column="0"


Not a child of a Grid, this attribute doesn't do anything.

Yusyuriv · 2024-04-16T07:11:02Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs

+    private void Others_Click(object sender, RoutedEventArgs e)
+    {
+        CustomBrowsersList.Visibility = CustomBrowsersList.Visibility switch
        {
-            EditSelectedCustomBrowser();
-        }
+            Visibility.Collapsed => Visibility.Visible,
+            _ => Visibility.Collapsed
+        };
+    }


In my opinion this is a little easier to read than the original if/else. What do you think?

Yusyuriv · 2024-04-16T07:30:25Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

-        private const string queryAllBookmarks = @"SELECT moz_places.url, moz_bookmarks.title
-              FROM moz_places
-              INNER JOIN moz_bookmarks ON (
+    private const string QueryAllBookmarks = """


Rename private const to start with a capital letter. Same with DbPathFormat.

I feel like this add newline that does not exist before? have you tested whether it still worked?

This does add a linebreak at the and and also reduce indenting. Luckily, SQL is not a language with significant whitespace, so I don't think it matters, and I think a raw string literal (""") visually conveys the idea that the string is multiline much better than a verbatim string (@"). I did test if it works: I added this text when reading from the database:

Yusyuriv · 2024-04-16T07:31:45Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

+        return reader
+            .Select(
+                x => new Bookmark(
+                    x["title"] is DBNull ? string.Empty : x["title"].ToString(),
+                    x["url"].ToString()
+                )
+            )
+            .ToList();


I think it's much more readable when every method call in the chain is on its own line. I'd like to hear your opinion.

Yeah I think it's fine. Though for me, I majorly like putting the first .Select on the same line...though I don't know how others will think about this. @Flow-Launcher/team

Yusyuriv · 2024-04-16T07:33:34Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

+            // get firefox default profile directory from profiles.ini
+            using var sReader = new StreamReader(profileIni);
+            var ini = sReader.ReadToEnd();


Replaced using block with the using declaration. Since there's no block anymore, it's no longer necessary to have ini declared outside.

Yusyuriv · 2024-04-16T07:34:05Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

+                StartWithLastProfile=1
+                Version=2
+            */
+            var lines = ini.Split("\r\n").ToList();


This does exactly the same thing as before, but with less code.

Yusyuriv · 2024-04-16T07:35:23Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

+            */
+            var lines = ini.Split("\r\n").ToList();
+
+            var defaultProfileFolderNameRaw = lines.FirstOrDefault(x => x.Contains("Default=") && x != "Default=1") ?? string.Empty;


Where call is redundant because FirstOrDefault also can accept a function for filtering.

Yusyuriv · 2024-04-16T07:40:40Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs

+            return _cachedBookmarks
+                .Select(
+                    c => new Result
                    {
-                        context.API.OpenUrl(c.Url);
+                        Title = c.Name,
+                        SubTitle = c.Url,
+                        IcoPath = @"Images\bookmark.png",
+                        Score = BookmarkLoader.MatchProgram(c, param).Score,
+                        Action = _ =>
+                        {
+                            _context.API.OpenUrl(c.Url);

-                        return true;
-                    },
-                    ContextData = new BookmarkAttributes
-                    {
-                        Url = c.Url
+                            return true;
+                        },
+                        ContextData = new BookmarkAttributes { Url = c.Url }
                    }
-                }).Where(r => r.Score > 0);
-                return returnList.ToList();
-            }
-            else
-            {
-                return cachedBookmarks.Select(c => new Result()
-                {
-                    Title = c.Name,
-                    SubTitle = c.Url,
-                    IcoPath = @"Images\bookmark.png",
-                    Score = 5,
-                    Action = _ =>
-                    {
-                        context.API.OpenUrl(c.Url);
-                        return true;
-                    },
-                    ContextData = new BookmarkAttributes
+                )
+                .Where(r => r.Score > 0)
+                .ToList();


Same as in FirefoxBookmarkReader.cs: I think it's much more readable when every chained method call is in a separate line, and I'd like to hear your opinion on this. Also, the variable is unnecessary here since it immediately returns, so I just continued the chain of method calls.

taooceros · 2024-04-17T00:12:59Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

-        private const string queryAllBookmarks = @"SELECT moz_places.url, moz_bookmarks.title
-              FROM moz_places
-              INNER JOIN moz_bookmarks ON (
+    private const string QueryAllBookmarks = """


I feel like this add newline that does not exist before? have you tested whether it still worked?

taooceros · 2024-04-17T00:15:40Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs

+        return reader
+            .Select(
+                x => new Bookmark(
+                    x["title"] is DBNull ? string.Empty : x["title"].ToString(),
+                    x["url"].ToString()
+                )
+            )
+            .ToList();


Yeah I think it's fine. Though for me, I majorly like putting the first .Select on the same line...though I don't know how others will think about this. @Flow-Launcher/team

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs

taooceros · 2024-04-17T00:20:15Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs

-        private BrowserType browserType = BrowserType.Chromium;
+    private string _name;
+    private string _dataDirectoryPath;
+    private BrowserType _browserType = BrowserType.Chromium;


I suppose you want to say private field (they are different in c#).

VictoriousRaptor · 2024-04-30T14:53:31Z

LGTM. Tested?

Yusyuriv · 2024-04-30T15:18:33Z

I did launch it, and it does work as expected. Here are bookmarks from my Firefox after the changes

Here's a bookmark from Edge

It was probably unintentional, but in your last commit you added an import in two files and modified a project file. @VictoriousRaptor

VictoriousRaptor · 2024-05-01T03:12:06Z

It was probably unintentional, but in your last commit you added an import in two files and modified a project file.

Fixed. Sorry about that.

Code cleanup: Flow.Launcher.Plugin.BrowserBookmark

f6d25aa

Yusyuriv requested a review from taooceros April 16, 2024 07:03

Yusyuriv self-assigned this Apr 16, 2024

This comment has been minimized.

Sign in to view

Yusyuriv commented Apr 16, 2024

View reviewed changes

Fix Firefox bookmark SQL query formatting

6373778

This comment has been minimized.

Sign in to view

Yusyuriv commented Apr 16, 2024

View reviewed changes

Fix a typo in variable name in FirefoxBookmarkLoader.cs

7393b27

This comment has been minimized.

Sign in to view

Yusyuriv marked this pull request as ready for review April 16, 2024 08:04

taooceros reviewed Apr 17, 2024

View reviewed changes

Add a hyphen in a word inside a comment in BrowserBookmark/Main.cs

148afc4

This comment has been minimized.

Sign in to view

Update expect.txt

420446d

This comment has been minimized.

Sign in to view

Try to add terms in allow.txt

507a126

VictoriousRaptor force-pushed the code-cleanup/Flow.Launcher.Plugin.BrowserBookmark branch from 263f23e to 507a126 Compare May 1, 2024 03:11

VictoriousRaptor enabled auto-merge May 1, 2024 03:24

VictoriousRaptor approved these changes May 1, 2024

View reviewed changes

VictoriousRaptor merged commit 3554136 into dev May 1, 2024
4 checks passed

VictoriousRaptor deleted the code-cleanup/Flow.Launcher.Plugin.BrowserBookmark branch May 1, 2024 03:42

VictoriousRaptor added the Code Refactor label May 1, 2024

VictoriousRaptor added this to the 1.19.0 milestone May 1, 2024

jjw24 added the enhancement New feature or request label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code cleanup: Flow.Launcher.Plugin.BrowserBookmark #2652

Code cleanup: Flow.Launcher.Plugin.BrowserBookmark #2652

Yusyuriv commented Apr 16, 2024 •

edited

This comment has been minimized.

Yusyuriv Apr 16, 2024

taooceros Apr 17, 2024

Yusyuriv Apr 17, 2024

Yusyuriv Apr 16, 2024

Yusyuriv Apr 16, 2024

Yusyuriv Apr 16, 2024

This comment has been minimized.

Yusyuriv Apr 16, 2024

taooceros Apr 17, 2024

Yusyuriv Apr 17, 2024

Yusyuriv Apr 16, 2024

taooceros Apr 17, 2024

Yusyuriv Apr 16, 2024

Yusyuriv Apr 16, 2024

Yusyuriv Apr 16, 2024

Yusyuriv Apr 16, 2024 •

edited

This comment has been minimized.

taooceros Apr 17, 2024

taooceros Apr 17, 2024

taooceros Apr 17, 2024

This comment has been minimized.

VictoriousRaptor commented Apr 30, 2024

This comment has been minimized.

Yusyuriv commented Apr 30, 2024

VictoriousRaptor commented May 1, 2024

Code cleanup: Flow.Launcher.Plugin.BrowserBookmark #2652

Code cleanup: Flow.Launcher.Plugin.BrowserBookmark #2652

Conversation

Yusyuriv commented Apr 16, 2024 • edited

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yusyuriv Apr 16, 2024 • edited

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

VictoriousRaptor commented Apr 30, 2024

This comment has been minimized.

Yusyuriv commented Apr 30, 2024

VictoriousRaptor commented May 1, 2024

Yusyuriv commented Apr 16, 2024 •

edited

Yusyuriv Apr 16, 2024 •

edited