Add setting to disable thousands separator in Calculator plugin #4206
Conversation
Co-authored-by: Jack251970 <[email protected]>
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a configurable thousands-separator option to the Calculator plugin: new Settings property (default true), FormatResult respects the setting, UI checkbox and localization key added, and three NUnit tests added to validate enabled/disabled/large-number behavior. Changes
Sequence Diagram(s)(omitted — changes are localized conditional formatting, UI, and tests; no multi-component sequential flow introduced) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable setting to disable the thousands separator in Calculator plugin results, addressing user needs for cleaner copy-paste workflows while maintaining backward compatibility.
Changes:
- Added
UseThousandsSeparatorboolean setting with default valuetrue - Modified result formatting logic to conditionally apply thousands separator based on the setting
- Added UI checkbox control and English localization for the new setting
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Settings.cs | Adds new UseThousandsSeparator property with default value true for backward compatibility |
| Main.cs | Updates FormatResult() method to conditionally apply thousands separator based on setting |
| CalculatorSettings.xaml | Adds checkbox control for the new setting with proper Grid row assignment |
| Languages/en.xaml | Adds English localization key for the setting label |
| CalculatorTest.cs | Adds three test cases covering enabled/disabled states and large numbers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (1)
45-55: Consider strengthening the test assertion.The test allows either
"1,234"or"1234"as valid results whenUseThousandsSeparator = true. However, examiningFormatResultandGetGroupSeparatorin Main.cs, the group separator should always be applied when enabled for numbers > 3 digits. The current assertion may not catch bugs where the separator fails to apply.Consider asserting the expected formatted result more precisely:
- ClassicAssert.IsTrue(result.Contains(",") || result == "1234", - "Expected result to contain thousands separator or be without one if system doesn't use it"); + ClassicAssert.AreEqual("1,234", result, + "Expected result to contain thousands separator when enabled");Note: If the system locale affects the group separator character (could be "." in some locales), you may need to account for that.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Test/Plugins/CalculatorTest.csPlugins/Flow.Launcher.Plugin.Calculator/Languages/en.xamlPlugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Settings.csPlugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
📚 Learning: 2025-09-13T12:50:05.306Z
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Settings.csPlugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Languages/en.xamlPlugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xamlFlow.Launcher.Test/Plugins/CalculatorTest.cs
📚 Learning: 2025-09-13T12:50:05.306Z
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Languages/en.xamlFlow.Launcher.Test/Plugins/CalculatorTest.cs
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Flow.Launcher/HotkeyControlDialog.xaml:6-6
Timestamp: 2025-09-05T11:56:27.267Z
Learning: In Flow.Launcher's migration to iNKORE.UI.WPF.Modern UI framework, dialog resource keys like PopuBGColor, PopupButtonAreaBGColor, PopupButtonAreaBorderColor, and PopupTextColor are provided by the iNKORE.UI.WPF.Modern framework itself, not defined locally in the codebase theme files.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Languages/en.xaml
📚 Learning: 2025-09-13T06:04:26.977Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml
📚 Learning: 2025-09-11T08:30:29.487Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml:0-0
Timestamp: 2025-09-11T08:30:29.487Z
Learning: In Flow.Launcher's SettingsPaneTheme.xaml theme editor panel, the inner ui:ScrollViewerEx within the Border (Grid.Column="1") is intentionally kept because its height cannot be expanded to the whole page due to layout constraints. This serves a different scrolling purpose than the outer ScrollViewerEx and is necessary for the theme editor panel functionality.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: gitStream.cm
- GitHub Check: build
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (7)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
366-366: LGTM! Clean conditional implementation.The additional check for
_settings.UseThousandsSeparatorcorrectly gates the thousands separator formatting. The short-circuit evaluation ensures the grouping logic is skipped when the setting is disabled, meeting the PR objective to support plain numeric output for copy-paste workflows.Plugins/Flow.Launcher.Plugin.Calculator/Settings.cs (1)
11-11: LGTM! Property definition is correct.The
UseThousandsSeparatorproperty is properly defined with a sensible default oftrue, ensuring backward compatibility for existing users while enabling the new opt-out behavior requested in issue #4174.Plugins/Flow.Launcher.Plugin.Calculator/Languages/en.xaml (1)
17-17: LGTM! Localization string is clear and consistent.The new localization key follows the established naming convention and provides clear, user-friendly text for the UI checkbox control.
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml (1)
19-19: LGTM! UI bindings and layout are correctly implemented.The XAML changes properly integrate the new
UseThousandsSeparatorsetting:
- Added row definition to accommodate the layout
- First checkbox (row 2) correctly binds to
Settings.UseThousandsSeparatorwith two-way binding- Second checkbox (row 3) binds to
Settings.ShowErrorMessage- Both use
DynamicResourcefor localized content- Layout is clean and consistent with existing controls
Also applies to: 70-71, 73-81
Flow.Launcher.Test/Plugins/CalculatorTest.cs (3)
19-20: LGTM! Test initialization correctly includes the new setting.The settings initialization properly includes
UseThousandsSeparator = true, matching the default value and ensuring tests run with predictable state.
57-65: LGTM! Test correctly verifies the disabled state.This test properly validates that when
UseThousandsSeparator = false, the output is plain numeric text ("1234"), which directly addresses the copy-paste workflow issue described in issue #4174.
67-75: LGTM! Good coverage for large numbers.This test appropriately verifies that large numbers (which would have multiple separator positions) are formatted correctly as plain text when
UseThousandsSeparator = false. This provides good coverage for the feature.
| var result = GetCalculationResult("1000+234"); | ||
| // When thousands separator is enabled, the result should contain a separator (comma in this case) | ||
| ClassicAssert.IsTrue(result.Contains(",") || result == "1234", | ||
| "Expected result to contain thousands separator or be without one if system doesn't use it"); |
There was a problem hiding this comment.
If the result is fixed "1,234" why do we meed the OR condition here?
There was a problem hiding this comment.
If the result is fixed "1,234" why do we meed the OR condition here?
It is AI-generated. It has been modified.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Flow.Launcher.Test/Plugins/CalculatorTest.cs:
- Around line 117-123: The test sets _settings.DecimalSeparator to
DecimalSeparator.UseSystemLocale which makes expected dot-formatted results
locale-dependent; change the test to explicitly set _settings.DecimalSeparator =
DecimalSeparator.Dot (and keep _settings.UseThousandsSeparator = false) inside
CalculatorTest so the output uses '.' deterministically and tests won't fail on
comma-locales.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Test/Plugins/CalculatorTest.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
📚 Learning: 2025-09-13T12:50:05.306Z
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Flow.Launcher.Test/Plugins/CalculatorTest.cs
📚 Learning: 2025-09-13T12:50:05.306Z
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Flow.Launcher.Test/Plugins/CalculatorTest.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (5)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (5)
15-21: LGTM!The settings initialization correctly adds
UseThousandsSeparator = truewith a clear comment documenting the default value, aligning with the PR's backward compatibility goal.
22-28: LGTM!The engine configuration change adds the missing
econstant with a helpful comment explaining why it's needed, supporting the existingln(e)test case.
45-61: LGTM!This test effectively validates thousands separator formatting for both decimal separator configurations (dot → comma thousands, comma → dot thousands). The inline comments clearly explain the expected behavior.
63-71: LGTM!The test correctly validates that disabling the thousands separator produces plain numeric output (
"1234"), matching the PR objective for copy-paste workflows.
73-81: LGTM!Good coverage for larger numbers with multiple potential separator positions (millions), ensuring the disabled setting works correctly across the entire integer part.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Changes
UseThousandsSeparatorboolean property (defaults totrue)FormatResult()to conditionally apply separator based on setting:flowlauncher_plugin_calculator_use_thousands_separatorThe default value maintains backward compatibility while allowing users to disable separators for cleaner copy-paste workflows.
Fixes #4174