Skip to content

Fix possible null reference when query is cancelled & Add new image icon for images whose formats are not support & Other improvements #3099

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 12 commits into from
Dec 7, 2024
Merged

Fix possible null reference when query is cancelled & Add new image icon for images whose formats are not support & Other improvements #3099

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
129169e
Fix possible null reference when query is cancelled
Jack251970 Nov 27, 2024
a8d69c7
Add support when image format is not support
Jack251970 Nov 27, 2024
756e718
Regard format not support files as error type
Jack251970 Nov 27, 2024
0720779
Fix possible null reference when query is cancelled
Jack251970 Nov 27, 2024
b95bbb3
Introduce concurrent directionary for topmost record
Jack251970 Nov 27, 2024
c474b30
Fix possible null reference
Jack251970 Nov 27, 2024
d8c07a5
Fix result update issue during modified collection enumeration
Jack251970 Nov 27, 2024
c3c6126
Fix xaml convert issue and use icon loader for PluginActivationIcon
Jack251970 Nov 27, 2024
c665482
Improve code quality
Jack251970 Dec 2, 2024
4830988
Add image for format not supported images
Jack251970 Dec 2, 2024
3f413d1
Update image for format not supported images
Jack251970 Dec 3, 2024
00e76f6
Revert "Fix result update issue during modified collection enumeration"
Jack251970 Dec 4, 2024
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
12 changes: 10 additions & 2 deletions Flow.Launcher.Infrastructure/Image/ImageLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,16 @@ private static ImageResult GetThumbnailResult(ref string path, bool loadFullImag
type = ImageType.ImageFile;
if (loadFullImage)
{
image = LoadFullImage(path);
type = ImageType.FullImageFile;
try
{
image = LoadFullImage(path);
type = ImageType.FullImageFile;
}
catch (NotSupportedException)
{
image = null;
type = ImageType.Error;
}
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion Flow.Launcher/MainWindow.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@
VerticalAlignment="Center"
Panel.ZIndex="2"
RenderOptions.BitmapScalingMode="HighQuality"
Source="{Binding PluginIconPath}"
Source="{Binding PluginIconSource}"
Stretch="Uniform"
Style="{DynamicResource PluginActivationIcon}" />
<Canvas Style="{DynamicResource SearchIconPosition}">
Expand Down
18 changes: 8 additions & 10 deletions Flow.Launcher/Storage/TopMostRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,29 @@
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Text.Json.Serialization;
using Flow.Launcher.Plugin;

namespace Flow.Launcher.Storage
{
// todo this class is not thread safe.... but used from multiple threads.
public class TopMostRecord
{
[JsonInclude]
public Dictionary<string, Record> records { get; private set; } = new Dictionary<string, Record>();
public ConcurrentDictionary<string, Record> records { get; private set; } = new ConcurrentDictionary<string, Record>();

internal bool IsTopMost(Result result)
{
if (records.Count == 0 || !records.ContainsKey(result.OriginQuery.RawQuery))
if (records.IsEmpty || (result.OriginQuery != null && !records.ContainsKey(result.OriginQuery.RawQuery)))
{
return false;
}

// since this dictionary should be very small (or empty) going over it should be pretty fast.
return records[result.OriginQuery.RawQuery].Equals(result);
return result.OriginQuery != null && records[result.OriginQuery.RawQuery].Equals(result);
}

internal void Remove(Result result)
{
records.Remove(result.OriginQuery.RawQuery);
records.Remove(result.OriginQuery.RawQuery, out _);
}

internal void AddOrUpdate(Result result)
Expand All @@ -34,17 +34,15 @@ internal void AddOrUpdate(Result result)
Title = result.Title,
SubTitle = result.SubTitle
};
records[result.OriginQuery.RawQuery] = record;

records.AddOrUpdate(result.OriginQuery.RawQuery, record, (key, oldValue) => record);
}

public void Load(Dictionary<string, Record> dictionary)
{
records = dictionary;
records = new ConcurrentDictionary<string, Record>(dictionary);
}
}


public class Record
{
public string Title { get; set; }
Expand Down
7 changes: 6 additions & 1 deletion Flow.Launcher/Storage/UserSelectedRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ private static int GenerateResultHashCode(Result result)

private static int GenerateQueryAndResultHashCode(Query query, Result result)
{
if (query == null)
{
return GenerateResultHashCode(result);
}

int hashcode = GenerateStaticHashCode(query.ActionKeyword);
hashcode = GenerateStaticHashCode(query.Search, hashcode);
hashcode = GenerateStaticHashCode(result.Title, hashcode);
Expand Down Expand Up @@ -101,4 +106,4 @@ public int GetSelectedCount(Result result)
return selectedCount;
}
}
}
}
44 changes: 30 additions & 14 deletions Flow.Launcher/ViewModel/MainViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
Expand All @@ -23,6 +23,8 @@
using System.Globalization;
using System.Windows.Input;
using System.ComponentModel;
using Flow.Launcher.Infrastructure.Image;
using System.Windows.Media;

namespace Flow.Launcher.ViewModel
{
Expand Down Expand Up @@ -722,6 +724,8 @@ public double ResultSubItemFontSize
set => Settings.ResultSubItemFontSize = value;
}

public ImageSource PluginIconSource { get; private set; } = null;

