Skip to content
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

[desktop-webview-window] Control URL requests made by the webview (Breaking!) #296

Merged
merged 12 commits into from Nov 8, 2023

Conversation

Iri-Hor
Copy link
Contributor

@Iri-Hor Iri-Hor commented Oct 12, 2023

This PR introduces the capability of controlling url requests made by the webview. Therefore the "OnUrlRequestCallback" now allows to return a boolean value. Returning true grants the webview the navigation to the requested url and returning false doesn't.

Caution: Changes are breaking, because the function "addOnUrlRequestCallback" is now called "setOnUrlRequestCallback", because there is only one of those callbacks possible anymore, as multiple callbacks could return conflicting values for granting or rejecting the url request.

{flutter::EncodableValue("url"), flutter::EncodableValue(wide_to_utf8(std::wstring(uri)))},
}));

if (triggerOnUrlRequestedEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? It should make more sense to trigger onUrlRequest on any request (including from navigate(String) method).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion its more convenient like this, because otherwise the call of "webview.launch(url)" would trigger the OnUrlRequest event every time and you manually had to grant it every time. If you call "webview.launch(url)" you know already that the webview will navigate to the url now, because you requested it yourself. But I am more interested in navigations triggered by the webview itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I can also make it, that also "webview.launch(url)" triggers the event. How would you like to have it?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for later.

But I still stand by my opinion.

for example:
If you need to display the currently requested URL in the console or elsewhere, you only need to add a callback instead of adding code in various places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok seems also reasonable. Then I suggest to add an optional parameter with default value to webview.launch:
webview.launch(..., triggerOnUrlRequestEvent=true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new commit containing the additional parameter for webview.launch

Please review

@@ -222,13 +224,13 @@ class WebviewImpl extends Webview {
}

@override
void addOnUrlRequestCallback(OnUrlRequestCallback callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the callbackparameter should be nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

@override
void removeOnUrlRequestCallback(OnUrlRequestCallback callback) {
_onUrlRequestCallbacks.remove(callback);
void removeOnUrlRequestCallback() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this function.

Copy link
Contributor Author

@Iri-Hor Iri-Hor left a comment

Choose a reason for hiding this comment

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

I supplied commits for all requested changes. Please review again

@Iri-Hor Iri-Hor requested a review from boyan01 November 6, 2023 21:21
@boyan01 boyan01 merged commit 6927ef8 into MixinNetwork:main Nov 8, 2023
@boyan01
Copy link
Contributor

boyan01 commented Nov 8, 2023

LGTM, Thanks for the contribution. 💙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants