Skip to content

Fix clearing of results logic & minor adjustment to results update #3524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 10, 2025
Merged

Fix clearing of results logic & minor adjustment to results update #3524

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d5d6c2
fix clearing of results logic & minor adjustment to results update
jjw24 May 10, 2025
8bc497a
fix naming
jjw24 May 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 38 additions & 26 deletions Flow.Launcher/ViewModel/MainViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public partial class MainViewModel : BaseModel, ISavable, IDisposable

private bool _isQueryRunning;
private Query _lastQuery;
private bool _lastIsHomeQuery;
private bool _previousIsHomeQuery;
private string _queryTextBeforeLeaveResults;
private string _ignoredQueryText; // Used to ignore query text change when switching between context menu and query results

Expand Down Expand Up @@ -1264,7 +1264,7 @@ private async Task QueryResultsAsync(bool searchDelay, bool isReQuery = false, b

App.API.LogDebug(ClassName, $"Start query with ActionKeyword <{query.ActionKeyword}> and RawQuery <{query.RawQuery}>");

var isHomeQuery = query.RawQuery == string.Empty;
var currentIsHomeQuery = query.RawQuery == string.Empty;

_updateSource?.Dispose();

Expand All @@ -1284,14 +1284,10 @@ private async Task QueryResultsAsync(bool searchDelay, bool isReQuery = false, b
// Update the query's IsReQuery property to true if this is a re-query
query.IsReQuery = isReQuery;

// handle the exclusiveness of plugin using action keyword
RemoveOldQueryResults(query, isHomeQuery);

_lastQuery = query;
_lastIsHomeQuery = isHomeQuery;


ICollection<PluginPair> plugins = Array.Empty<PluginPair>();
if (isHomeQuery)
if (currentIsHomeQuery)
{
if (Settings.ShowHomePage)
{
Expand Down Expand Up @@ -1347,7 +1343,7 @@ private async Task QueryResultsAsync(bool searchDelay, bool isReQuery = false, b
// plugins are ICollection, meaning LINQ will get the Count and preallocate Array

Task[] tasks;
if (isHomeQuery)
if (currentIsHomeQuery)
{
tasks = plugins.Select(plugin => plugin.Metadata.HomeDisabled switch
{
Expand Down Expand Up @@ -1397,7 +1393,7 @@ async Task QueryTaskAsync(PluginPair plugin, CancellationToken token)
{
App.API.LogDebug(ClassName, $"Wait for querying plugin <{plugin.Metadata.Name}>");

if (searchDelay && !isHomeQuery) // Do not delay for home query
if (searchDelay && !currentIsHomeQuery) // Do not delay for home query
{
var searchDelayTime = plugin.Metadata.SearchDelayTime ?? Settings.SearchDelayTime;

Expand All @@ -1410,7 +1406,7 @@ async Task QueryTaskAsync(PluginPair plugin, CancellationToken token)
// Task.Yield will force it to run in ThreadPool
await Task.Yield();

var results = isHomeQuery ?
var results = currentIsHomeQuery ?
await PluginManager.QueryHomeForPluginAsync(plugin, query, token) :
await PluginManager.QueryForPluginAsync(plugin, query, token);

Expand Down Expand Up @@ -1439,8 +1435,13 @@ await PluginManager.QueryHomeForPluginAsync(plugin, query, token) :

App.API.LogDebug(ClassName, $"Update results for plugin <{plugin.Metadata.Name}>");

// Indicate if to clear existing results so to show only ones from plugins with action keywords
var clearExistingResultsRequired = RequireClearExistingResults(query, currentIsHomeQuery);
_lastQuery = query;
_previousIsHomeQuery = currentIsHomeQuery;

Copy link
Contributor

@coderabbitai coderabbitai bot May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential race condition when updating _lastQuery / _previousIsHomeQuery

QueryTaskAsync runs once per plugin and executes in parallel.
Each invocation updates the shared fields _lastQuery and _previousIsHomeQuery:

_lastQuery          = query;
_previousIsHomeQuery = currentIsHomeQuery;

This means later-finishing plugin tasks from the previous query (or simply slower plugins) can overwrite these fields after a new query has already started, causing RequireClearExistingResults to make a decision based on stale data.

Suggestion

• Move the assignments into the outer `QueryResultsAsync` method *before* launching plugin tasks.
• Or guard them with a dedicated lock / atomic exchange and verify the cancellation token hasn’t been triggered.

Example (outside the tasks):

// before launching Tasks
var clearExistingResultsRequired = RequireClearExistingResults(query, currentIsHomeQuery);
_lastQuery          = query;
_previousIsHomeQuery = currentIsHomeQuery;

tasks = plugins.Select(p => QueryTaskAsync(p, clearExistingResultsRequired, currentCancellationToken)).ToArray();

Then pass clearExistingResultsRequired as a parameter into QueryTaskAsync.
This removes any cross-thread contention and ensures the flag is computed exactly once per user query.

Copy link
Member Author

@jjw24 jjw24 May 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not applicable in this instance because we always want to determine whether to clear results based on the current and previous query type (action keyword, global or home page) that will be push to the channel immediately.

This means later-finishing plugin tasks from the previous query (or simply slower plugins) can overwrite these fields after a new query has already started, causing RequireClearExistingResults to make a decision based on stale data.

That is correct expectation the upcoming query should determine whether results are to be cleared. RequireClearExistingResults should make decision based on the immediate previous and current query that will be pushed through to channel even if they come later due to slowness, i.e. if GitHub search query comes late and is from an action keyword, and the previous query is from global action keyword then results still need to be cleared.

Let's keep this comment open so can refer back to in the future if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjw24 I understand your explanation about the design intention. The behavior you describe makes sense - each plugin task needs to independently evaluate whether to clear results based on the immediate previous and current query state when the results are pushed to the channel, regardless of timing.

This is important for situations like your GitHub example, where a late-arriving query with a different action keyword type still needs to clear existing results appropriately.

I noticed the method is called ShouldClearExistingResults in the code but appears as RequireClearExistingResults in the diff. Is this just a naming difference from development, or is there something else I should be aware of?

(•‿•)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the method is called ShouldClearExistingResults in the code but appears as RequireClearExistingResults in the diff. Is this just a naming difference from development, or is there something else I should be aware of?

You looking at outdated diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@jjw24 You're absolutely right. I was referencing an outdated diff. Thanks for pointing that out - I'll make sure to check the latest code in future reviews.

🏁 Script executed:

#!/bin/bash
# Check the PR diff for the lines containing "ClearExistingResults"
git diff -p origin/master HEAD -- Flow.Launcher/ViewModel/MainViewModel.cs | grep -A 5 -B 5 "ClearExistingResults"

Length of output: 150

@jjw24 You're right - I was looking at an outdated diff. Thank you for the correction. I can now see that the method is consistently named ShouldClearExistingResults in the code, and the variable name in the current code is shouldClearExistingResults. I appreciate you pointing this out, and I'll make sure to reference the most current code in future reviews.

🧬 Code Graph Analysis Results

Relevant snippet from Flow.Launcher/ViewModel/MainViewModel.cs (lines 710-758):

/// <summary>
/// Determines whether the existing search results should be cleared based on the current query and the previous query type.
/// This is needed because of the design that treats plugins with action keywords and global action keywords separately. Results are gathered
/// either from plugins with matching action keywords or global action keyword, but not both. So when the current results are from plugins
/// with matching action keywords and a new result set comes from a new query with the global action keyword, the existing results need to be cleared,
/// and vice versa. The same applies to home page query results.
/// 
/// There is no need to clear results from global action keyword if a new set of results comes along that is also from global action keywords.
/// This is because the removal of obsolete results is handled in ResultsViewModel.NewResults(ICollection<ResultsForUpdate>).
/// </summary>
/// <param name="query">The current query.</param>
/// <param name="currentIsHomeQuery">A flag indicating if the current query is a home query.</param>
/// <returns>True if the existing results should be cleared, false otherwise.</returns>
private bool ShouldClearExistingResults(Query query, bool currentIsHomeQuery)
{
    // If previous or current results are from home query, we need to clear them
    if (_previousIsHomeQuery || currentIsHomeQuery)
    {
        App.API.LogDebug(ClassName, $"Cleared old results");
        return true;
    }

    // If the last and current query are not home query type, we need to check the action keyword
    if (_lastQuery?.ActionKeyword != query?.ActionKeyword)
    {
        App.API.LogDebug(ClassName, $"Cleared old results");
        return true;
    }

    return false;
}

Relevant snippet from Flow.Launcher/ViewModel/MainViewModel.cs (lines 600-720):

This snippet shows the usage of the method ShouldClearExistingResults inside the QueryResultsAsync method:

private async Task QueryResultsAsync(bool searchDelay, bool isReQuery = false, bool reSelect = true)
{
    _updateSource?.Cancel();

    App.API.LogDebug(ClassName, $"Start query with text: <{QueryText}>");

    var query = await ConstructQueryAsync(QueryText, Settings.CustomShortcuts, Settings.BuiltinShortcuts);

    if (query == null) // shortcut expanded
    {
        App.API.LogDebug(ClassName, $"Clear query results");

        // Hide and clear results again because running query may show and add some results
        Results.Visibility = Visibility.Collapsed;
        Results.Clear();

        // Reset plugin icon
        PluginIconPath = null;
        PluginIconSource = null;
        SearchIconVisibility = Visibility.Visible;

        // Hide progress bar again because running query may set this to visible
        ProgressBarVisibility = Visibility.Hidden;
        return;
    }

    App.API.LogDebug(ClassName, $"Start query with ActionKeyword <{query.ActionKeyword}> and RawQuery <{query.RawQuery}>");

    var currentIsHomeQuery = query.RawQuery == string.Empty;

    _updateSource?.Dispose();

    var currentUpdateSource = new CancellationTokenSource();
    _updateSource = currentUpdateSource;
    var currentCancellationToken = _updateSource.Token;
    _updateToken = currentCancellationToken;

    ProgressBarVisibility = Visibility.Hidden;
    _isQueryRunning = true;

    // Switch to ThreadPool thread
    await TaskScheduler.Default;

    if (currentCancellationToken.IsCancellationRequested) return;

    // Update the query's IsReQuery property to true if this is a re-query
    query.IsReQuery = isReQuery;

    ICollection<PluginPair> plugins = Array.Empty<PluginPair>();
    if (currentIsHomeQuery)
    {
        if (Settings.ShowHomePage)
        {
            plugins = PluginManager.ValidPluginsForHomeQuery();
        }

        PluginIconPath = null;
        PluginIconSource = null;
        SearchIconVisibility = Visibility.Visible;
    }
    else
    {
        plugins = PluginManager.ValidPluginsForQuery(query);

        if (plugins.Count == 1)
        {
            PluginIconPath = plugins.Single().Metadata.IcoPath;
            PluginIconSource = await App.API.LoadImageAsync(PluginIconPath);
            SearchIconVisibility = Visibility.Hidden;
        }
        else
        {
            PluginIconPath = null;
            PluginIconSource = null;
            SearchIconVisibility = Visibility.Visible;
        }
    }

    var validPluginNames = plugins.Select(x => $"<{x.Metadata.Name}>");
    App.API.LogDebug(ClassName, $"Valid <{plugins.Count}> plugins: {string.Join(" ", validPluginNames)}");

    _ = Task.Delay(200, currentCancellationToken).ContinueWith(_ =>
        {
            // start the progress bar if query takes more than 200 ms and this is the current running query and it didn't finish yet
            if (_isQueryRunning)
            {
                ProgressBarVisibility = Visibility.Visible;
            }
        },
        currentCancellationToken,
        TaskContinuationOptions.NotOnCanceled,
        TaskScheduler.Default);

    Task[] tasks;
    if (currentIsHomeQuery)
    {
        tasks = plugins.Select(plugin => plugin.Metadata.HomeDisabled switch
        {
            false => QueryTaskAsync(plugin, currentCancellationToken),
            true => Task.CompletedTask
        }).ToArray();

        // Query history results for home page firstly so it will be put on top of the results
        if (Settings.ShowHistoryResultsForHomePage)
        {
            QueryHistoryTask(currentCancellationToken);
        }
    }
    else
    {
        tasks = plugins.Select(plugin => plugin.Metadata.Disabled switch
        {
            false => QueryTaskAsync(plugin, currentCancellationToken),
            true => Task.CompletedTask
        }).ToArray();
    }

    try
    {
        await Task.WhenAll(tasks);
    }
    catch (OperationCanceledException)
    {
        // nothing to do here
    }

    if (currentCancellationToken.IsCancellationRequested) return;

    _isQueryRunning = false;

    if (!currentCancellationToken.IsCancellationRequested)
    {
        ProgressBarVisibility = Visibility.Hidden;
    }

    // Local function
    async Task QueryTaskAsync(PluginPair plugin, CancellationToken token)
    {
        App.API.LogDebug(ClassName, $"Wait for querying plugin <{plugin.Metadata.Name}>");

        if (searchDelay && !currentIsHomeQuery) // Do not delay for home query
        {
            var searchDelayTime = plugin.Metadata.SearchDelayTime ?? Settings.SearchDelayTime;

            await Task.Delay(searchDelayTime, token);

            if (token.IsCancellationRequested) return;
        }

        await Task.Yield();

        var results = currentIsHomeQuery ?
            await PluginManager.QueryHomeForPluginAsync(plugin, query, token) :
            await PluginManager.QueryForPluginAsync(plugin, query, token);

        if (token.IsCancellationRequested) return;

        IReadOnlyList<Result> resultsCopy;
        if (results == null)
        {
            resultsCopy = _emptyResult;
        }
        else
        {
            resultsCopy = DeepCloneResults(results, token);
        }

        foreach (var result in resultsCopy)
        {
            if (string.IsNullOrEmpty(result.BadgeIcoPath))
            {
                result.BadgeIcoPath = plugin.Metadata.IcoPath;
            }
        }

        if (token.IsCancellationRequested) return;

        App.API.LogDebug(ClassName, $"Update results for plugin <{plugin.Metadata.Name}>");

        var shouldClearExistingResults = ShouldClearExistingResults(query, currentIsHomeQuery);
        _lastQuery = query;
        _previousIsHomeQuery = currentIsHomeQuery;

        if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(resultsCopy, plugin.Metadata, query,
            token, reSelect, shouldClearExistingResults)))
        {
            App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
        }
    }

    void QueryHistoryTask(CancellationToken token)
    {
        var historyItems = _history.Items.TakeLast(Settings.MaxHistoryResultsToShowForHomePage).Reverse();

        var results = GetHistoryItems(historyItems);

        if (token.IsCancellationRequested) return;

        App.API.LogDebug(ClassName, $"Update results for history");

        if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(results, _historyMetadata, query,
            token)))
        {
            App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
        }
    }
}

Summary of ShouldClearExistingResults method in MainViewModel.cs:

  • Purpose:
    Determines if existing search results should be cleared when a new query is executed. This is necessary because results from plugins with action keywords and global plugins are handled separately, and mixing them requires clearing old results to avoid stale or irrelevant data.

  • Parameters:

    • Query query: The current query object containing the action keyword and raw query.
    • bool currentIsHomeQuery: Indicates if the current query is a home query (empty raw query).

  • Returns:

    • bool: Returns true if existing results should be cleared, otherwise false.

  • Logic:

    • Clears results if either the previous or current query is a home query.
    • Clears results if the action keyword of the last query differs from the current query's action keyword.
    • Otherwise, does not clear results.

  • Usage:
    Called during the query execution process (QueryResultsAsync) to decide whether to clear existing results before adding new ones.

Additional context:

  • The method ShouldClearExistingResults is used internally in QueryResultsAsync to manage the results update queue.
  • The naming difference mentioned in the user comment (ShouldClearExistingResults vs RequireClearExistingResults) appears to be a naming difference in the diff or code review context; the actual method in the current code is named ShouldClearExistingResults.
  • The method is private and tightly coupled with the query execution and results update logic in MainViewModel.

No other code snippets from the provided list are directly relevant to the naming or behavior of ShouldClearExistingResults or RequireClearExistingResults in MainViewModel.cs.

if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(resultsCopy, plugin.Metadata, query,
token, reSelect)))
token, reSelect, clearExistingResultsRequired)))
{
App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
}
Expand Down Expand Up @@ -1542,25 +1543,36 @@ private async Task BuildQueryAsync(IEnumerable<BaseBuiltinShortcutModel> builtIn
}
}

