Set deep linking flag to true by default #52350

hannah-hyj · 2024-04-24T01:25:22Z

doc: flutter.dev/go/deep-link-flag-migration

Action item: make sure customers are aware of this change before merging this PR.

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 Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

shell/platform/darwin/ios/framework/Source/FlutterAppDelegateTest.mm

shell/platform/android/test/io/flutter/embedding/android/FlutterFragmentActivityTest.java

jmagman

Is there any mechanism for the framework to say whether pushRouteInformation successfully pushed a known route? If so, it might be better to return that (the non-deprecated version of this API is async https://developer.apple.com/documentation/uikit/uiapplication/1648685-openurl, so we could migrate onto that)

hannah-hyj · 2024-04-24T22:54:16Z

@jmagman yes, framework method Future<bool> didPushRouteInformation(RouteInformation routeInformation) does return true when successfully pushed a known route.

But i think migrating to a non-deprecated async API will be a separated issue and a separated PR from this flag change .

jmagman · 2024-04-24T23:01:24Z

But i think migrating to a non-deprecated async API will be a separated issue and a separated PR from this flag change .

I'm not really concerned that it's deprecated, more that this PR changes behavior where this method will be returning true even when there's no deep linking set up, and it's not opted into.

hannah-hyj · 2024-05-01T21:49:31Z

Looks like the framework currently sends routeInformationUpdated message to navigationChannel when route changes. But these navigation messages are only handled on the web, it’s not handled on the iOS engine.

Maybe we can do

On the iOS engine, add a method call handler to ios navigationChannel and save the route information so the engine knows if the route is changed.
On the iOS engine openURL method, after sending pushRouteInformation, wait for 1 second(just an eye-balled value) and check if the route information is changed, if so, return YES, if not, return NO.

chunhtai · 2024-05-01T22:33:32Z

Looks like the framework currently sends routeInformationUpdated message to navigationChannel when route changes. But these navigation messages are only handled on the web, it’s not handled on the iOS engine.

Maybe we can do

On the iOS engine, add a method call handler to ios navigationChannel and save the route information so the engine knows if the route is changed.

On the iOS engine openURL method, after sending pushRouteInformation, wait for 1 second(just an eye-balled value) and check if the route information is changed, if so, return YES, if not, return NO.

Why does iOS need to listen for routeInformationUpdated ? should we be able to tell from didPushRouteInformation's return value to tell whether anything has handled the deeplink?

hannah-hyj · 2024-05-01T22:40:31Z

cause didPushRouteInformation's return value is on the framework side, i thought the way for engine to get it is through a method channel, so the ios openURL returns true only when the route is changed on the framework side

chunhtai · 2024-05-01T22:55:18Z

Can you make _handleNavigationInvocation to return a future of boolean to indicate whether any of the widgetbindObserver.didpushRouteInformation has handled the route or not?

The didUpdateRouteInformation is to report current route in framework to engine. It will be smart enough to check whether the current route/state with the one in engine is the same or not before sending the request. So it is not guaranteed that didUpdateRouteInformation will be called after a successful deeplink

chunhtai · 2024-05-01T22:56:45Z

That being said we should probably add the feature to reject deeplink in go_router, right now it just swallow all and handle error on the framework side

hannah-hyj · 2024-05-01T23:00:07Z

Can you make _handleNavigationInvocation to return a future of boolean to indicate whether any of the widgetbindObserver.didpushRouteInformation has handled the route or not? >> noted, I will do that, thanks!

reidbaker · 2024-05-09T18:51:24Z

@Hangyujin Are you wanting to make more changes or is this ready for revew?

hannah-hyj · 2024-05-09T19:22:22Z

@reidbaker i want to land flutter/flutter#147901 and #52643 first.

…r any of the observer has handled the route or not (#147901) follow up on comments on flutter/engine#52350

zanderso · 2024-05-30T20:14:44Z

From PR triage: This appears to still be waiting on #52643

yaakovschectman · 2024-06-13T18:16:31Z

From triage: Is this ready for review yet?

hannah-hyj · 2024-06-18T19:02:43Z

i will close this for now until #52643 is landed

…mework (#52643) follow up on comments on #52350 framework pr : flutter/flutter#147901 ## 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 [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

sfshaza2 · 2024-08-01T18:44:18Z

Thanks, for calling this out, @Hangyujin! I've created a docs bug for adding this info in Q4 (for that release).

)

…152707) flutter/engine@17e3c7d...1cbe88e 2024-08-01 jonahwilliams@google.com [iOS] Supported rendering platform views without merging the raster thread. (flutter/engine#53826) 2024-08-01 jhy03261997@gmail.com Set deep linking flag to true by default (flutter/engine#52350) 2024-08-01 leroux_bruno@yahoo.fr [Android] Revert "Reset IME state on clear text input client" (flutter/engine#54277) 2024-08-01 skia-flutter-autoroll@skia.org Roll Skia from 03732b9f885e to ddb6901e6141 (5 revisions) (flutter/engine#54287) 2024-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 5acd806b6dad to acbbbe73b5eb (1 revision) (flutter/engine#54286) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#152707) flutter/engine@17e3c7d...1cbe88e 2024-08-01 jonahwilliams@google.com [iOS] Supported rendering platform views without merging the raster thread. (flutter/engine#53826) 2024-08-01 jhy03261997@gmail.com Set deep linking flag to true by default (flutter/engine#52350) 2024-08-01 leroux_bruno@yahoo.fr [Android] Revert "Reset IME state on clear text input client" (flutter/engine#54277) 2024-08-01 skia-flutter-autoroll@skia.org Roll Skia from 03732b9f885e to ddb6901e6141 (5 revisions) (flutter/engine#54287) 2024-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 5acd806b6dad to acbbbe73b5eb (1 revision) (flutter/engine#54286) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jacobsimionato · 2024-10-23T00:24:42Z

We noticed that the change in default of FlutterDeepLinkEnabled here broke Firebase dynamic links integration for a Google app. I wonder if this might also cause problems for other apps?

See internal bug b/371483505.

zanderso · 2024-10-23T00:55:36Z

@jacobsimionato a comment on a PR that was landed over two months ago is probably not the right way to flag this issue. I think the normal flow is to file a mirror issue in GitHub that can then go through teams' regular triage processes?

jacobsimionato · 2024-10-23T04:34:39Z

Sorry about that! The reason I commented rather than filing a bug is because I'm not actually sure if this is a problem at all or intentional behavior that is fully expected. TBH I think it's probably the latter based on flutter.dev/go/deep-link-flag-migration. The team who filed b/371483505 in Google simply opted out of FlutterDeepLinkingEnabled so that issue is considered resolved and no-one is blocked.

Maybe Chat or Discord is a better place to mention something like this?

Anyway, apologies for the confusion and noise.

hannah-hyj · 2024-10-23T18:00:43Z

@jocobsiomato I think the behavior is intended. It's not a flutter issue.

If Firebase dynamic links integration is broken. They should update their docs to tell end users that if they want to use Firebase dynamic links, they must opt out of the Flutter default deep link setup by setting the flag. I replied in the internal issue.

reidbaker · 2024-10-23T18:29:31Z

@hannah-hyj it seems unlikely they would know about this change in our code. Did we reach out to firebase to tell them?

hannah-hyj · 2024-10-23T18:32:13Z

From their website, https://firebase.google.com/docs/dynamic-links/flutter/create Firebase dynamic links is deprecated, should we still nudge them on this?
We can discuss this further in the internal issue or chat as #52350 (comment) mentioned a closed PR is not the best place to discuss this.

…ault value change. (#11461) The deep link flag default value is changed in flutter/engine#52350, and the PR is included Flutter 3.27 stable release. The Deep link docs should be updated to reflect the changes: 1. If you are using flutter deep link handlers, you don't need to manually set the flutter_deeplinking_enabled flag to true anymore. 3. If you use third-party plug-ins to handle deep links, you need to manually set the flutter_deeplinking_enabled flag to `false` More context: [Design doc](https://docs.google.com/document/d/1TUhaEhNdi2BUgKWQFEbOzJgmUAlLJwIAhnFfZraKgQs/edit?tab=t.0) _Issues fixed by this PR (if any): #10984 _PRs or commits this PR depends on (if any): flutter/engine#52350 ## Presubmit checklist - [ ] This PR is marked as draft with an explanation if not meant to land until a future stable release. - [ ] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [ ] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [ ] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <44418985+sfshaza2@users.noreply.github.com>

… website (#11493) Add doc about the breaking change and migration guide. Related PR: flutter/engine#52350 ## Presubmit checklist - [ ] This PR is marked as draft with an explanation if not meant to land until a future stable release. - [ ] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [ ] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [ ] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <44418985+sfshaza2@users.noreply.github.com>

This comment was marked as outdated.

Sign in to view

github-actions bot added platform-ios platform-android labels Apr 24, 2024

jmagman self-requested a review April 24, 2024 21:04

hannah-hyj requested a review from chunhtai April 24, 2024 21:21

chunhtai reviewed Apr 24, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterAppDelegateTest.mm Show resolved Hide resolved

shell/platform/android/test/io/flutter/embedding/android/FlutterFragmentActivityTest.java Show resolved Hide resolved

jmagman reviewed Apr 24, 2024

View reviewed changes

camsim99 requested a review from reidbaker April 26, 2024 17:47

jmagman self-requested a review May 1, 2024 21:06

This was referenced May 7, 2024

Update _handlePushRouteInformation to Future<bool> to indicate whether any of the observer has handled the route or not flutter/flutter#147901

Merged

[deep link][ios] Update openURL method to reflect the result from framework #52643

Merged

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 15, 2024

Update _handlePushRouteInformation to Future<bool> to indicate whethe…

6826fc0

…r any of the observer has handled the route or not (#147901) follow up on comments on flutter/engine#52350

hannah-hyj closed this Jun 18, 2024

hannah-hyj reopened this Jul 17, 2024

hannah-hyj requested a review from chunhtai July 17, 2024 21:02

hannah-hyj force-pushed the deeplinkflag branch from 1e68db5 to b79b311 Compare July 17, 2024 21:55

hannah-hyj and others added 9 commits August 1, 2024 11:22

update

96e87e3

add tests

8ea2d1b

Update FlutterFragmentActivityTest.java

8f1b9c5

fix testing failure

eb0a4ef

move duplicated code to shared place

5a1ddc6

lint

318ab25

lint

740ffa9

update comments

7c09085

fix import

c22082d

hannah-hyj force-pushed the deeplinkflag branch from 590016d to c22082d Compare August 1, 2024 18:24

sfshaza2 mentioned this pull request Aug 1, 2024

Add a breaking change doc for deep linking support flutter/website#10984

Closed

1 task

hannah-hyj merged commit 30ffe6e into flutter:main Aug 1, 2024
27 checks passed

engine-flutter-autoroll mentioned this pull request Aug 1, 2024

Roll Flutter Engine from 17e3c7d520e7 to 1cbe88e8115e (5 revisions) flutter/flutter#152707

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 1, 2024

30ffe6eb4 Set deep linking flag to true by default (flutter/engine#52350

110e032

)

llfbandit mentioned this pull request Sep 10, 2024

iOS Universal Links not working correctly with Flutter 3.24 llfbandit/app_links#156

Closed

hannah-hyj mentioned this pull request Oct 5, 2024

iOS deep links not working correctly with Flutter 3.24.2 Milad-Akarie/auto_route_library#2055

Open

hannah-hyj mentioned this pull request Dec 3, 2024

Update deep link docs due to the flutter_deeplinking_enabled flag default value change. flutter/website#11461

Merged

4 tasks

hannah-hyj mentioned this pull request Dec 13, 2024

Add a breaking change doc about deep links flag change to the flutter website flutter/website#11493

Merged

4 tasks

Set deep linking flag to true by default #52350

Set deep linking flag to true by default #52350

Uh oh!

Conversation

hannah-hyj commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

hannah-hyj commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented Apr 24, 2024

Uh oh!

hannah-hyj commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented May 1, 2024

Uh oh!

hannah-hyj commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented May 1, 2024

Uh oh!

chunhtai commented May 1, 2024

Uh oh!

hannah-hyj commented May 1, 2024

Uh oh!

reidbaker commented May 9, 2024

Uh oh!

hannah-hyj commented May 9, 2024

Uh oh!

zanderso commented May 30, 2024

Uh oh!

yaakovschectman commented Jun 13, 2024

Uh oh!

hannah-hyj commented Jun 18, 2024

Uh oh!

sfshaza2 commented Aug 1, 2024

Uh oh!

Uh oh!

jacobsimionato commented Oct 23, 2024

Uh oh!

zanderso commented Oct 23, 2024

Uh oh!

jacobsimionato commented Oct 23, 2024

Uh oh!

hannah-hyj commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reidbaker commented Oct 23, 2024

Uh oh!

hannah-hyj commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hannah-hyj commented Apr 24, 2024 •

edited

Loading

hannah-hyj commented Apr 24, 2024 •

edited

Loading

hannah-hyj commented May 1, 2024 •

edited

Loading

hannah-hyj commented May 1, 2024 •

edited

Loading

hannah-hyj commented Oct 23, 2024 •

edited

Loading

hannah-hyj commented Oct 23, 2024 •

edited

Loading