Fix JsonRPC Plugins Environment Issue & Improve Environment Selection #3407
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issues related to theme selection initialization and application dispose behavior while also refining environment path handling.
- Refactored ThemeSelector to use lazy initialization for the Theme dependency.
- Updated App.xaml.cs to set the ShutdownMode to OnExplicitShutdown to prevent premature application exit.
- Improved environment plugin configurations by updating translation calls, using static methods, and enhancing file path handling.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs | Refactors theme initialization by replacing the _theme field with a lazily initialized property. |
| Flow.Launcher/App.xaml.cs | Adds explicit ShutdownMode setting to prevent unintended application exit. |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Refactors and streamlines translation calls, file dialogs, and file path operations. |
Comments suppressed due to low confidence (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16
- [nitpick] Consider renaming the private field 'theme' to a more explicit name (e.g., '_themeInstance') to clearly differentiate it from the property 'Theme'.
private Theme theme = null;
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple files. The Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (6)
✨ 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 (
|
![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 (1)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (1)
36-36: Consider caching the themes list.The
Themesproperty now callsTheme.LoadAvailableThemes()on every access, which might be inefficient if this method is expensive or creates a new list each time. Consider caching the result in a field that's initialized on first access.-private List<Theme.ThemeData> Themes => Theme.LoadAvailableThemes(); +private List<Theme.ThemeData> _themes; +private List<Theme.ThemeData> Themes => _themes ??= Theme.LoadAvailableThemes();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(8 hunks)Flow.Launcher/App.xaml.cs(1 hunks)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-30T07:42:06.567Z
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 (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (11)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs (6)
1-12: Organized imports improve readabilityThe reorganization of the using directives improves code readability by grouping related imports together.
45-49: Improved code clarity with explanatory commentAdding a comment to explain the purpose of the conditional check improves code clarity. The consistent formatting of the conditional block also enhances readability.
61-62: Good practice: using API.GetTranslation instead of singletonSwitching from
InternationalizationManager.Instance.GetTranslationtoAPI.GetTranslationis a better approach as it leverages dependency injection rather than relying on a singleton. This improves testability and adheres to better architectural practices.Also applies to: 68-68, 88-88
126-126: Correctly marked method as staticMaking
GetFileFromDialogstatic is appropriate since it doesn't use any instance members. This follows the principle that methods not accessing instance state should be static.
201-201: Improved path handling with Path.CombineUsing
Path.Combinefor path construction is a good practice as it ensures proper handling of path separators across different platforms.
218-218: Modern C# syntax for substring extractionUsing the C# range operator (
..) for substring extraction is more concise and readable than the traditional substring method.Flow.Launcher/App.xaml.cs (1)
141-144: Good fix for premature application shutdown issue.The explicit setting of
ShutdownMode.OnExplicitShutdownaddresses the issue where the application exits unexpectedly when a message box is created and closed before the main window. This aligns with the PR objective to fix the application disposal issue mentioned in the PR description.Just ensure that the application is properly shutdown elsewhere in the codebase when needed. The current implementation using exit event handlers in
RegisterExitEvents()method seems to handle this correctly.Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs (4)
14-17: Good improvement to theme initialization logic.The lazy initialization pattern prevents potential null reference exceptions during application startup, addressing the theme select initialization issue mentioned in the PR objectives. By deferring theme initialization until it's actually needed, you avoid the issue when
Application.Current.Resources.MergedDictionariesmight not be fully initialized.
26-26: LGTM - Updated to use new Theme property.Correctly updated to use the lazily initialized Theme property.
30-30: LGTM - Updated to use new Theme property.Correctly updated to use the lazily initialized Theme property.
32-32: LGTM - Updated to use new Theme property.Correctly updated to use the lazily initialized Theme property.
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
Jack251970
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/Languages/en.xaml (1)
12-16: Fix typo in the new string resource.There's a typo in the message: "agian" should be "again" in line 15.
- Click yes if you would like select the {0} executable agian. Click no if you would like to download {1} + Click yes if you would like select the {0} executable again. Click no if you would like to download {1}Additionally, there's a slight grammar issue - "would like select" should be "would like to select".
- Click yes if you would like select the {0} executable again. Click no if you would like to download {1} + Click yes if you would like to select the {0} executable again. Click no if you would like to download {1}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs(8 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues related to theme initialization and application disposal and enhances the environment selection process for external plugins. Key changes include:
- Delaying theme initialization in ThemeSelector.cs via lazy instantiation.
- Preventing premature application shutdown in App.xaml.cs by setting ShutdownMode to OnExplicitShutdown.
- Improving file path handling and user prompts for environment selection in AbstractPluginEnvironment.cs.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs | Uses lazy instantiation for Theme to avoid null reference issues. |
| Flow.Launcher/App.xaml.cs | Sets ShutdownMode to OnExplicitShutdown to prevent application exit due to early message box disposal. |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Refactors file dialog handling and environment path updates, and improves user prompt loop logic. |
Files not reviewed (1)
- Flow.Launcher/Languages/en.xaml: Language not supported
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs:252
- [nitpick] Variable 'ExecutablePathSubstring' does not follow typical C# camelCase conventions. Consider renaming it to 'executablePathSubstring'.
var ExecutablePathSubstring = filePath[(index + DataLocation.PluginEnvironments.Length)..];
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses three issues: fixing a theme selection initialization bug, preventing premature application shutdown by setting an explicit shutdown mode, and improving the environment selection workflow for plugins by prompting or downloading a valid executable path.
- Fixes a null reference issue in theme initialization by deferring Theme retrieval via lazy loading.
- Prevents the application from exiting before the main window is created by setting ShutdownMode to OnExplicitShutdown.
- Enhances the environment selection process within plugin environments by refining file path management and user prompts.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs | Refactors theme initialization to lazy-load the Theme service and updates theme-related method calls. |
| Flow.Launcher/App.xaml.cs | Sets ShutdownMode to OnExplicitShutdown to resolve premature application disposal. |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Reorders using directives and consolidates dependency injections. |
| Flow.Launcher.Core/ExternalPlugins/Environments/*.cs | Updates PluginsSettingsFilePath properties and refines file path handling and user prompts. |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Improves the environment setup flow and updates error messaging to use the new API translations. |
Files not reviewed (1)
- Flow.Launcher/Languages/en.xaml: Language not supported
Comments suppressed due to low confidence (2)
Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16
- [nitpick] Consider renaming the private field 'theme' to something like '_themeInstance' to clearly differentiate it from the public property 'Theme'.
private Theme theme = null;
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs:252
- Consider checking that 'index' is not -1 before using the range operator for substring extraction to avoid potential exceptions if the expected substring is not found in filePath.
var executablePathSubstring = filePath[(index + DataLocation.PluginEnvironments.Length)..];
@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 ...
|
|
🥷 Code experts: jjw24 Jack251970 has 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:
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 |
Fix theme select initialization issue
Issue from onesounds.
Fix application dispose issue
Follow on with #3093.
Because new message box api uses
MessageBoxExwindow, if it is created and closed before main window is created, it will cause the application to exit. So we should setShutdownModetoOnExplicitShutdownto prevent the application from shutting down before main window is created.Improve Environment Selection
If users' selected environment path is invalid, FL will prompt users to select the path again or download until they select a valid path or choose to download.