private void RemoveOldQueryResults(Query query, bool isHomeQuery)
/// <summary>
/// Determines whether the existing search results should be cleared based on the current query and the previous query type.
/// This is needed because of the design that treats plugins with action keywords and global action keywords separately. Results are gathered
/// either from plugins with matching action keywords or global action keyword, but not both. So when the current results are from plugins
/// with a matching action keyword and a new result set comes from a new query with the global action keyword, the existing results need to be cleared,
/// and vice versa. The same applies to home page query results.
///
/// There is no need to clear results from global action keyword if a new set of results comes along that is also from global action keywords.
/// This is because the removal of obsolete results is handled in ResultsViewModel.NewResults(ICollection<ResultsForUpdate>).
/// </summary>
/// <param name="query">The current query.</param>
/// <param name="currentIsHomeQuery">A flag indicating if the current query is a home query.</param>
/// <returns>True if the existing results should be cleared, false otherwise.</returns>
private bool RequireClearExistingResults(Query query, bool currentIsHomeQuery)
{
// If last and current query are home query, we don't need to clear the results
if (_lastIsHomeQuery && isHomeQuery)
// If previous or current results are from home query, we need to clear them
if (_previousIsHomeQuery || currentIsHomeQuery)
{
return;
App.API.LogDebug(ClassName, $"Cleared old results");
return true;
}
// If last or current query is home query, we need to clear the results
else if (_lastIsHomeQuery || isHomeQuery)
{
App.API.LogDebug(ClassName, $"Remove old results");
Results.Clear();
}
// If last and current query are not home query, we need to check action keyword
else if (_lastQuery?.ActionKeyword != query?.ActionKeyword)

// If the last and current query are not home query type, we need to check the action keyword
if (_lastQuery?.ActionKeyword != query?.ActionKeyword)
{
App.API.LogDebug(ClassName, $"Remove old results");
Results.Clear();
App.API.LogDebug(ClassName, $"Cleared old results");
return true;
}

return false;
}

