Fix history results clear logic & Make sure results cleared #3525
+54
−17
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
There was a problem hiding this comment.
Pull Request Overview
This PR aims to fix the history results clear logic by checking the shouldClearExistingResults condition when querying history results. Key changes include:
- Removing redundant blank lines for cleaner formatting.
- Introducing a new variable, shouldClearExistingResults, to determine if existing results should be cleared.
- Updating the _resultsUpdateChannelWriter.TryWrite call to pass the additional parameters including token, reSelect, and shouldClearExistingResults.
Comments suppressed due to low confidence (2)
Flow.Launcher/ViewModel/MainViewModel.cs:1460
- Ensure that the new parameter 'shouldClearExistingResults' is correctly positioned in the _resultsUpdateChannelWriter.TryWrite call and that the method signature has been updated accordingly.
var shouldClearExistingResults = ShouldClearExistingResults(query, currentIsHomeQuery);
Flow.Launcher/ViewModel/MainViewModel.cs:1460
- [nitpick] Consider refining the comment to explicitly state that the flag determines whether to clear existing results to display only plugin-specific action keywords.
var shouldClearExistingResults = ShouldClearExistingResults(query, currentIsHomeQuery);
|
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.
📝 Walkthrough## Walkthrough
This change introduces a private boolean `_needClearResults` in `MainViewModel` to defer clearing search results when queries are cancelled rapidly, preventing flicker. It updates `QueryResultsAsync` to set this flag on cancellation and clear results later. Additionally, `OnLoaded` in `MainWindow.xaml.cs` was changed from asynchronous to synchronous.
## Changes
| File(s) | Change Summary |
|-------------------------------------------|-----------------------------------------------------------------------------------------------|
| Flow.Launcher/ViewModel/MainViewModel.cs | Added `_needClearResults` flag to defer clearing results on cancellation; updated `QueryResultsAsync` to set this flag and conditionally clear results after plugin queries; modified `ShouldClearExistingResults` to clear results if deferred. |
| Flow.Launcher/MainWindow.xaml.cs | Changed `OnLoaded` method signature from `async void` to synchronous `void` without other logic changes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant MainViewModel
participant ResultsUpdateChannel
User->>MainViewModel: Call QueryResultsAsync(query)
MainViewModel->>MainViewModel: Initialize resultsCleared = false
MainViewModel->>MainViewModel: Launch plugin query tasks
loop For each plugin query task
MainViewModel->>MainViewModel: Set _lastQuery, _previousIsHomeQuery
MainViewModel->>MainViewModel: resultsCleared = true
MainViewModel->>ResultsUpdateChannel: Write results
end
MainViewModel->>MainViewModel: Await all plugin tasks
alt Cancellation requested
MainViewModel->>MainViewModel: Set _needClearResults = true
MainViewModel-->>User: Return early without clearing results
else No cancellation and no resultsCleared
MainViewModel->>MainViewModel: Clear Results immediately
end
MainViewModel->>MainViewModel: On next update, ShouldClearExistingResults clears if _needClearResults truePossibly related PRs
Suggested labels
Suggested reviewers
Poem
|
This comment has been minimized.
This comment has been minimized.
prlabeler
bot
added
bug
Jack251970
changed the title
jjw24
reviewed
May 11, 2025
jjw24
reviewed
May 11, 2025
| @@ -1445,6 +1447,11 @@ await PluginManager.QueryHomeForPluginAsync(plugin, query, token) : | |||
| { | |||
| App.API.LogError(ClassName, "Unable to add item to Result Update Queue"); | |||
| } | |||
| else | |||
| { | |||
| // Only update the clear flag when we successfully write to the channel | |||
There was a problem hiding this comment.
Do we actually get failure writes?
There was a problem hiding this comment.
I think we should still take that into consideration.
There was a problem hiding this comment.
Hmm we probably should take into consideration.
There was a problem hiding this comment.
But that will cause issue as below
jjw24
reviewed
May 11, 2025
| @@ -1376,6 +1375,9 @@ private async Task QueryResultsAsync(bool searchDelay, bool isReQuery = false, b | |||
| // nothing to do here | |||
| } | |||
|
|
|||
| // If results are not cleared, we need to clear the results | |||
| if (!resultsCleared) Results.Clear(); | |||
|
|
|||
There was a problem hiding this comment.
Is this really necessary? This could cause unexplained/unexpected flicker
There was a problem hiding this comment.
We must need it. This is used to make sure results are cleared when there are not any update tasks.
There was a problem hiding this comment.
Think about one situation. Home page feature is off and you just change query text from e to empty. You will find the results will not be cleared because there are not any update tasks. So we should force call Results.Clear() here when it is not be called in QueryResult or QueryHistory functions.
There was a problem hiding this comment.
When you say not any update task are you referring to one example where you toggle off home page in settings and results are not cleared?
There was a problem hiding this comment.
Let me give you one demo video
There was a problem hiding this comment.
@jjw24 It seems that I have issues when uploading video here. Please see more in Discord.
jjw24
removed
bug
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
prlabeler
bot
added
bug
Jack251970
removed
bug
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: onesounds 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: To learn more about /:\ gitStream - Visit our Docs |
|
Fixed flickering issue when typing very fast |
|
I haven't had time to sit down and think about this. Will find some in the upcoming days. |
jjw24
added
review in progress
Get it. |
prlabeler
bot
added
bug
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
I do not know why, but sometimes if I input whitespace, the results are not cleared. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow on with #3524 to fix two issues in that PR.
1. Clear existing results for history results query function
We also need to check
shouldClearExistingResultswhen querying history results.2. Make sure results cleared when update tasks are not called
When update tasks are not called, we need to clear results manually afterwards. This can fix an issue that results are not cleared when Home Page feature is off, and
QueryTaskAsyncandQueryHistoryTaskare not called.