[FEATURE] Topmost automatically #4159
Conversation
|
🥷 Code experts: Jack251970, VictoriousRaptor 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughAdds a new boolean setting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ 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). (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
536-543: Consider the UX implications of automatically marking every selected result as topmost.When
AutoTopmostis enabled, every result a user selects from the query results will be automatically added to the topmost tracker for that query. This could lead to:
Rapid accumulation of topmost records: If a user frequently explores different results, many items could be marked as topmost, potentially diminishing the value of the feature.
Loss of intentional curation: The manual topmost feature (accessed via context menu, lines 1756-1797) allows users to deliberately mark important results. Auto-marking removes this intentionality.
No way to undo easily: Unlike the manual feature which has "Cancel topmost in this query" option, auto-marked results would need to be removed through the context menu, which users might not discover.
Consider adding guidance in the tooltip or implementing one of these enhancements:
- Add a frequency threshold (e.g., only mark as topmost if selected 2+ times)
- Limit the number of auto-topmost results per query
- Add a separate UI indicator or notification when a result is auto-marked as topmost
- Document this behavior more clearly in the feature description
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
📚 Learning: 2025-05-01T05:38:25.673Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3500
File: Flow.Launcher/Storage/TopMostRecord.cs:145-149
Timestamp: 2025-05-01T05:38:25.673Z
Learning: For the MultipleTopMostRecord implementation in Flow.Launcher, sequence order of records in the ConcurrentBag does not need to be preserved, as confirmed by the developer. The unordered nature of ConcurrentBag is acceptable for this implementation.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.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:
Flow.Launcher/Languages/en.xaml
📚 Learning: 2024-10-08T15:52:58.573Z
Learnt from: taooceros
Repo: Flow-Launcher/Flow.Launcher PR: 2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Applied to files:
Flow.Launcher/Languages/en.xaml
📚 Learning: 2025-06-08T14:12:21.348Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Applied to files:
Flow.Launcher/Languages/en.xaml
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Flow.Launcher/Languages/en.xaml
📚 Learning: 2025-09-28T03:57:43.995Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3770
File: Flow.Launcher/Helper/HotKeyMapper.cs:0-0
Timestamp: 2025-09-28T03:57:43.995Z
Learning: In Flow.Launcher/Helper/HotKeyMapper.cs, the resource descriptions for arrow keys are intentionally swapped - Up/Down keys use "HotkeyLeftRightDesc" and Left/Right keys use "HotkeyUpDownDesc". This is documented with an inline comment and is intended behavior, not a bug.
Applied to files:
Flow.Launcher/Languages/en.xaml
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
Flow.Launcher/Storage/QueryHistory.cs (1)
Add(36-65)Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-646)Flow.Launcher/Storage/TopMostRecord.cs (3)
AddOrUpdate(99-102)AddOrUpdate(130-140)AddOrUpdate(216-246)
⏰ 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 (2)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
499-511: LGTM!The property implementation follows the established pattern used throughout the Settings class, includes proper change notification, and has a safe default value of
false.Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
99-111: LGTM!The SettingsCard implementation follows the established patterns in this file, uses appropriate spacing and styling, and is logically placed next to the related "Show At Topmost" setting. The icon reuse (glyph
) is appropriate since both features relate to topmost behavior.Note: The misleading feature description flagged in en.xaml applies to this UI card as well, but the issue originates from the resource strings, not the XAML structure itself.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
99-111: Consider using a distinct icon for the AutoTopmostResults setting.The new setting uses the same icon glyph (
) as the ShowAtTopmost setting (Line 90). While these features are related, they serve different purposes—ShowAtTopmost controls whether the launcher window stays on top, while AutoTopmostResults appears to automatically prioritize frequently selected results. Using distinct icons would help users differentiate between these features at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/ViewModel/MainViewModel.cs
- Flow.Launcher/Languages/en.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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
499-512: LGTM!The AutoTopmostResults property implementation correctly follows the established pattern for boolean settings in this class, including proper INotifyPropertyChanged support and a sensible default value of false.
Co-authored-by: VictoriousRaptor <[email protected]>
jjw24
added
the
review in progress
jjw24
left a comment
jjw24
left a comment
There was a problem hiding this comment.
This leads to the MultipleTopMostRecord.json file growing indefinitely with unique queries when the option is on.
If the goal is to allow faster access to results by pushing frequently selected results to the top, I think the ideal change is to rebalance the 'selected record count' bump. The mechanism is there to allow frequently selected results to show up higher and seems like it is not as effective. We can rebalance it to have results show up better like this:
jjw24
dismissed
Jack251970’s stale review
Need discussion first. See my comment about the 'selected count' bump
Hmm, it works, but changing the current result balance may affect other users who are already satisfied with the existing behavior. What do you think about using the new option I added in my PR? When enabled, it would prioritize the last selected result by placing it at the top of the ranking, rebalancing the selected result with a higher score. The new option could be something like "Prioritize the last query as the top result". When this option is enabled, the query score could be set to 100, guaranteeing priority in the results. Otherwise, we can keep using the default value (5). |
|
@01Dri thank you for considering this option as well. I think currently I have seen some issues and comments that the current bump is not significant or obvious enough, users have had to select the result several times before they are satisfied with the placement. So I think the rebalancing is not going to be too negative on user experience. What I propose, since this option is significantly less effort, how about we bump it appropriately and once we get some feedback then we can consider adding the toggle option if it is negatively impacting the users. What do you guys think? |
|
One thing I would recommend is also we need to be mindful of introducing new features unless they are highly requested, because the more we add the more complex flow becomes to maintain. This is not to say this feature is not good, just that we can solve the root cause with something significantly simpler, it would be better for the long term. |
Ok for me. My friend will take care of this feature because I’m traveling at the moment. |
Sorry for the delay, I’ll take care of it. |
I’ve reviewed the code, and the actual value doesn’t matter. The logic is that if a user clicks, for example, Notepad four times and then clicks Sublime once, Sublime should not move up because it hasn’t surpassed the user’s favorite item. However, I think a better approach would be to always move an item to the top whenever the user clicks it. 202601062142.mp4 |
5 participants
Issue: #3984
Example:
2025-12-14.19-05-17.mp4