Add setting to disable non-error notifications#4429
Open
1203zxy wants to merge 11 commits intoFlow-Launcher:devfrom
Open
Add setting to disable non-error notifications#44291203zxy wants to merge 11 commits intoFlow-Launcher:devfrom
1203zxy wants to merge 11 commits intoFlow-Launcher:devfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Settings flag Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (plugin/feature)
participant PublicAPI as PublicAPIInstance
participant Settings as Settings (EnableSuccessNotification)
participant Notifier as Notification Service
Caller->>PublicAPI: ShowMsg(title, subtitle, iconPath, useMainWindowAsOwner?, forceShown?)
PublicAPI->>Settings: read EnableSuccessNotification
alt forceShown == true
PublicAPI->>Notifier: Notification.Show(...)
else if EnableSuccessNotification == false AND iconPath.toLower() != Constant.ErrorIcon.toLower()
PublicAPI-->>Caller: return (suppressed)
else
PublicAPI->>Notifier: Notification.Show(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
130-153: Extract the suppression check into a small helper to avoid duplication.The same 4-line guard is duplicated in
ShowMsgandShowMsgWithButton. A tiny helper keeps the intent in one place and makes future tweaks (e.g., also comparing against other non-error icons) straightforward.♻️ Suggested refactor
+ private bool ShouldSuppressNonErrorNotification(string iconPath) => + _settings.DisableSuccessNotifications && + !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase); + public void ShowMsg(string title, string subTitle, string iconPath, bool useMainWindowAsOwner = true) { - if (_settings.DisableSuccessNotifications && - !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase)) - { - return; - } + if (ShouldSuppressNonErrorNotification(iconPath)) return; Notification.Show(title, subTitle, iconPath); } @@ public void ShowMsgWithButton(string title, string buttonText, Action buttonAction, string subTitle, string iconPath, bool useMainWindowAsOwner = true) { - if (_settings.DisableSuccessNotifications && - !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase)) - { - return; - } + if (ShouldSuppressNonErrorNotification(iconPath)) return; Notification.ShowWithButton(title, buttonText, buttonAction, subTitle, iconPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/PublicAPIInstance.cs` around lines 130 - 153, Extract the duplicated suppression guard into a small private helper (e.g., bool ShouldSuppressNotification(string iconPath)) that returns _settings.DisableSuccessNotifications && !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase); then replace the four-line guards in ShowMsg and ShowMsgWithButton to call ShouldSuppressNotification(iconPath) and return when true; keep remaining calls to Notification.Show and Notification.ShowWithButton unchanged so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 132-136: The current guard in PublicAPIInstance.cs treats only
Constant.ErrorIcon as an "error" so custom error-like icons passed to
ShowMsg(...) or ShowMsgWithButton(...) can be suppressed when
_settings.DisableSuccessNotifications is true; add XML documentation comments to
the IPublicAPI members (e.g., ShowMsg, ShowMsgWithButton, ShowMsgError,
ShowMsgErrorWithButton) explaining this behavior and advising plugin authors to
call ShowMsgError/ShowMsgErrorWithButton for error messages (or use
Constant.ErrorIcon) so their error notifications are not hidden; reference
Constant.ErrorIcon and the guard in PublicAPIInstance.cs in the docs for
clarity.
In `@Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml`:
- Around line 95-103: The SettingsPane shows a double-negative: the SettingsCard
header reads as "Disable..." while the ToggleSwitch uses OnContent/OffContent
"Enable"/"Disable", which is confusing; update either the header to a positive
phrasing (e.g., "ShowSuccessNotifications") and invert the bound value for
ToggleSwitch IsOn by using a Boolean inverter converter or renaming the bound
property (Settings.DisableSuccessNotifications ->
Settings.ShowSuccessNotifications), or keep the negative property and simply
remove the ToggleSwitch OnContent/OffContent attributes so the control uses its
default visuals; apply the same change consistently to other toggles in this
pane (references: ui:SettingsCard, ui:ToggleSwitch,
Settings.DisableSuccessNotifications, OnContent, OffContent).
---
Nitpick comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 130-153: Extract the duplicated suppression guard into a small
private helper (e.g., bool ShouldSuppressNotification(string iconPath)) that
returns _settings.DisableSuccessNotifications && !string.Equals(iconPath,
Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase); then replace the
four-line guards in ShowMsg and ShowMsgWithButton to call
ShouldSuppressNotification(iconPath) and return when true; keep remaining calls
to Notification.Show and Notification.ShowWithButton unchanged so behavior is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 009c9a45-f2ea-4cfb-aa75-29d8b5615945
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher/Languages/en.xamlFlow.Launcher/PublicAPIInstance.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
Jack251970
requested changes
Apr 26, 2026
Member
Jack251970
left a comment
Jack251970
left a comment
There was a problem hiding this comment.
Double-negative toggle wording may confuse users.
I think we should change this option to EnableSuccessNotification
Author
Ok, I'll update the option to EnableSuccessNotification to avoid the double-negative wording confusion. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
130-153: Optional: extract the suppression check to a small helper to avoid duplication.The same guard is repeated verbatim in
ShowMsgandShowMsgWithButton. A tiny helper keeps the policy in one place so any future refinement (e.g., recognizing additional error-icon variants) only has to change once.♻️ Proposed refactor
+ private bool ShouldSuppressNotification(string iconPath) => + !_settings.EnableSuccessNotification && + !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase); + public void ShowMsg(string title, string subTitle, string iconPath, bool useMainWindowAsOwner = true) { - if (!_settings.EnableSuccessNotification && - !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase)) - { - return; - } + if (ShouldSuppressNotification(iconPath)) return; Notification.Show(title, subTitle, iconPath); } public void ShowMsgWithButton(string title, string buttonText, Action buttonAction, string subTitle, string iconPath, bool useMainWindowAsOwner = true) { - if (!_settings.EnableSuccessNotification && - !string.Equals(iconPath, Constant.ErrorIcon, StringComparison.OrdinalIgnoreCase)) - { - return; - } + if (ShouldSuppressNotification(iconPath)) return; Notification.ShowWithButton(title, buttonText, buttonAction, subTitle, iconPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/PublicAPIInstance.cs` around lines 130 - 153, The duplicate guard that checks _settings.EnableSuccessNotification and compares iconPath to Constant.ErrorIcon appears in ShowMsg and ShowMsgWithButton; extract it into a small helper (e.g., CanShowNotification or ShouldSuppressNotification) that accepts the iconPath and returns a bool, then replace the inline checks in ShowMsg and ShowMsgWithButton with that helper and keep the Notification.Show and Notification.ShowWithButton calls unchanged so future policy changes are applied in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 132-136: The current check using
_settings.EnableSuccessNotification together with iconPath/Constant.ErrorIcon
also suppresses in-progress/informational notifications (e.g., callers like
Updater.cs calling ShowMsg("Checking for new update") with empty iconPath), so
update the UI/tooltip and any related text to clearly state that this setting
controls non-error notifications (both success and informational pop-ups) while
errors are always shown; search for _settings.EnableSuccessNotification,
ShowMsg, Constant.ErrorIcon and update the displayed tooltip/label text (and any
user-facing help) to something like "Show non-error notifications (success and
informational pop-ups). Error notifications are always shown." Also apply the
same text change to the other occurrence referenced (lines 146-150).
---
Nitpick comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 130-153: The duplicate guard that checks
_settings.EnableSuccessNotification and compares iconPath to Constant.ErrorIcon
appears in ShowMsg and ShowMsgWithButton; extract it into a small helper (e.g.,
CanShowNotification or ShouldSuppressNotification) that accepts the iconPath and
returns a bool, then replace the inline checks in ShowMsg and ShowMsgWithButton
with that helper and keep the Notification.Show and Notification.ShowWithButton
calls unchanged so future policy changes are applied in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdde3a67-9809-41b6-8f50-fdf4e72cf094
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher/Languages/en.xamlFlow.Launcher/PublicAPIInstance.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Infrastructure/UserSettings/Settings.cs">
<violation number="1" location="Flow.Launcher.Infrastructure/UserSettings/Settings.cs:465">
P2: Renaming this persisted setting without a compatibility migration breaks existing user settings files and will re-enable success notifications after upgrade.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Clarified the "enableSuccessNotification" label to indicate it enables all non-error notifications, not just success notifications.
Added forceShown to ShowMsg in IPublicAPI and updated PublicAPIInstance to support forced notifications. Error messages now always show regardless of user settings. Simplified ShowMsgWithButton to always display. Changed _saveSettingsLock to Lock type and made minor formatting improvements.
Added a new overload of the ShowMsg method in JsonRPCPublicAPI to support a forceShown parameter, enabling callers to control whether the message is forcibly displayed. This overload delegates to the underlying _api.ShowMsg with the new argument.
Refactored message display calls to use error-specific methods (e.g., ShowMsgError) and added iconPath and forceShown parameters. These changes improve user feedback and ensure consistent message presentation, especially for errors and update notifications.
Contributor
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/PublicAPIInstance.cs">
<violation number="1" location="Flow.Launcher/PublicAPIInstance.cs:128">
P0: ShowMsg's 3-argument overload recursively calls itself, so normal notification calls overflow the stack instead of displaying a message.</violation>
<violation number="2" location="Flow.Launcher/PublicAPIInstance.cs:131">
P0: ShowMsg's 4-argument overload recursively calls itself, so callers that use the bool overload never reach the notification implementation.</violation>
<violation number="3" location="Flow.Launcher/PublicAPIInstance.cs:145">
P0: ShowMsgWithButton's short overload recursively calls itself, breaking button notifications and the error-button path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Explicitly set useMainWindowAsOwner and forceShown parameters in message and notification methods to ensure consistent notification display and ownership context. This improves reliability and fixes potential issues with notification visibility.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Flow.Launcher/PublicAPIInstance.cs (1)
122-122: Nit: spacing in named arguments.
forceShown:trueanduseMainWindowAsOwner:useMainWindowAsOwnerare missing the conventional space after the colon. Most of the codebase (anddotnet formatdefaults) usesname: value.♻️ Proposed tweak
- ShowMsg(title, subTitle, Constant.ErrorIcon, forceShown:true); + ShowMsg(title, subTitle, Constant.ErrorIcon, forceShown: true); ... - ShowMsg(title, subTitle, iconPath, useMainWindowAsOwner:useMainWindowAsOwner); + ShowMsg(title, subTitle, iconPath, useMainWindowAsOwner: useMainWindowAsOwner);Also applies to: 131-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/PublicAPIInstance.cs` at line 122, The named-argument formatting is inconsistent: update the calls to ShowMsg so named arguments follow the "name: value" convention (add a space after the colon) — specifically change occurrences using forceShown:true and useMainWindowAsOwner:useMainWindowAsOwner to forceShown: true and useMainWindowAsOwner: useMainWindowAsOwner in the ShowMsg(...) invocations to match project formatting (e.g., the call within PublicAPIInstance.cs where ShowMsg(title, subTitle, Constant.ErrorIcon, forceShown:true) is used and the similar call at the later occurrence).Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
139-147: Adding an abstract method to a public interface is a binary-breaking change for external implementers.Anyone outside the repo who implements
IPublicAPI(tests, alternative hosts, mocks) will fail to compile after this PR. Since the new overload is just a strict superset of the 4-arg one, consider giving it a default interface implementation that forwards to the existing overload, so external implementers aren't forced to update:♻️ Optional: default interface implementation
- void ShowMsg(string title, string subTitle, string iconPath, bool useMainWindowAsOwner = true, bool forceShown = false); + void ShowMsg(string title, string subTitle, string iconPath, bool useMainWindowAsOwner = true, bool forceShown = false) + => ShowMsg(title, subTitle, iconPath, useMainWindowAsOwner);Then
PublicAPIInstanceoverrides it as today. This preserves binary compat for third-partyIPublicAPIimplementers while giving plugins access to the new behavior when running against the official host.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs` around lines 139 - 147, You added a new ShowMsg overload to IPublicAPI which is a binary-breaking change for external implementers; make it a default interface implementation that forwards to the existing 4-argument ShowMsg so external implementers aren’t forced to recompile. In IPublicAPI provide a default body for the new ShowMsg(string title, string subTitle, string iconPath, bool useMainWindowAsOwner = true, bool forceShown = false) that simply calls the existing ShowMsg(title, subTitle, iconPath, useMainWindowAsOwner), and keep PublicAPIInstance overriding behavior as-is.Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (1)
113-114: Minor:alreadyexistsis more "info/no-op" than "error".Switching this to
ShowMsgErrorwill now displayapp_error.png(red error badge) and route through the always-shown path. The condition is simply "this plugin is already at this version, nothing to do" — not really an error, just an idempotent skip. The existing pattern elsewhere (e.g.Flow.Launcher/CustomShortcutSetting.xaml.cs:48-52) usesShowMsgBox(Localize.duplicateShortcut())for similar conflicts.If the goal is just to ensure the message isn't suppressed when success notifications are off, an alternative is to keep
ShowMsgand passforceShown: trueso the visual cue still says "informational" rather than "error".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs` around lines 113 - 114, Replace the error-style notification for the "already exists" case in PluginsManager (the Context.API.ShowMsgError call) with an informational notification: call Context.API.ShowMsg(Context.API.GetTranslation("plugin_pluginsmanager_update_alreadyexists"), forceShown: true) (or use Context.API.ShowMsgBox/ShowMsgBox(Localize.duplicateShortcut()) pattern if you prefer a modal) so the message is shown as an informational/no-op rather than an error; leave the subsequent return in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs`:
- Line 146: The XML doc for the parameter forceShown incorrectly references a
non-existent setting name "Show more notifications"; update the doc comment in
IPublicAPI.cs for the forceShown parameter to reference the actual setting key
EnableSuccessNotification or use the user-facing label (e.g., "Show success
notifications") or a neutral descriptive phrase (e.g., "regardless of the user's
success notification setting") so plugin authors can find the correct toggle;
ensure the doc mentions the setting by its code name EnableSuccessNotification
and/or the user-facing label used in en.xaml and keep the description concise.
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 124-125: ShowMsgErrorWithButton currently calls the 6-arg
ShowMsgWithButton which doesn't honor the "always show on error" behavior in
ShowMsg; either mirror the gating logic there or plumb a forceShown flag
through. Update ShowMsgWithButton (the overload that calls
Notification.ShowWithButton) to check for iconPath == Constant.ErrorIcon (or
accept a bool forceShown parameter) and bypass EnableSuccessNotification when
true, and change ShowMsgErrorWithButton to pass forceShown=true (or call the new
overload) so error popups with buttons are always displayed.
In
`@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingDownloadHelper.cs`:
- Around line 49-50: The success notification is being forced via
api.ShowMsg(..., forceShown: true) which bypasses EnableSuccessNotification; in
EverythingDownloadHelper remove the forceShown: true argument from the
"installation success" api.ShowMsg call so the success toast respects the user's
EnableSuccessNotification setting (optionally also remove forceShown: true from
the "installing..." api.ShowMsg call if you decide it should be treated as a
non-essential courtesy notification).
---
Nitpick comments:
In `@Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs`:
- Around line 139-147: You added a new ShowMsg overload to IPublicAPI which is a
binary-breaking change for external implementers; make it a default interface
implementation that forwards to the existing 4-argument ShowMsg so external
implementers aren’t forced to recompile. In IPublicAPI provide a default body
for the new ShowMsg(string title, string subTitle, string iconPath, bool
useMainWindowAsOwner = true, bool forceShown = false) that simply calls the
existing ShowMsg(title, subTitle, iconPath, useMainWindowAsOwner), and keep
PublicAPIInstance overriding behavior as-is.
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Line 122: The named-argument formatting is inconsistent: update the calls to
ShowMsg so named arguments follow the "name: value" convention (add a space
after the colon) — specifically change occurrences using forceShown:true and
useMainWindowAsOwner:useMainWindowAsOwner to forceShown: true and
useMainWindowAsOwner: useMainWindowAsOwner in the ShowMsg(...) invocations to
match project formatting (e.g., the call within PublicAPIInstance.cs where
ShowMsg(title, subTitle, Constant.ErrorIcon, forceShown:true) is used and the
similar call at the later occurrence).
In `@Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs`:
- Around line 113-114: Replace the error-style notification for the "already
exists" case in PluginsManager (the Context.API.ShowMsgError call) with an
informational notification: call
Context.API.ShowMsg(Context.API.GetTranslation("plugin_pluginsmanager_update_alreadyexists"),
forceShown: true) (or use
Context.API.ShowMsgBox/ShowMsgBox(Localize.duplicateShortcut()) pattern if you
prefer a modal) so the message is shown as an informational/no-op rather than an
error; leave the subsequent return in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffa741a2-8992-43b9-8adc-f35a8b8611a0
📒 Files selected for processing (8)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.csFlow.Launcher.Core/Plugin/PluginManager.csFlow.Launcher.Core/Updater.csFlow.Launcher.Plugin/Interfaces/IPublicAPI.csFlow.Launcher/Languages/en.xamlFlow.Launcher/PublicAPIInstance.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingDownloadHelper.csPlugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/Languages/en.xaml
Update all ShowMsg calls in PluginInstaller.cs and PluginsManager.cs to include iconPath (empty string) and forceShown (true). This ensures consistent message display behavior and no icon for plugin install, update, and uninstall notifications.
Increased the top margin of the "enableSuccessNotification" SettingsCard from 4 to 14 units for improved spacing. Added a header icon (FontIcon with glyph ) to enhance visual clarity.
Jack251970
approved these changes
Apr 26, 2026
Member
Jack251970
left a comment
Jack251970
left a comment
There was a problem hiding this comment.
LGTM! Please wait for jjw24's review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
56-56: Unrelated refactor: use ofSystem.Threading.Lockinstead ofobjectfor_saveSettingsLock.This change is compatible with the project's .NET 9 target but unrelated to the PR's scope (adding notification gating). Keep off-topic refactors in separate PRs to maintain clear change history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/PublicAPIInstance.cs` at line 56, The field _saveSettingsLock was changed to use System.Threading.Lock which is an unrelated refactor; revert it to the original pattern using a private readonly object (e.g., private readonly object _saveSettingsLock = new();), and update any Monitor/lock usages that rely on _saveSettingsLock to work with an object; keep this refactor out of this PR and restore the original _saveSettingsLock type and initialization in PublicAPIInstance (symbol: _saveSettingsLock) so the change history stays focused on the notification gating work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 130-131: The 4-arg ShowMsg overload is dropping the incoming
useMainWindowAsOwner because it forwards a hard-coded true; update the wrapper
so ShowMsg(string title, string subTitle, string iconPath, bool
useMainWindowAsOwner = true) forwards the actual useMainWindowAsOwner value to
the 5-arg ShowMsg(title, subTitle, iconPath, useMainWindowAsOwner:
useMainWindowAsOwner, forceShown: false), or remove the 4-arg overload entirely
and rely on the existing 5-arg ShowMsg that defaults forceShown to false to
eliminate the forwarding hazard.
- Around line 133-142: The public API methods ShowMsg and ShowMsgWithButton
declare a useMainWindowAsOwner parameter but never use it—either plumb it
through by adding owner-aware overloads in the Notification class (e.g.,
Notification.Show(string title, string subTitle, string iconPath = null, bool
useMainWindowAsOwner = true) and Notification.ShowWithButton(string title,
string buttonText, Action buttonAction, string subTitle, string iconPath = null,
bool useMainWindowAsOwner = true)) and pass useMainWindowAsOwner from
PublicAPIInstance.ShowMsg/ShowMsgWithButton, or remove the unused
useMainWindowAsOwner parameter from the ShowMsg and ShowMsgWithButton signatures
to avoid a misleading API; update corresponding calls to Notification.Show and
Notification.ShowWithButton accordingly.
---
Nitpick comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Line 56: The field _saveSettingsLock was changed to use System.Threading.Lock
which is an unrelated refactor; revert it to the original pattern using a
private readonly object (e.g., private readonly object _saveSettingsLock =
new();), and update any Monitor/lock usages that rely on _saveSettingsLock to
work with an object; keep this refactor out of this PR and restore the original
_saveSettingsLock type and initialization in PublicAPIInstance (symbol:
_saveSettingsLock) so the change history stays focused on the notification
gating work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c33834bf-d07f-4df6-81bb-2d0a715bacc7
📒 Files selected for processing (1)
Flow.Launcher/PublicAPIInstance.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginInstaller.cs (1)
307-308: Forcing the “no updates available” toast may surprise users who disabled success notifications.The
silentUpdate == falsebranch already gates this on the caller’s explicit request to surface a result, butforceShown: trueadditionally bypassesEnableSuccessNotification. For users who disabled the setting because they want a silent workflow, an informational "no results" pop-up is arguably the most expendable of the new forced messages. Consider droppingforceShown: truehere so the user setting still applies, even though the caller asked for non-silent behavior.♻️ Suggested change
- PublicApi.Instance.ShowMsg(Localize.updateNoResultTitle(), Localize.updateNoResultSubtitle(), iconPath: "", - forceShown: true); + PublicApi.Instance.ShowMsg(Localize.updateNoResultTitle(), Localize.updateNoResultSubtitle());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Plugin/PluginInstaller.cs` around lines 307 - 308, The ShowMsg call is forcing a "no updates" toast despite user preference; remove the forceShown: true argument from PublicApi.Instance.ShowMsg (the invocation using Localize.updateNoResultTitle/Subtitle) so that the existing silentUpdate handling respects EnableSuccessNotification, i.e., call ShowMsg without the forceShown override when silentUpdate == false so the user's success-notification setting is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Flow.Launcher.Core/Plugin/PluginInstaller.cs`:
- Around line 307-308: The ShowMsg call is forcing a "no updates" toast despite
user preference; remove the forceShown: true argument from
PublicApi.Instance.ShowMsg (the invocation using
Localize.updateNoResultTitle/Subtitle) so that the existing silentUpdate
handling respects EnableSuccessNotification, i.e., call ShowMsg without the
forceShown override when silentUpdate == false so the user's
success-notification setting is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c6688e2-9125-49fe-a09f-dd5ba64508d7
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginInstaller.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlPlugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
- Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add option to disable success notifications.
Related Issue
Fixes #4423
Changes
Summary by cubic
Adds a setting to hide non-error notifications across Flow Launcher. Errors and forced messages still show; the Settings toggle text is clearer with small UI tweaks.
Summary of changes
Settings.EnableSuccessNotification. Non-errorShowMsgcalls are suppressed unlessforceShownis true. Errors always display viaShowMsgError/ShowMsgErrorWithButton(updated callers incl. plugin init failures and Plugins Manager “already exists”). Standardized notifications in installer/manager/updater/Everything flows to passiconPath: "",forceShown: true, and use the main window as owner.EnableSuccessNotification(default true) with a General settings toggle labeled “Show more notifications” and tooltip (plus spacing and a header icon). AddedIPublicAPI.ShowMsg(..., forceShown)and aJsonRPCPublicAPIoverload;PublicAPIInstancegates display based on the setting andforceShown. Backward-compatibleShowMsgoverloads retained._saveSettingsLockobject withLock; no behavior change.Written for commit 9640686. Summary will update on new commits.