Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@BeMacized
Copy link
Contributor

This PR adds a onUrlChanged event to the platform interface that is supposed to be fired whenever the url for a webview changes. In contrast to the already existing onPageStarted, onProgress, and onPageFinished events, onUrlChanged also fires for url changes without a full page load, like when navigating around in a single page application.

Relevant issue:

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter]. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

LGTM with one small nit.

…b/src/platform_interface/webview_platform_callbacks_handler.dart

Co-authored-by: Maurits van Beusekom <[email protected]>
@BeMacized BeMacized added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Nov 16, 2021
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite ios-platform_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Nov 16, 2021
@BeMacized BeMacized added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Nov 17, 2021
@fluttergithubbot fluttergithubbot merged commit 349a858 into flutter:master Nov 18, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 18, 2021
@stuartmorgan-g
Copy link
Contributor

This was a breaking change :(

Because implementations use implements rather than extends, it can't be trivially fixed within this package, so I'm going to have to revert while we figure out how to do this differently.

stuartmorgan-g added a commit to stuartmorgan-g/plugins that referenced this pull request Nov 18, 2021
@BeMacized
Copy link
Contributor Author

This was a breaking change :(

I was not aware this would lead to a breaking change, absolutely my bad.
Currently I do not see a way to do this differently without marking it as a breaking change.
I'll discuss this with @mvanbeusekom tomorrow to see if we can find a better solution.

stuartmorgan-g added a commit that referenced this pull request Nov 18, 2021
Reverts #4509, which is ecosystem-breaking since it adds a new method to the interface that no existing code implements.

Note that while this PR is technically a breaking change, it is deliberately versioned as a non-breaking change so that people will automatically pick up the fix for the previous accidentally-breaking change. (In practice, this revert would only breaking if someone implemented this new method in an unendorsed webview implementation sometime in the last ~12 hours that the change was live).
@stuartmorgan-g
Copy link
Contributor

I was not aware this would lead to a breaking change, absolutely my bad.

Yes, this is a structural issue in our CI; we need to implement flutter/flutter#89862

@stuartmorgan-g
Copy link
Contributor

Currently I do not see a way to do this differently without marking it as a breaking change.
I'll discuss this with @mvanbeusekom tomorrow to see if we can find a better solution.

This interface is "internal" to the implementations (i.e., it's not something that clients of the app-facing package would implement), right?

If so, the non-breaking change would likely be something like:

  • Add a new, second callbacks object. (To future-proof this one, prominently document that anything implementing it should extend, not implement, as more things may be added in the future. And that any method added to it must have a default implementation.)
  • Make a new alternate build method in the platform interface that includes that second object as an argument; give it a default implementation that calls the old build method (discarding the new object) so that's not breaking.

Then the implementations can implement the new version of build to wire things up using that new object, and the app-facing package can switch to calling the new build instead of the old one. It's not great from an API perspective, but if that's contained to the implementing packages it's livable.

@BeMacized
Copy link
Contributor Author

@stuartmorgan The problem I'm running into with this solution is that the WebViewCupertino and WebViewAndroid classes from the native packages both implement WebViewPlatform. As far as I can see, this solution would still result in a breaking change.

@stuartmorgan-g
Copy link
Contributor

The problem I'm running into with this solution is that the WebViewCupertino and WebViewAndroid classes from the native packages both implement WebViewPlatform.

Well that's not good.

I'll need to take a look in detail at the class structure here and see if there's a point where we could set up a new opt-in path, or if we're stuck doing a breaking change to fix the overuse of implements

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
Reverts flutter#4509, which is ecosystem-breaking since it adds a new method to the interface that no existing code implements.

Note that while this PR is technically a breaking change, it is deliberately versioned as a non-breaking change so that people will automatically pick up the fix for the previous accidentally-breaking change. (In practice, this revert would only breaking if someone implemented this new method in an unendorsed webview implementation sometime in the last ~12 hours that the change was live).
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
Reverts flutter#4509, which is ecosystem-breaking since it adds a new method to the interface that no existing code implements.

Note that while this PR is technically a breaking change, it is deliberately versioned as a non-breaking change so that people will automatically pick up the fix for the previous accidentally-breaking change. (In practice, this revert would only breaking if someone implemented this new method in an unendorsed webview implementation sometime in the last ~12 hours that the change was live).
@devnta

This comment was marked as off-topic.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes p: webview_flutter Edits files for a webview_flutter plugin waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants