-
-
Notifications
You must be signed in to change notification settings - Fork 448
Fix ChangeQuery & ReQuery Work Issue When Results Are Not From Query Results #3240
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
Fix ChangeQuery & ReQuery Work Issue When Results Are Not From Query Results #3240
Conversation
This comment has been minimized.
This comment has been minimized.
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request introduces multiple minor adjustments across the codebase. The changes include a removed blank line in a view model, simplification of query re-entry logic by eliminating redundant condition checks and calls, adjustments to method visibility, and updates to documentation comments in the public API interface. Additionally, conditional URL modifications have been added for GitHub repositories in the context menu, and asynchronous call patterns have been clarified in the Shell plugin. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MV as MainViewModel
participant API as PublicAPI
U->>MV: Trigger ReQuery(reselect)
MV->>MV: Call BackToQueryResults()
MV->>MV: Execute QueryResults(isReQuery: true, reSelect: reselect)
MV->>API: Initiate re-query process
API-->>MV: Return query results
sequenceDiagram
participant CM as ContextMenu
participant PM as PluginManifest
CM->>PM: Retrieve UrlSourceCode
alt URL starts with "https://github.com"
PM-->>CM: Provide GitHub URL (with "/tree/..." segment)
CM->>CM: Modify URL → Remove trailing segment and append "/issues"
else
PM-->>CM: Provide original URL
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
What would be the current behavior of change query under context menu? Directly change the query text under the context menu search text? |
As described in #3087, this is used to make sure FL will be back to query results so that As for the current behavior, 2025-02-17.14-45-11.mp4 |
I feel like ChangeQuery now simply behave weird when not at result. Should we fix that behavior (like make sure it change to Results before changing query or something)? |
From my perspective, I believe we do not need to fix that behavior because, in most scenarios, |
@jjw24 Hello, what are your thoughts on the API function |
I think if change query works fluently on context menu (i.e. change the query text for context menu) then we can skip that. If it will make the behavior of flow weird then we should fix the change query directly. Backtoqueryresult can have other usage. |
Well, I will do that. |
This comment has been minimized.
This comment has been minimized.
I will take a look on how changequery work. We can proceed this first. |
[RelayCommand] | ||
private void OpenDeletePluginWindow() | ||
{ | ||
PluginManager.API.BackToQueryResults(); |
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.
Should we make it consistent to App.API? Similar to the following.
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.
I use PluginManager.API here because OpenDeletePluginWindow use it to get the api instance.
This comment has been minimized.
This comment has been minimized.
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
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)
381-381
: Consider handling task exceptions.The task result is explicitly discarded with
_
, but unhandled exceptions in the task could be silently swallowed. Consider wrapping the task body with try-catch or using proper error handling.- _ = Task.Run(() => + _ = Task.Run(async () => + { + try + { + context.API.ShowMainWindow(); + context.API.BackToQueryResults(); + context.API.ChangeQuery($"{context.CurrentPluginMetadata.ActionKeywords[0]}{Plugin.Query.TermSeparator}"); + } + catch (Exception ex) + { + Log.Exception("Error in OnWinRPressed", ex); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Flow.Launcher/CustomQueryHotkeySetting.xaml.cs
(1 hunks)Flow.Launcher/CustomShortcutSetting.xaml.cs
(1 hunks)Flow.Launcher/ResultListBox.xaml.cs
(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs
(2 hunks)Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
(1 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs
(1 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/ContextMenu.cs
(0 hunks)Plugins/Flow.Launcher.Plugin.Shell/Main.cs
(1 hunks)
💤 Files with no reviewable changes (1)
- Plugins/Flow.Launcher.Plugin.PluginsManager/ContextMenu.cs
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/ViewModel/MainViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- Flow.Launcher/ViewModel/PluginStoreItemViewModel.cs
- Flow.Launcher/CustomQueryHotkeySetting.xaml.cs
- Flow.Launcher/CustomShortcutSetting.xaml.cs
- Flow.Launcher/ResultListBox.xaml.cs
- Flow.Launcher/ViewModel/PluginViewModel.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)
383-386
: LGTM! Good addition of BackToQueryResults.The addition of
BackToQueryResults()
beforeChangeQuery
ensures consistent behavior when items are selected from the context menu, aligning with the PR objectives.
🥷 Code experts: jjw24 Jack251970 has 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:
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 |
Both of you are correct. I would think though have change query and requery methods call it under the hood just makes it a little easier and more convenient for plugin devs when they use those two methods. Sorry I missed this part during the PR 3087 review. |
This comment has been minimized.
This comment has been minimized.
@taooceros Following jjw24's comments, now I remove #3087. When we call |
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
🔭 Outside diff range comments (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
1-334
: 💡 Verification agent🧩 Analysis chain
⚠️ Breaking Change: Removal ofBackToQueryResults
method.The removal of the
BackToQueryResults
method from the public API is a breaking change that could affect existing plugins. While this change aligns with the PR objectives by integrating the functionality intoChangeQuery
andReQuery
, existing plugins may need to be updated.Let's check for any existing usage of this method in plugins:
🏁 Script executed:
#!/bin/bash # Search for any usage of BackToQueryResults in plugins rg -i "BackToQueryResults" --type csLength of output: 273
Breaking Change: Removal of
BackToQueryResults
Method Requires Internal UpdatesThe removal of
BackToQueryResults
from the public API is intentional in favor of integrating its functionality intoChangeQuery
andReQuery
. Our search did not reveal any external plugins using this method; however, we did find internal references inFlow.Launcher/ViewModel/MainViewModel.cs
(three occurrences). Please update these internal calls to utilize the new query management logic to ensure consistency with the PR objectives.
- In
Flow.Launcher/ViewModel/MainViewModel.cs
, replace calls toBackToQueryResults()
with the appropriate new method (ChangeQuery
orReQuery
).
🧹 Nitpick comments (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
20-21
: Consider clarifying the documentation.The documentation update explains the behavior well, but consider rephrasing to focus on the outcome rather than the implementation detail:
- /// When current results is from context menu or history, we will back to query results before changing query. + /// When current results are from context menu or history, the query will be changed from the original query results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs
(0 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
(2 hunks)Flow.Launcher/PublicAPIInstance.cs
(0 hunks)Flow.Launcher/ViewModel/MainViewModel.cs
(3 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs
(0 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs
(0 hunks)Plugins/Flow.Launcher.Plugin.Shell/Main.cs
(1 hunks)
💤 Files with no reviewable changes (4)
- Flow.Launcher/ViewModel/PluginViewModel.cs
- Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs
- Plugins/Flow.Launcher.Plugin.Program/Main.cs
- Flow.Launcher/PublicAPIInstance.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Plugins/Flow.Launcher.Plugin.Shell/Main.cs
- Flow.Launcher/ViewModel/MainViewModel.cs
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3087
File: Flow.Launcher/PublicAPIInstance.cs:321-322
Timestamp: 2024-11-23T09:16:05.648Z
Learning: In this codebase, documentation comments are not required in `PublicAPIInstance.cs` for methods like `BackToQueryResults()` if the same method is already documented in `IPublicAPI.cs`.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
303-303
: LGTM!The documentation update maintains consistency with the
ChangeQuery
method's behavior.
Sorry, I don't mean remove the back to query result API though, just have requery and change query call it before the rest of the code. |
Oh, sorry for misunderstanding. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@jjw24 Ready for review.💪 |
Co-authored-by: Jeremy Wu <[email protected]>
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. Pattern suggestions ✂️ (1)You could add these patterns to
If the flagged items are 🤯 false positivesIf items relate to a ...
|
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.
👍
Fix ChangeQuery & ReQuery Work Issue When Results Are Not From Query Results
The
ChangeQuery
andReQuery
function fail to work when results are from the context menu or history. Now we will back to query results when we call these functions.Test
ChangeQuery
when results are from the context menu or history.ReQuery
when results are from the context menu or history.