Add Flow Launcher Theme Selector to Sys plugin #2448
Conversation
|
Great idea. I think this plugin would be best served by publishing to the Store. We can have a look at exposing the needed methods via the API. |
I feel like we can allow this plugin to be built in. Thememanager is pretty internal. |
|
Well yes and no, it's directly referencing the core project, ideal design is to have even the default plugins as self contained. |
Some of our internal stuff uses the plugin interface for receiving query, which I think is a pretty good idea. I am not sure whether exposing everything to the interface is a very good idea. PublicAPI sometimes can be the curse as we may want to maintain backward compatibility. |
|
Well I think at least this functionality is a good one to enable plugins to use. Remember the more plugins we pack the more we need to maintain and the bigger the size eventually get. More ideal to encourage plugin development. |
well a plugin only takes 400k size...which should be pretty neglectable. and this feature seems to be good to built in. |
|
@Odotocodot isn't you also need to add the plugin folder to the solution? |
This comment has been minimized.
This comment has been minimized.
What about putting this functionality in the Sys plugin since it has flow related functionalities like open log file and reload data etc. |
|
Sorry ive been busy with uni!
So move to be inside the sys plugin?
Im not sure its needed? or im confused... its be a while |
I would say ideally exposing the methods via the plugin API would be the best so people can build plugin around it, but this change can be complicated, so putting it in the Sys plugin is the next best option. |
|
I've moved the theme selector plugin into the sys plugin now, though I have only done the english translation. |
📝 WalkthroughWalkthroughWalkthroughThe updates enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant ThemeSelector
User->>Main: Query for themes
Main->>ThemeSelector: Delegate theme query
ThemeSelector-->>Main: Returns available themes
Main-->>User: Displays themes
User->>Main: Select a theme
Main->>ThemeSelector: Apply selected theme
ThemeSelector-->>Main: Theme applied
Main-->>User: Confirmation of applied theme
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 2
Outside diff range and nitpick comments (3)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (2)
11-11: Consider renaming the constantKeywordto something more specific to avoid potential naming conflicts and increase clarity.
56-83: The methodsCreateThemeResultare well implemented and provide a clear way to create result objects. Consider adding XML documentation to these methods to improve code maintainability.Plugins/Flow.Launcher.Plugin.Sys/Languages/en.xaml (1)
30-30: Ensure the command description aligns with the functionality of setting themes. It might be helpful to specify that this command allows for dynamic theme switching.
|
@jjw24 is there anything else left on this pr? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (2)
22-44: 🛠️ Refactor suggestionAdd parameter validation and optimize string operations.
The Query method needs parameter validation and can be optimized:
public List<Result> Query(Query query) { + if (query == null) throw new ArgumentNullException(nameof(query)); + if (string.IsNullOrEmpty(query.Search)) return new List<Result>(); + if (query.IsReQuery) { LoadThemes(); } int keywordIndex = query.Search.IndexOf(Keyword, StringComparison.Ordinal); + if (keywordIndex == -1) return new List<Result>(); string search = query.Search[(keywordIndex + Keyword.Length + 1)..];
54-55: 🛠️ Refactor suggestionAdd exception handling for theme loading.
The LoadThemes method should handle potential IO exceptions that might occur during theme loading.
private void LoadThemes() - => themes = ThemeManager.Instance.LoadAvailableThemes().Select(Path.GetFileNameWithoutExtension); + { + try + { + themes = ThemeManager.Instance.LoadAvailableThemes().Select(Path.GetFileNameWithoutExtension); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + context.API.LogException("Failed to load themes", ex); + themes = Enumerable.Empty<string>(); + } + }
🧹 Nitpick comments (3)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (3)
9-14: Add XML documentation for public members.Please add XML documentation for the public class and constant to improve API documentation.
+ /// <summary> + /// Manages theme selection functionality for the Flow Launcher Sys plugin. + /// </summary> public class ThemeSelector : IDisposable { + /// <summary> + /// The keyword used to trigger theme selection functionality. + /// </summary> public const string Keyword = "fltheme";
11-11: Consider using a more intuitive keyword.The keyword "fltheme" is abbreviated and might not be immediately clear to users. Consider using "flowtheme" as mentioned in the PR description or "theme" for simplicity.
59-84: Extract magic numbers and hardcoded values into constants.Consider extracting magic numbers and hardcoded values into named constants for better maintainability.
public class ThemeSelector : IDisposable { public const string Keyword = "fltheme"; + private const int CURRENT_THEME_SCORE_BOOST = 2000; + private const string THEME_ICON_FONT = "/Resources/#Segoe Fluent Icons"; + private const string THEME_ICON_CODE = "\ue790"; + private const string CURRENT_THEME_INDICATOR = "★"; // ... existing code ... private static Result CreateThemeResult(string theme, int score, IList<int> highlightData) { string title; if (theme == ThemeManager.Instance.Settings.Theme) { - title = $"{theme} ★"; - score = 2000; + title = $"{theme} {CURRENT_THEME_INDICATOR}"; + score = CURRENT_THEME_SCORE_BOOST; } // ... existing code ... - Glyph = new GlyphInfo("/Resources/#Segoe Fluent Icons", "\ue790"), + Glyph = new GlyphInfo(THEME_ICON_FONT, THEME_ICON_CODE),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
This comment has been minimized.
This comment has been minimized.
jjw24
changed the title
This comment has been minimized.
This comment has been minimized.
Getting there but it's not ready yet. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (1)
62-62: Add null check for TitleHighlightData.Consider adding a null check for TitleHighlightData to prevent potential issues if highlightData is null.
- TitleHighlightData = highlightData, + TitleHighlightData = highlightData != null ? highlightData : new List<int>(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Sys/Main.cs(5 hunks)Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.Sys/Main.cs
🧰 Additional context used
🧠 Learnings (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (1)
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-03-12T13:10:00.156Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
🔇 Additional comments (6)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (6)
8-11: LGTM: Class declaration and keyword.The class is well-structured with a clear constant for the keyword. The "fltheme" keyword matches the discussion in the PR objectives.
15-19: Constructor implementation looks good.The constructor follows the project conventions by accepting the PluginInitContext and using dependency injection to get the theme service.
21-39: Query method handles theme search effectively.The method loads available themes, properly handles empty searches, and implements fuzzy search for non-empty queries. Using
query.SecondToEndSearchaddresses the previous issue mentioned in comments when nothing is entered after the keyword.
41-72: Theme result creation logic is well-implemented.The implementation correctly prioritizes the current theme with a star indicator and higher score, and provides all necessary properties for the results.
8-74: Consider implementing IDisposable pattern.The class currently doesn't implement the IDisposable interface, but past review comments indicate this was suggested. If the class needs to handle unmanaged resources or event unsubscription, consider implementing the complete IDisposable pattern.
- public class ThemeSelector + public class ThemeSelector : IDisposable { public const string Keyword = "fltheme"; private readonly Theme _theme; private readonly PluginInitContext _context; + private bool _disposed; public ThemeSelector(PluginInitContext context) { _context = context; _theme = Ioc.Default.GetRequiredService<Theme>(); } // ... existing methods ... + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + // Dispose managed resources if needed + } + // Free unmanaged resources + + _disposed = true; + } + } + + ~ThemeSelector() + { + Dispose(false); + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } }Don't forget to add
using System;to the imports if implementing IDisposable.
23-23:✅ Verification successful
Consider adding exception handling when loading themes.
The method directly calls
_theme.LoadAvailableThemes()without any exception handling. This could cause unhandled exceptions if there are issues accessing theme files.- var themes = _theme.LoadAvailableThemes().Select(x => x.FileNameWithoutExtension); + var themes = Enumerable.Empty<string>(); + try + { + themes = _theme.LoadAvailableThemes().Select(x => x.FileNameWithoutExtension); + } + catch (System.Exception ex) + { + _context.API.LogException("Error loading themes", ex); + return new List<Result> + { + new Result + { + Title = "Error loading themes", + SubTitle = ex.Message, + IcoPath = "Images\\theme_selector.png", + Glyph = new GlyphInfo("/Resources/#Segoe Fluent Icons", "\ue790"), + } + }; + }Let's verify if the
LoadAvailableThemesmethod already has exception handling:
🏁 Script executed:
#!/bin/bash # Check if LoadAvailableThemes has exception handling rg -A 10 "LoadAvailableThemes" Flow.Launcher.Core/Resource/Theme.csLength of output: 567
Exception Handling for Theme Loading
After verifying that the
LoadAvailableThemesmethod inFlow.Launcher.Core/Resource/Theme.csdoes not internally handle exceptions, it's clear that wrapping its call in a try-catch block inThemeSelector.csis necessary. This change will ensure that any errors during theme file access are caught, logged (using_context.API.LogException), and result in an appropriate fallback response without crashing the application.
- File:
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs(Lines: 23)- Changes:
- Wrap the call to
_theme.LoadAvailableThemes().Select(...)in a try-catch block.- Log any caught exceptions.
- Return a fallback error
Resultlist when an exception occurs.The provided diff snippet correctly implements these improvements.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (5)
21-33: Consider handling a potential null assignment to SelectedTheme.
If a null value were somehow passed toSelectedTheme, referencingvalue.FileNameWithoutExtensionat line 28 would cause aNullReferenceException. Although this is likely prevented by design, you could add a guard condition or argument check to ensure robustness.
35-35: Evaluate caching themes to enhance performance.
Accessing_theme.LoadAvailableThemes()each time theThemesproperty is read can repeatedly scan the themes from disk or memory. Caching the themes when the plugin is invoked (e.g. on visibility change or first query) might improve performance, especially if there are many themes.
69-84: Ensure theme loading strategy aligns with user expectations.
Loading all themes on every query helps ensure the list stays updated but can be more resource-intensive. If performance becomes a concern, consider loading themes only once on plugin activation or OnVisibilityChanged, then refreshing selectively if needed.
86-135: Replace magic scores with named constants for better readability.
Using 2000 for the selected theme and 1000 for others is clear enough, but constants (e.g.,const int SelectedThemeScore = 2000;) can improve maintainability by making the intent explicit.
63-67: Consider adding a dedicated test for ThemeSelector logic.
Although manual testing might suffice, automated tests can validate queries, theme switching, and highlight data. Testing can help ensure future changes do not introduce regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (1)
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-03-12T13:10:00.156Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
|
@Odotocodot Hi, I have changed theme selection part and here is the final result. Firstly, I use codes from Secondly, every time you change the theme, the window will not be closed but re-query the results because users may not feel good about the theme they changed so they possibly want to change theme for many times. Thirdly, I add light/dark theme tooltip into results as their subtitles. 2025-03-12.22-24-26.mp4This works fine for me and I have approved those changes. Please check if this works for you, and feel free to merge if you think it is ready. Lastly, thanks for your idea and your patience! |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (5)
38-38: Consider caching theme data to improve performance.The
Themesproperty directly calls_theme.LoadAvailableThemes()on each access, which could lead to performance issues if accessed frequently (especially during typing or multiple queries).- private List<Theme.ThemeData> Themes => _theme.LoadAvailableThemes(); + private List<Theme.ThemeData> _themes; + private List<Theme.ThemeData> Themes => _themes ??= _theme.LoadAvailableThemes();
86-88: Remove redundant boolean comparison.The explicit comparison with
trueis unnecessary and can be simplified.- if (theme.IsDark == true) + if (theme.IsDark)
91-93: Remove redundant boolean comparison.Similar to the previous issue, the explicit comparison with
trueis unnecessary and can be simplified.- if (theme.HasBlur == true) + if (theme.HasBlur)
42-47: Class is missing a using directive for System namespace.Add the System namespace for proper access to types like String and ArgumentNullException.
+using System; using System.Collections.Generic; using System.Linq;
30-35: Add user notification for drop shadow effect changes.When automatically disabling the drop shadow effect due to blur being enabled, consider notifying the user about this change to improve user experience.
if (_theme.BlurEnabled && _settings.UseDropShadowEffect) { _theme.RemoveDropShadowEffectFromCurrentTheme(); _settings.UseDropShadowEffect = false; + _context.API.ShowMsg(_context.API.GetTranslation("fltheme_drop_shadow_disabled"), + _context.API.GetTranslation("fltheme_drop_shadow_disabled_due_to_blur")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (1)
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-03-12T13:10:00.156Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
⏰ 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.Sys/ThemeSelector.cs (4)
42-47: Consider implementing IDisposable for proper resource cleanup.Since the class subscribes to events in other files (as seen from past review comments about
VisibilityChanged), it should implement IDisposable to properly unsubscribe from these events.- public class ThemeSelector + public class ThemeSelector : IDisposable { // existing code... + + private bool _disposed; + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + // Unsubscribe from events + // Clean up managed resources + } + + // Clean up unmanaged resources + + _disposed = true; + } + } + + ~ThemeSelector() + { + Dispose(false); + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } }
49-64: Good implementation of the Query method with proper handling of empty searches.The Query method efficiently handles both empty searches and specific search terms. It uses fuzzy search for filtering themes and properly orders the results.
98-113: Well-structured Result object with appropriate score prioritization.The Result creation logic is well-designed, with thoughtful handling of the current theme (marking it with a star and giving it higher priority) and inclusion of descriptive information. The action to change the theme and requery is also appropriately implemented.
103-104:✅ Verification successful
Verify that the theme_selector.png image exists.
Make sure the referenced image file exists in the specified path, as missing resources could cause runtime issues.
🏁 Script executed:
#!/bin/bash # Check if the theme_selector.png exists in the Images directory ls -la Plugins/Flow.Launcher.Plugin.Sys/Images/theme_selector.png 2>/dev/null || echo "Image file not found"Length of output: 175
Asset Verification Passed: theme_selector.png Confirmed
The referenced image file exists in the expected path (
Plugins/Flow.Launcher.Plugin.Sys/Images/theme_selector.png). No further action is necessary.
|
@Jack251970 Nice! I fixed a null exception and removed some extra code (In If eveything is in order I am happy to merge! |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Using If the flagged items are 🤯 false positivesIf items relate to a ...
|
*happy for a merge! |
4 participants
A plugin to quickly change the current Flow Launcher theme.
If someone has a better suggestion for the default action keyword rather than
flowtheme, would love to hear it. Was a bit apprehensive in using theme 🤷♂️ .