Add New API to Force Web Url #3616
Conversation
prlabeler
bot
added
bug
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. |
📝 Walkthrough""" WalkthroughTwo new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebSearchPlugin
participant PublicAPIInstance
participant Browser
User->>WebSearchPlugin: Triggers search or suggestion action
WebSearchPlugin->>PublicAPIInstance: OpenWebUrl(url)
PublicAPIInstance->>Browser: Opens url in configured browser (including file://)
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
Flow.Launcher/PublicAPIInstance.cs (2)
OpenWebUrl(422-425)OpenWebUrl(427-430)
Flow.Launcher/PublicAPIInstance.cs (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
OpenWebUrl(312-312)OpenWebUrl(319-319)
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
[warning] 210-210: Spell check warning: spefic is not a recognized word. (unrecognized-spelling)
[warning] 364-364: Spell check warning: requerying is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs
[warning] 154-154: Spell check warning: copyurl is not a recognized word. (unrecognized-spelling)
Flow.Launcher/PublicAPIInstance.cs
[warning] 52-52: Spell check warning: Ioc is not a recognized word. (unrecognized-spelling)
[warning] 150-150: Spell check warning: VSTHRD is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
308-319: LGTM! Well-designed API addition.The new
OpenWebUrlmethods provide a clear and useful distinction from the existingOpenUrlmethods. The documentation clearly explains that these methods will force URLs to open in the configured browser, even for local file URLs, which addresses the PR objective perfectly.The method signatures are consistent with existing patterns and the positioning in the interface is logical.
Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs (2)
74-74: Perfect adoption of the new API.Correctly replaces
OpenUrlwithOpenWebUrlfor opening search result URLs. This ensures that search URLs (including local file URLs) will open in the configured browser as intended.
138-138: Correct usage of OpenWebUrl for suggestions.Properly adopts the new
OpenWebUrlmethod for opening suggestion URLs, maintaining consistency with the search result behavior and ensuring proper browser handling.Flow.Launcher/PublicAPIInstance.cs (1)
422-430: Implementation logic is correct despite the typo.The implementation correctly calls
OpenUriwith the force browser flag set totrue, which will ensure URLs open in the configured browser. The logic flow and method signatures are well-designed.Once the typo is fixed in the
OpenUrimethod, these implementations will work perfectly.
This comment has been minimized.
This comment has been minimized.
Flow.Launcher/PublicAPIInstance.cs
Outdated
| @@ -390,10 +390,9 @@ public void OpenDirectory(string directoryPath, string fileNameOrFilePath = null | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| private void OpenUri(Uri uri, bool? inPrivate = null) | |||
| private void OpenUri(Uri uri, bool? inPrivate = null, bool forceBrower = false) | |||
Flow.Launcher/PublicAPIInstance.cs
Outdated
| { | ||
| if (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps) | ||
| if (forceBrower || uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps) |
| /// Opens the URL using the browser with the given Uri object, even if the URL is a local file. | ||
| /// The browser and mode used is based on what's configured in Flow's default browser settings. | ||
| /// </summary> | ||
| public void OpenWebUrl(Uri url, bool? inPrivate = null); |
There was a problem hiding this comment.
Why do we need these two in the API, they seem to be the same as the two below
There was a problem hiding this comment.
Please see more information in description
There was a problem hiding this comment.
Can you update OpenUri's method and it's summary with the new parameter
There was a problem hiding this comment.
I do not think this is a good idea. I prefer to add a new API function to ensure compatibility.
| /// The browser and mode used is based on what's configured in Flow's default browser settings. | ||
| /// Non-C# plugins should use this method. | ||
| /// </summary> | ||
| public void OpenWebUrl(string url, bool? inPrivate = null); |
There was a problem hiding this comment.
This is the same as OpenUrl with string uri parameter, need to differentiate these two. can you mention something like 'Opens the URL using the browser with the given string, even if the URL is a local file, by setting forceBrowser to true on OpenUrl method.'
There was a problem hiding this comment.
I think I have already described this in function summaries.
Opens the URL using the browser with the given Uri object, even if the URL is a local file.
Could you please explain your meaning further?
There was a problem hiding this comment.
And I have no idea about why we need to add forceBrowser. This is the parameter of a private function which should be invisible to developers.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new API to force URLs (including file:// schemes) to open in the web browser rather than be treated as local files.
- Switches WebSearch plugin from
OpenUrltoOpenWebUrlfor query and suggestion actions. - Extends
PublicAPIInstancewith aforceBrowserflag and twoOpenWebUrloverloads. - Updates
IPublicAPIto declare the newOpenWebUrlmethods.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs | Replaced OpenUrl calls with OpenWebUrl to force use of browser. |
| Flow.Launcher/PublicAPIInstance.cs | Added forceBrowser parameter to OpenUri and implemented OpenWebUrl overloads. |
| Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs | Declared new OpenWebUrl methods in the public plugin interface. |
Comments suppressed due to low confidence (4)
Flow.Launcher/PublicAPIInstance.cs:393
- The new
forceBrowserparameter lacks XML documentation. Consider adding a<param name="forceBrowser">tag in the method's XML comments to explain its purpose.
private void OpenUri(Uri uri, bool? inPrivate = null, bool forceBrowser = false)
Flow.Launcher/PublicAPIInstance.cs:422
- These
OpenWebUrloverloads are missing XML doc comments. Add<summary>and<param>tags to document what each overload does and the meaning ofinPrivate.
public void OpenWebUrl(string url, bool? inPrivate = null)
Plugins/Flow.Launcher.Plugin.WebSearch/Main.cs:74
- [nitpick] Consider adding a version check or fallback for
OpenWebUrlin case the host API hasn't been updated yet, to avoid runtime errors in older versions.
_context.API.OpenWebUrl(searchSource.Url.Replace("{q}", Uri.EscapeDataString(keyword)));
Flow.Launcher/PublicAPIInstance.cs:422
- There are no unit tests covering the new
OpenWebUrloverloads. Add tests to verify that URLs (includingfile://schemes) are correctly passed through to the default browser.
public void OpenWebUrl(string url, bool? inPrivate = null)
jjw24
added
the
review in progress
Context
In WebSearch plugin, users may input url like
file:///D:/search/index.html?query={q}which is regarded as files url in C# standard process. So we need to a new api to force using web brower to open it.Resolve #3608.
Test
file://prefix like "file:///D:/search/index.html?query={q}"