New API Function from Theme & Improve Theme Model #3420
Conversation
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis pull request refactors and centralizes theme management across the application. It removes the old string-based theme retrieval in favor of a new system that uses a dedicated Changes
Possibly related PRs
Suggested labels
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 (
|
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Flow.Launcher.Plugin/ThemeData.cs:11
- The comment for the property 'IsDark' (appearing below) incorrectly uses 'Theme file path'. Consider changing it to something like 'Indicates whether the theme is dark' for clarity.
/// Theme file name without extension
![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)
Flow.Launcher/PublicAPIInstance.cs (1)
360-368: Clean implementation of theme-related API methodsThe implementations of the theme-related API methods are concise and follow good delegation patterns:
GetAvailableThemes()andGetCurrentTheme()directly delegate to the Theme serviceSetCurrentTheme(ThemeData theme)appropriately calls theme change logic and refreshes the UI asynchronouslyThe use of the
_discard for the Task returned byRefreshFrameAsync()indicates that you're not awaiting the operation, which might be intentional, but could potentially lead to race conditions if multiple theme changes occur in quick succession.Consider using
awaitwithTask.Run(() => _theme.RefreshFrameAsync())to prevent potential UI thread blocking while ensuring the operation completes, or add a comment explaining why the result is discarded.Flow.Launcher.Plugin/ThemeData.cs (1)
1-39: Well-structured theme data model with proper encapsulationThe ThemeData class is a well-designed model with proper XML documentation and encapsulation through private init-only properties. The constructor parameters align with the property names, making the code more readable.
There's a minor issue with the documentation for the
IsDarkandHasBlurproperties - both have the same XML comment "Theme file path" which is incorrect. They should describe these boolean properties properly.Correct the XML documentation for the
IsDarkandHasBlurproperties to properly describe their purpose, e.g., "Indicates whether the theme is dark" and "Indicates whether the theme has blur effects".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Flow.Launcher.Core/Resource/Theme.cs(6 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher.Plugin/ThemeData.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
Flow.Launcher.Plugin/ThemeData.cs (3)
Flow.Launcher/PublicAPIInstance.cs (1)
ThemeData(362-362)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
ThemeData(358-358)Flow.Launcher.Core/Resource/Theme.cs (2)
ThemeData(331-363)ThemeData(383-387)
Flow.Launcher/PublicAPIInstance.cs (3)
Flow.Launcher.Core/Resource/Theme.cs (6)
Theme(23-902)Theme(51-78)List(389-402)ThemeData(331-363)ThemeData(383-387)ChangeTheme(404-455)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (4)
List(141-141)List(352-352)ThemeData(358-358)SetCurrentTheme(365-365)Flow.Launcher.Plugin/ThemeData.cs (2)
ThemeData(8-73)ThemeData(33-39)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (3)
Flow.Launcher/PublicAPIInstance.cs (4)
List(175-175)List(360-360)ThemeData(362-362)SetCurrentTheme(364-368)Flow.Launcher.Core/Resource/Theme.cs (3)
List(389-402)ThemeData(331-363)ThemeData(383-387)Flow.Launcher.Plugin/ThemeData.cs (2)
ThemeData(8-73)ThemeData(33-39)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (1)
Flow.Launcher.Core/Resource/Theme.cs (2)
ThemeData(331-363)ThemeData(383-387)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (20)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
347-365: API enhancement with theme management capabilitiesThe addition of these three theme-related methods to the IPublicAPI interface is a well-structured enhancement that centralizes theme management operations. The methods are properly documented with XML comments and provide a comprehensive set of operations for theme management: retrieving available themes, getting the current theme, and setting a new theme.
These methods will allow plugins to interact with the theme system in a type-safe manner using the ThemeData class, which is a good improvement over string-based theme handling.
Flow.Launcher/PublicAPIInstance.cs (1)
40-41: Good use of lazy initialization with dependency injectionThe implementation uses proper lazy initialization for the Theme property, ensuring the Theme service is only retrieved from the DI container when needed. This is a good practice for managing dependencies and improving performance.
Flow.Launcher.Plugin/ThemeData.cs (3)
41-51: Correct implementation of equality operatorsThe equality operators are properly implemented, with
==delegating to theEqualsmethod and!=defined as the logical negation of==, which is the correct pattern.
53-66: Proper implementation of equality and hash code methodsThe
EqualsandGetHashCodemethods are correctly implemented to consider only theFileNameWithoutExtensionandNameproperties for equality comparison. This follows best practices for implementing equality in C#.
68-72: Simple and effective ToString implementationThe
ToStringmethod is appropriately overridden to return theNameproperty, which is a reasonable string representation for a theme.Flow.Launcher.Core/Resource/Theme.cs (7)
125-126: Direct access to settings instead of method callThe code now directly accesses
_settings.Themeinstead of using a method call, which simplifies the code and reduces an extra function call. Good refactoring.
328-329: Simplified theme retrieval in GetCurrentResourceDictionarySimilar to the previous change, the method now directly accesses
_settings.Themewhich improves code clarity and performance by eliminating an unnecessary method call.
381-387: New GetCurrentTheme method returns a ThemeData objectThis method is part of the new theme management API, returning the current theme as a ThemeData object. It appropriately fallbacks to the first available theme if the current theme setting isn't found, providing good error resilience.
389-402: Renamed method with consistent return typeThe method was renamed from
LoadAvailableThemestoGetAvailableThemes, which better reflects its purpose as a retrieval method rather than a loading method. The implementation remains sound, filtering out base theme files and ordering themes by name for a better user experience.
407-408: Direct access to settings in ChangeTheme methodSimilar to previous changes, this directly accesses
_settings.Themeinstead of using a method call, maintaining consistency with other refactored parts of the code.
595-596: Direct access to settings in RefreshFrameAsyncAnother consistent change to directly access
_settings.Themein theSetBlurForWindowmethod call within theRefreshFrameAsyncmethod.
614-615: Direct access to settings in SetBlurForWindowAsyncConsistent with other changes, this directly passes
_settings.ThemetoSetBlurForWindowinstead of using a method call.Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (5)
19-21: Adoption of centralized API for theme managementThe code has been refactored to use the centralized API methods for retrieving theme information instead of maintaining a local state. This is a good improvement for consistency across the application.
25-25: Proper handling of theme selectionThe code now correctly passes both the theme and selected theme to the CreateThemeResult method, allowing for proper comparison and styling of the selected theme.
30-34: Improved search implementation with new APIThe fuzzy search implementation now works with the new theme data structure, maintaining the same functionality while leveraging the centralized API.
79-82: Simplified theme selection through APIThe action now uses the centralized API to set the current theme, which is more maintainable and consistent with the rest of the application.
37-39:❓ Verification inconclusive
Verify ThemeData equality comparison
The CreateThemeResult method signature has been updated to work with ThemeData objects. However, the comparison at line 43 (
theme == selectedTheme) relies on proper equality comparison.
🏁 Script executed:
#!/bin/bash # Check if ThemeData class overrides equality comparison rg -A 10 "class ThemeData" --type csharpLength of output: 689
Review Verification – ThemeData Equality Check
After reviewing
Flow.Launcher.Plugin/ThemeData.cs, it appears that theThemeDataclass does not override the default equality members (i.e., there’s no custom implementation ofEquals,GetHashCode, or overloading of the==operator). As a result, the expressiontheme == selectedThemeinThemeSelector.cs(line 43) performs a reference equality check.
- Action Required: Please verify that reference equality is the intended behavior. If you require a value-based comparison (for example, comparing based on
FileNameWithoutExtensionandName), updateThemeDatato implement the necessary equality overrides.Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (3)
31-32: Centralized theme data retrievalThe code now retrieves available themes through the centralized API rather than directly accessing the theme manager. This is a good improvement for maintainability and consistency.
40-42:✅ Verification successful
Verify UI refresh after theme change
The code now calls
App.API.SetCurrentTheme(value)but no longer explicitly calls_theme.RefreshFrameAsync(). Ensure the API method internally handles UI refresh.
🏁 Script executed:
#!/bin/bash # Check SetCurrentTheme implementation for UI refresh calls rg -A 15 "SetCurrentTheme.*ThemeData" --type csharpLength of output: 1339
Confirmation: The API now handles UI refresh internally
The implementation ofApp.API.SetCurrentTheme(value)inFlow.Launcher/PublicAPIInstance.cscalls_theme.RefreshFrameAsync(), which confirms that the UI refresh after a theme change is handled internally. No further explicit call is necessary in the settings page.
34-38:✅ Verification successful
Consider potential ThemeData comparison issue
The Find method uses
v => v == App.API.GetCurrentTheme()to find the current theme. If ThemeData is a reference type without proper equality overrides, this comparison might not work correctly.
🏁 Script executed:
#!/bin/bash # Check implementation of ThemeData equality rg -B 5 -A 15 "Equals.*override" --type csharp --glob "*ThemeData.cs" # Alternative check for operator overloading rg -B 5 -A 15 "operator ==" --type csharp --glob "*ThemeData.cs"Length of output: 1326
ThemeData equality comparison verified
After reviewing the implementation inFlow.Launcher.Plugin/ThemeData.cs, it’s confirmed that theoperator ==and the overriddenEqualsmethod are properly implemented. This makes the comparison inFlow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(lines 34-38) valid and safe.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 6 out of 6 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:41
- [nitpick] Consider using theme.Name instead of FileNameWithoutExtension for display consistency, as the UI later appends a star to theme.Name. Align both the displayed and identifier properties to avoid potential confusion.
var themeName = theme.FileNameWithoutExtension;
Flow.Launcher/Core/Resource/Theme.cs:125
- [nitpick] For consistent theme retrieval logic, consider calling GetCurrentTheme() instead of directly accessing _settings.Theme. This centralizes the current theme logic and reduces potential inconsistencies if the retrieval logic changes in the future.
var themeName = _settings.Theme;
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher/PublicAPIInstance.cs (1)
365-369: Consider using the property for consistency when refreshing the UI.While the implementation works correctly, consider using the
Themeproperty instead of directly accessing the_themefield when callingRefreshFrameAsync(). This would maintain consistency with how the other methods access the theme service and protect against potential null reference issues.public void SetCurrentTheme(ThemeData theme) { Theme.ChangeTheme(theme.FileNameWithoutExtension); - _ = _theme.RefreshFrameAsync(); + _ = Theme.RefreshFrameAsync(); }Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
360-366: Remove unnecessary returns tag in documentation.The XML documentation includes a
<returns></returns>tag, but since the method returns void, this tag is unnecessary and should be removed./// <summary> /// Set the current theme /// </summary> /// <param name="theme"></param> -/// <returns></returns> public void SetCurrentTheme(ThemeData theme);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
Flow.Launcher/PublicAPIInstance.cs (3)
Flow.Launcher.Core/Resource/Theme.cs (6)
Theme(23-902)Theme(51-78)List(389-402)ThemeData(331-363)ThemeData(383-387)ChangeTheme(404-455)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (4)
List(141-141)List(352-352)ThemeData(358-358)SetCurrentTheme(365-365)Flow.Launcher.Plugin/ThemeData.cs (2)
ThemeData(8-73)ThemeData(33-39)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
Flow.Launcher/PublicAPIInstance.cs (4)
List(176-176)List(361-361)ThemeData(363-363)SetCurrentTheme(365-369)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Flow.Launcher/PublicAPIInstance.cs (2)
41-42: Good implementation of lazy initialization with dependency injection.The
Themeproperty is well-implemented using the null-coalescing assignment operator (??=) to ensure the service is only requested from the IoC container when needed and then cached for future use.
361-364: LGTM! Clean implementation of theme retrieval methods.These methods provide a clean interface for accessing theme data by delegating to the underlying
Themeservice.Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
348-353: LGTM! New API method for retrieving available themes.This method enhances the public API by providing access to all available themes, which aligns with the PR objectives.
354-359: LGTM! New API method for retrieving the current theme.This method provides a clean way to access the current theme data, which is essential for theme management functionality.
|
🥷 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:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
Flow.Launcher.Plugin/ThemeData.cs
Outdated
| /// <summary> | ||
| /// Theme data model | ||
| /// </summary> | ||
| public class ThemeData |
There was a problem hiding this comment.
Everything else LGTM, could you move this to SharedModel directory. Any that is just a model of an object class should go into that directory.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs:38
- [nitpick] Consider comparing themes using their FileNameWithoutExtension (e.g. v.FileNameWithoutExtension == App.API.GetCurrentTheme().FileNameWithoutExtension) to avoid potential ambiguity from relying solely on the overridden equality operator.
get => _selectedTheme ??= Themes.Find(v => v == App.API.GetCurrentTheme());
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher.Plugin/SharedModels/ThemeData.cs (2)
57-64: Consider implementing IEquatable for better type-safety and performance.Implementing the
IEquatable<ThemeData>interface would provide better type-safety and could improve performance when comparing theme objects, especially when used in collections.-public class ThemeData +public class ThemeData : IEquatable<ThemeData> { // existing code... /// <inheritdoc /> public override bool Equals(object obj) { if (obj is not ThemeData other) return false; return FileNameWithoutExtension == other.FileNameWithoutExtension && Name == other.Name; } + + /// <summary> + /// Determines whether the specified theme is equal to the current theme. + /// </summary> + public bool Equals(ThemeData other) + { + if (other is null) + return false; + return FileNameWithoutExtension == other.FileNameWithoutExtension && + Name == other.Name; + }
33-39: Add parameter validation in the constructor.Consider adding validation for
fileNameWithoutExtensionandnameparameters to ensure they're not null or empty strings, as these are essential for theme identification.public ThemeData(string fileNameWithoutExtension, string name, bool? isDark = null, bool? hasBlur = null) { + if (string.IsNullOrEmpty(fileNameWithoutExtension)) + throw new ArgumentException("File name cannot be null or empty", nameof(fileNameWithoutExtension)); + if (string.IsNullOrEmpty(name)) + throw new ArgumentException("Theme name cannot be null or empty", nameof(name)); + FileNameWithoutExtension = fileNameWithoutExtension; Name = name; IsDark = isDark; HasBlur = hasBlur; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Plugin/SharedModels/ThemeData.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Flow.Launcher.Plugin/SharedModels/ThemeData.cs (1)
8-77: Well-structured data model with good encapsulation.The
ThemeDataclass is well-designed with immutable properties, proper documentation, and correct implementation of equality methods. The fixes for the null reference issues in equality operators from previous reviews have been correctly implemented.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Flow.Launcher/PublicAPIInstance.cs
Outdated
| public void SetCurrentTheme(ThemeData theme) | ||
| { | ||
| Theme.ChangeTheme(theme.FileNameWithoutExtension); | ||
| _ = _theme.RefreshFrameAsync(); |
There was a problem hiding this comment.
Consider awaiting RefreshFrameAsync() or properly handling its exceptions to prevent silent failures in theme refresh.
| _ = _theme.RefreshFrameAsync(); | |
| try | |
| { | |
| await _theme.RefreshFrameAsync(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Log the exception or handle it as needed | |
| Log.Error($"Failed to refresh theme: {ex.Message}"); | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@jjw24 I think we can remove _ = _theme.RefreshFrameAsync(); here since ChangeTheme already calls this function.
There was a problem hiding this comment.
For me, changing theme from Appreance Page and Sys plugin still work well
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.
Jack251970
changed the title
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs:38
- [nitpick] The equality check relies on the overridden operator== in ThemeData; ensure that this implementation meets all intended scenarios for comparing themes. If more granular control is needed, consider comparing specific key properties instead.
get => _selectedTheme ??= Themes.Find(v => v == App.API.GetCurrentTheme());
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs:38
- [nitpick] The equality check relies on the overloaded operator for ThemeData; for clarity, consider explicitly comparing key properties like FileNameWithoutExtension.
get => _selectedTheme ??= Themes.Find(v => v == App.API.GetCurrentTheme());
|
@jjw24 All resolved and I think it is good to go. |
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Flow.Launcher/PublicAPIInstance.cs:370
- Add validation to ensure that the passed 'theme' is not null before calling ChangeTheme, to avoid potential null reference exceptions.
public bool SetCurrentTheme(ThemeData theme) => Theme.ChangeTheme(theme.FileNameWithoutExtension);
@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 ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
366-373: Good implementation of theme setting with status feedback.The
SetCurrentTheme(ThemeData theme)method properly returns a boolean value to indicate success or failure, which is a best practice compared to void return types for operations that can fail.Consider expanding the documentation to clarify specific failure scenarios that would result in a
falsereturn value, such as invalid theme data, missing theme files, or permission issues./// <summary> /// Set the current theme /// </summary> /// <param name="theme"></param> /// <returns> /// True if the theme is set successfully, false otherwise. +/// Failures may occur if the theme is invalid, theme files are missing, or there are permission issues. /// </returns> public bool SetCurrentTheme(ThemeData theme);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(3 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/PublicAPIInstance.cs
🔇 Additional comments (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (3)
10-11: Appropriate imports for theme-related functionality.The newly added imports support the theme management functionality being introduced:
Flow.Launcher.Plugin.SharedModelscontains theThemeDatamodel used in the new methodsJetBrains.Annotationsprovides utilities like[NotNull]for better code quality
354-358: Well-defined method for retrieving available themes.The
GetAvailableThemes()method aligns with the PR objective to provide an API for retrieving all available themes. The method signature and documentation are clear and straightforward.
360-364: Well-defined method for retrieving the current theme.The
GetCurrentTheme()method aligns with the PR objective to provide an API for retrieving the currently active theme. The method signature and documentation are clear and straightforward.
New API Functions from Theme
Add new api functions to support plugin get available themes, get current theme and set current theme.
Improve
SettingsPaneThemePageandSysplugin with those api functions.Test