private Result ContextMenuTopMost(Result result)
Expand Down
3 changes: 2 additions & 1 deletion Flow.Launcher/ViewModel/ResultsForUpdate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ public record struct ResultsForUpdate(
PluginMetadata Metadata,
Query Query,
CancellationToken Token,
bool ReSelectFirstResult = true)
bool ReSelectFirstResult = true,
bool requireClearExistingResults = false)
{
public string ID { get; } = Metadata.ID;
}
Expand Down
11 changes: 8 additions & 3 deletions Flow.Launcher/ViewModel/ResultsViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,15 @@ private List<ResultViewModel> NewResults(ICollection<ResultsForUpdate> resultsFo
if (!resultsForUpdates.Any())
return Results;

var newResults = resultsForUpdates.SelectMany(u => u.Results, (u, r) => new ResultViewModel(r, _settings));

if (resultsForUpdates.Any(x => x.requireClearExistingResults))
return newResults.OrderByDescending(rv => rv.Result.Score).ToList();

return Results.Where(r => r?.Result != null && resultsForUpdates.All(u => u.ID != r.Result.PluginID))
.Concat(resultsForUpdates.SelectMany(u => u.Results, (u, r) => new ResultViewModel(r, _settings)))
.OrderByDescending(rv => rv.Result.Score)
.ToList();
.Concat(newResults)
.OrderByDescending(rv => rv.Result.Score)
.ToList();
}
#endregion

Expand Down
Loading