Fix results context menu display issue #3538
Merged
Conversation
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
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: onesounds, jjw24 Jack251970, onesounds 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 |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the issue where both results and context menu were displayed simultaneously by updating the visibility logic in the ResultsViewModel based on selection status.
- Introduces a MainViewModel reference into ResultsViewModel and updates its constructor.
- Updates initialization code in MainViewModel and SettingsPaneThemeViewModel to align with the new constructor signature.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Added _mainVM field and adjusted UpdateResults to update visibility only when results are selected. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Modified constructors to pass 'this' and added the ResultsSelected method. |
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Updated ResultsViewModel instantiation for preview purposes by passing null for MainViewModel. |
|
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough## Walkthrough
The constructors for `ResultsViewModel` in several view models were updated to accept an additional parameter, typically the parent `MainViewModel` or `null`. Logic in `ResultsViewModel` now uses this reference to determine result selection and visibility. A new method was added to `MainViewModel` to check if a results instance is currently selected.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------|
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Updated `ResultsViewModel` instantiation to include an explicit `null` as the second parameter in the constructor. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Updated `ResultsViewModel` instantiations to pass `this` as the second argument; added `ResultsSelected` method. |
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Modified constructor to accept and store a `MainViewModel` reference; updated result visibility logic to check selection state.|
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant MainViewModel
participant ResultsViewModel
MainViewModel->>ResultsViewModel: new ResultsViewModel(settings, this)
ResultsViewModel->>MainViewModel: _mainVM.ResultsSelected(this) (during UpdateResults)
alt ResultsViewModel is selected
ResultsViewModel->>ResultsViewModel: Set SelectedIndex and Visibility
else Not selected or _mainVM is null
ResultsViewModel->>ResultsViewModel: Skip selection/visibility update
endAssessment against linked issues
Suggested labels
Suggested reviewers
Poem
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with the simultaneous display of results and context menus.
- Injects MainViewModel into ResultsViewModel for better control over result selection visibility.
- Adds a cancellation token check in UpdateResults and updates MainViewModel and SettingsPaneThemeViewModel to pass the MainViewModel (or null for preview).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Modified constructors, added _mainVM field, and updated UpdateResults logic with a cancellation check. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Updated instantiations of ResultsViewModel and added ResultsSelected method. |
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Updated ResultsViewModel usage for preview mode by passing null for MainViewModel. |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue where results and context menus were displayed simultaneously by updating visibility logic in the ResultsViewModel. Key changes include introducing a MainViewModel dependency to ResultsViewModel, updating constructor calls in MainViewModel, and handling preview cases by passing null in specific contexts.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Added MainViewModel dependency and updated UpdateResults to conditionally set visibility |
| Flow.Launcher/ViewModel/MainViewModel.cs | Changed instantiation of ResultsViewModel and added ResultsSelected method |
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs | Adjusted ResultsViewModel creation for preview contexts by passing null |
This comment has been minimized.
This comment has been minimized.
|
@jjw24 I would like to include this small fix PR in 1.20.0 release. |
…w-Launcher/Flow.Launcher into results_context_menu_display
@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 ...
|
What tests have been done? |
|
The context menu is jumped and query input is also updated. And only the context menu is shown. |
jjw24
approved these changes
May 13, 2025
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.
Fix the issue that results and context menu showing at the same time
Check if the results are selected and then set its visibility in
if (_mainVM == null || _mainVM.ResultsSelected(this)). So if we click right arrow and input some characters at the same time, because the selected results are from context menu already, the visibility of the results will not be updated inprivate void UpdateResults(List<ResultViewModel> newResults, bool reselect = true, CancellationToken token = default).Resolve #2026.