public string PluginIconPath { get; set; } = null;

public string OpenResultCommandModifiers => Settings.OpenResultModifiers;
Expand Down Expand Up @@ -1066,6 +1070,7 @@ private async void QueryResults(bool isReQuery = false, bool reSelect = true)
Results.Clear();
Results.Visibility = Visibility.Collapsed;
PluginIconPath = null;
PluginIconSource = null;
SearchIconVisibility = Visibility.Visible;
return;
}
Expand Down Expand Up @@ -1099,11 +1104,13 @@ private async void QueryResults(bool isReQuery = false, bool reSelect = true)
if (plugins.Count == 1)
{
PluginIconPath = plugins.Single().Metadata.IcoPath;
PluginIconSource = await ImageLoader.LoadAsync(PluginIconPath);
SearchIconVisibility = Visibility.Hidden;
}
else
{
PluginIconPath = null;
PluginIconSource = null;
SearchIconVisibility = Visibility.Visible;
}

Expand Down Expand Up @@ -1440,26 +1447,35 @@ public void UpdateResultView(ICollection<ResultsForUpdate> resultsForUpdates)
}
#endif

foreach (var metaResults in resultsForUpdates)
try
Copy link
Member

Choose a reason for hiding this comment

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

could you explain for the try?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could happen when calling ResultsUpdated.Invoke event in IResultUpdate interface so fast that main view model cannot complete update event (still execute foreach sentence).

Take WebSearch plugin for example,

public async Task<List<Result>> QueryAsync(Query query, CancellationToken token)
{
    var results = new List<Result>();

    foreach (...)
    {
        ...

        results.Add(result);  // edit this List

        ...

        // Here you use this List to give to MainViewModel for updating
        // If you do the time-consuming work after this so fast, you will edit this list when updating the MainViewModel
        // So we need to use Try to avoid this problem
        ResultsUpdated?.Invoke(this, new ResultUpdatedEventArgs
        {
            Results = results,
            Query = query
        });

        ... // do time-consuming work
    }

    return results;
}

Copy link
Member

Choose a reason for hiding this comment

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

Make sense, but that also means we need additional synchronization? I remember the ConcurrentDictionary is already providing a enumerator that is thread safe?

Copy link
Member

Choose a reason for hiding this comment

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

How about a ConcurrentBag?

Copy link
Member Author

@Jack251970 Jack251970 Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The key of the problem is that this plugin has not tell the developer to do thread-safe operation in QueryAsync (like return a copy of the list as the Results in ResultUpdatedEventArgs). So the developer will possibly return the same List to the ResultUpdatedEventArgs and the result of the QueryAsync function.

If you want to fully address this problem, you need to add additional synchronization.

For example, you can change the definition of the ResultUpdatedEventArgs to

public class ResultUpdatedEventArgs : EventArgs
{
    public ConcurrentBag<Result> Results;  // thread-safe
    public Query Query;
    public CancellationToken Token { get; init; }
}

Or, you need to ask the developer to return a copy of the List so that the plugin will not change the List for updating the view.

Take WebSearch plugin for example,

public async Task<List<Result>> QueryAsync(Query query, CancellationToken token)
{
    var results = new List<Result>();

    foreach (...)
    {
        ...

        results.Add(result);

        ...

        var resultsCopy = results.Clone();  // create a copy of the results
        ResultsUpdated?.Invoke(this, new ResultUpdatedEventArgs
        {
            Results = resultsCopy,
            Query = query
        });

        ...
    }

    return results;
}

Anyway, I think this is the most developer-friendly solution for this problem and developers don not need to redevelop their plugins to fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@taooceros What do you mean by ConcurrentDictionary and ConcurrentBag?

Copy link
Member

Choose a reason for hiding this comment

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

Hold on, I remember this is a producer/consumer model with appropriate synchronization through the Channel class. Let me recheck.

Copy link
Member

Choose a reason for hiding this comment

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

I am a little bit suspicious of this change. Is it possible for you to revert just this part so we can merge first. Then Include it in a new pr so we can discuss specifically about the potential synchronization problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

@taooceros Sure! Please check!

{
foreach (var result in metaResults.Results)
foreach (var metaResults in resultsForUpdates)
{
if (_topMostRecord.IsTopMost(result))
foreach (var result in metaResults.Results)
{
result.Score = int.MaxValue;
}
else
{
var priorityScore = metaResults.Metadata.Priority * 150;
result.Score += _userSelectedRecord.GetSelectedCount(result) + priorityScore;
if (_topMostRecord.IsTopMost(result))
{
result.Score = int.MaxValue;
}
else
{
var priorityScore = metaResults.Metadata.Priority * 150;
result.Score += _userSelectedRecord.GetSelectedCount(result) + priorityScore;
}
}
}
}

// it should be the same for all results
bool reSelect = resultsForUpdates.First().ReSelectFirstResult;
// it should be the same for all results
bool reSelect = resultsForUpdates.First().ReSelectFirstResult;

Results.AddResults(resultsForUpdates, token, reSelect);
}
catch (Exception ex)
{
Log.Debug("MainViewModel", $"Error in UpdateResultView: {ex.Message}");
}

Results.AddResults(resultsForUpdates, token, reSelect);

}

#endregion
Expand Down
Loading