-
-
Notifications
You must be signed in to change notification settings - Fork 550
History results display actual result icon for Last Opened history style #4057
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.
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
Conversation
|
@Jack251970 @01Dri thoughts on showing the last opened with the actual result icon? Should it just show the icon instead of badge icon as well? |
|
Also, there is a bug in the comparison logic when saving history results because as seen in the screenshot 'Save Settings' result shouldn't be duplicated. |
It’s not really a bug. The equality comparison also uses the query, so in scenarios where I type “Ter” and open Terminal, and then type “Term” and open Terminal again, I end up with two history items “Ter” and “Term” both referring to the same application (Terminal), which causes duplicate results. In Query style, that makes sense because the two queries are different. CompositeKey (This name is just an example; it could be something else) |
I think the icon with the badge is more intuitive for the user, as it indicates that the item is a history entry. |
As 01Dri said, it is not a bug. Duplicated items will occur if you click A for many times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Showing icons enhance users' experience and I agree with this change. But there is still something we need to resolve
What about just use the actual results icons, no badge?
I think we should apply filtering of duplicates when last opened style is selected. What do you think? |
@jjw24 I think we can do the filtering here as long as we keep the history in the order of execute time so that we can utilize the history list more effectively. |
|
@Jack251970 @jjw24 Sorry for the question, I’m new to the open source community. My question is: am I allowed to make the requested changes, or is this PR only for jjw? Thanks! |
You can create PR to last_history_show_result_icon branch |
Ok thanks! |
|
@01Dri just to add, of course you can, just let the PR author know what/which area you are changing to avoid overlapping. If very minor like comments, typos or small corrections to the logic that you are sure won't overlap with what the author is doing then go ahead, otherwise its good to let the PR author know you intend to help out with certain areas of the PR. Having said that, I am still making changes to this draft but help is very welcomed still. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Flow.Launcher/Storage/QueryHistory.cs`:
- Around line 80-82: The comparison accessing existingHistoryItem.Glyph.Glyph
and .FontFamily can throw when either Glyph is null; update the condition in
QueryHistory.cs to perform null-safe comparisons (e.g., compare
existingHistoryItem.Glyph and result.Glyph for null first, then compare
Glyph.Glyph and Glyph.FontFamily only when non-null) so that you assign
existingHistoryItem.Glyph = result.Glyph when either Glyph is null or any of the
two properties differ; reference the existingHistoryItem.Glyph, result.Glyph,
Glyph.Glyph and Glyph.FontFamily symbols when making the change.
🧹 Nitpick comments (1)
Flow.Launcher/Storage/QueryHistory.cs (1)
7-7: Remove unusedusing Svg;directive.This namespace isn't referenced anywhere in the file.
🔧 Proposed fix
-using Svg;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Flow.Launcher/ViewModel/MainViewModel.cs`:
- Around line 432-449: The early return based solely on
SelectedResults.SelectedItem?.Result?.PluginID blocks built-in context-menu
items without PluginID from returning to Results; adjust the guard so it only
prevents action when in query-result context: remove or change the top-level
PluginID check and instead check PluginID only inside the QueryResultsSelected()
branch (e.g., if (QueryResultsSelected() &&
string.IsNullOrEmpty(SelectedResults.SelectedItem?.Result?.PluginID)) return) so
that built-in ContextMenu items (ContextMenu, Results, PreviewSelectedItem,
UpdatePreviewAsync()) can still fall through to the else branch and restore
Results/preview navigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/Storage/QueryHistory.cs (1)
65-88: Trim by least-recent execution, not list index.Because existing items are now updated in place (not necessarily the last entry),
RemoveAt(0)can evict a recently executed item once the cap is exceeded. Consider trimming after update/add and remove the oldest byExecutedDateTime(or move the updated item to the end).🐛 Proposed fix (trim by oldest execution time)
- // Maintain the max history limit - if (LastOpenedHistoryItems.Count > _maxHistory) - { - LastOpenedHistoryItems.RemoveAt(0); - } - // If the last item is the same as the current result, just update the timestamp and the icon path if (LastOpenedHistoryItems.Count > 0 && TryGetLastOpenedHistoryResult(result, out var existingHistoryItem)) { existingHistoryItem.ExecutedDateTime = DateTime.Now; @@ else { LastOpenedHistoryItems.Add(new LastOpenedHistoryResult(result)); } + + // Maintain the max history limit (remove least-recently executed item) + if (LastOpenedHistoryItems.Count > _maxHistory) + { + var oldest = LastOpenedHistoryItems + .OrderBy(x => x.ExecutedDateTime) + .First(); + LastOpenedHistoryItems.Remove(oldest); + }
|
@Jack251970 should be good to go, please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we upgrade Flow from old version to new one, the icons of those old results are blank.
The updated history.json contains null icon path, but the missing icon is not shown.
{
"Query": "steam",
"ExecutedDateTime": "2026-01-15T16:16:30.9760574+08:00",
"Title": "Steam",
"SubTitle": "",
"ActionKeywordAssigned": null,
"CopyText": "",
"AutoCompleteText": null,
"IcoPath": null,
"IcoPathAbsolute": null,
"BadgeIcoPath": null,
"RoundedIcon": false,
...
},
|
I can't reproduce the issue, the missing image icon shows up for me. Maybe:
|
It seems that this problem no longer appears in the latest commit. Let us forget it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Flow.Launcher.Plugin/Result.cs`:
- Around line 71-72: The XML doc comment for the Result.IcoPath property uses
invalid syntax "<IcoPathAbsolute/>"; update the remark to use a proper XML doc
cref by replacing that token with <see cref="IcoPathAbsolute"/> so the
documentation generates a proper reference to the IcoPathAbsolute member.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Jack251970
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it works well for me. And please do the final check if necessary
Context:
Display icon on last history results
Follows on with #4042
Changes:
Before

After

Tested