Improve Topmost Feature #3728
Improve Topmost Feature #3728
Conversation
📝 WalkthroughWalkthroughThe changes update the default value and description of the "Show At Topmost" setting, modify its explanatory text in the English language file, and adjust the icon and spacing for its UI representation in the settings pane. No changes were made to logic, bindings, or public APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings UI
participant Settings.cs
User ->> Settings UI: Opens General Settings
Settings UI ->> Settings.cs: Reads ShowAtTopmost default value
Settings.cs -->> Settings UI: Returns false (new default)
Settings UI -->> User: Displays updated text and icon for "Show At Topmost"
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ 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)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
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.
|
@onesounds If you have a chance, could you please implement the TODOs as described above? |
This comment has been minimized.
This comment has been minimized.
|
Done. I’m not entirely happy with using the converter (since I’m not sure if it will ever be reused in the future, or if it might need to handle reverse logic at some point — in which case, we might consider renaming it). Anyway, I’ll leave the wording and the converter details up to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher/Converters/BoolNegationConverter.cs (1)
7-22: Minor polishing: mark the converter assealedand add[ValueConversion]attributeThe converter is stateless and will never be inherited from.
Making itsealedenables inlining and reflects intent.
Adding the attribute improves tooling support (Blend shows valid source/target types).- public class BoolNegationConverter : IValueConverter + [ValueConversion(typeof(bool), typeof(bool))] + public sealed class BoolNegationConverter : IValueConverterFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
67-83: Explicitly set binding mode forIsEnabledto clarify intent
IsEnabledis a read-only DP from the view-model’s perspective, but withoutMode=OneWayWPF will still default toTwoWayand try to push updates back, which generates superfluous binding-errors in the debug window.- OnContent="{DynamicResource enable}" IsEnabled="{Binding Settings.HideWhenDeactivated, Converter={StaticResource BoolNegationConverter}}"/> + OnContent="{DynamicResource enable}" + IsEnabled="{Binding Settings.HideWhenDeactivated, + Converter={StaticResource BoolNegationConverter}, + Mode=OneWay}" />This is purely defensive but avoids noisy logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/Converters/BoolNegationConverter.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/Languages/en.xaml
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
- GitHub Check: Check Spelling
🔇 Additional comments (1)
Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
18-22: Good addition of a reusable converter resourceRegistering
BoolNegationConverterat page scope keeps XAML tidy and avoids multiple instantations.
Nothing to change here.
| public class BoolNegationConverter : IValueConverter | ||
| { | ||
| public object Convert(object value, Type targetType, object parameter, CultureInfo culture) | ||
| { | ||
| if (value is bool b) | ||
| return !b; | ||
| return value; | ||
| } | ||
|
|
||
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | ||
| { | ||
| if (value is bool b) | ||
| return !b; | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Return a sentinel value for non-boolean inputs to avoid runtime binding errors
Convert/ConvertBack currently return the original value when it is not a bool.
For bindings where the target expects a bool, returning, e.g. a string will raise a runtime error and WPF will silently dump a binding‐error in the output window.
- if (value is bool b)
- return !b;
- return value;
+ if (value is bool b)
+ return !b;
+ return DependencyProperty.UnsetValue; // let WPF decide what to doYou may need using System.Windows; for DependencyProperty.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class BoolNegationConverter : IValueConverter | |
| { | |
| public object Convert(object value, Type targetType, object parameter, CultureInfo culture) | |
| { | |
| if (value is bool b) | |
| return !b; | |
| return value; | |
| } | |
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | |
| { | |
| if (value is bool b) | |
| return !b; | |
| return value; | |
| } | |
| } | |
| public class BoolNegationConverter : IValueConverter | |
| { | |
| public object Convert(object value, Type targetType, object parameter, CultureInfo culture) | |
| { | |
| if (value is bool b) | |
| return !b; | |
| return DependencyProperty.UnsetValue; // let WPF decide what to do | |
| } | |
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | |
| { | |
| if (value is bool b) | |
| return !b; | |
| return DependencyProperty.UnsetValue; // let WPF decide what to do | |
| } | |
| } |
🤖 Prompt for AI Agents
In Flow.Launcher/Converters/BoolNegationConverter.cs around lines 7 to 22, the
Convert and ConvertBack methods return the original value when it is not a bool,
which can cause runtime binding errors in WPF if the target expects a bool. To
fix this, modify both methods to return DependencyProperty.UnsetValue for
non-boolean inputs instead of the original value. Add "using System.Windows;" if
needed to access DependencyProperty.
why do we need this disable relationship? even if hiding option is on, this option can help make sure Flow window is on topper of other Topmost windows. |
|
As I understand it, this feature only works when "lost focus" is turned off, right? I'm genuinely asking because I’m not sure. From what I know, this feature operates only when "lost focus" is off. If it's on, the window hides anyway when focus is lost, so this feature wouldn't make sense. If this isn't the functionality I’m thinking of, then I guess I don’t really understand what this feature is. I don't think I'm the right person to write or organize the description for it. Wait a second — is this feature actually meant to ignore other programs' "Always on Top" setting? |
This reverts commit f30c6fc.
This comment has been minimized.
This comment has been minimized.
- en.xaml: Updated "showAtTopmost" and tooltip text - SettingsPaneGeneral.xaml: Added "Hide Notify Icon" card and updated icon - Adjusted card group margins
@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: onesounds onesounds, Jack251970 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: To learn more about /:\ gitStream - Visit our Docs |
|
|
|
@onesounds I think it is good to go👍 |
Changes
Follow on with #3541.
TODO