Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/firebase_dynamic_links/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.5.0+7

* Support v2 embedding. This will remain compatible with the original embedding and won't require app migration.

## 0.5.0+6

* Updated README instructions for contributing for consistency with other Flutterfire plugins.
Expand Down
27 changes: 26 additions & 1 deletion packages/firebase_dynamic_links/android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,33 @@ android {
dependencies {
api 'com.google.firebase:firebase-dynamic-links:16.1.8'
implementation 'com.google.firebase:firebase-common:16.1.0'
implementation 'androidx.annotation:annotation:1.0.0'
}
}

apply from: file("./user-agent.gradle")

// TODO(bparrishMines): Remove this hack once androidx.lifecycle is included on stable. https://github.com/flutter/flutter/issues/42348
afterEvaluate {
def containsEmbeddingDependencies = false
for (def configuration : configurations.all) {
for (def dependency : configuration.dependencies) {
if (dependency.group == 'io.flutter' &&
dependency.name.startsWith('flutter_embedding') &&
dependency.isTransitive())
{
containsEmbeddingDependencies = true
break
}
}
}
if (!containsEmbeddingDependencies) {
android {
dependencies {
def lifecycle_version = "1.1.1"
compileOnly "android.arch.lifecycle:runtime:$lifecycle_version"
Copy link
Contributor

Choose a reason for hiding this comment

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

are there existing androidx transitive dependencies? (since there's an androidx annotation already) Is this a downgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These use the latest instruction from the migration wiki: https://github.com/flutter/flutter/wiki/Experimental:-Create-Flutter-Plugin.

Quoting: flutter/plugins#2221 (comment)
We introduced a constraint that the app uses the plugin has to be migrated to androidx. This patch removes the constraint.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mklim @blasten we had this conversation separately too. I guess we're saying mixing is ok and we're just letting plugins build as aars as fallbacks?

compileOnly "android.arch.lifecycle:common:$lifecycle_version"
compileOnly "android.arch.lifecycle:common-java8:$lifecycle_version"
}
}
}
}
1 change: 0 additions & 1 deletion packages/firebase_dynamic_links/android/gradle.properties

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@

package io.flutter.plugins.firebasedynamiclinks;

import android.app.Activity;
import android.content.Intent;
import android.net.Uri;
import androidx.annotation.NonNull;
Copy link
Contributor

Choose a reason for hiding this comment

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

You went from androidx to android.arch in the gradle, which seems to cause this to not work. Though I'm not sure if it's intentional. Or did firebase already conceptually "upgrade" to androidx @mklim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answered in #1372 (comment)

Copy link

Choose a reason for hiding this comment

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

The latest thing we've talked about here is:

  1. android.arch for the magic lifecycle dependencies in the Gradle script. That way Jetifier can run on the artifacts in cases where it's needed, and it will resolve as-is for unmigrated apps.
  2. annotation should just be removed from both Gradle and source code files entirely. If it's present in source code Jetifier won't be able to run on it and it will cause compile failures for when there's AndroidX incompatibilities. I don't think that's worth just having the annotations.

If this is being stripped from the source code but it's still in the Gradle files as android.arch.annotation it should probably be removed from the Gradle files too. If it's not in the Gradle files at all this makes sense to me and shouldn't be causing known issues.

import com.google.android.gms.tasks.OnCompleteListener;
import com.google.android.gms.tasks.OnFailureListener;
import com.google.android.gms.tasks.OnSuccessListener;
Expand All @@ -15,6 +15,10 @@
import com.google.firebase.dynamiclinks.FirebaseDynamicLinks;
import com.google.firebase.dynamiclinks.PendingDynamicLinkData;
import com.google.firebase.dynamiclinks.ShortDynamicLink;
import io.flutter.embedding.engine.plugins.FlutterPlugin;
import io.flutter.embedding.engine.plugins.activity.ActivityAware;
import io.flutter.embedding.engine.plugins.activity.ActivityPluginBinding;
import io.flutter.plugin.common.BinaryMessenger;
import io.flutter.plugin.common.MethodCall;
import io.flutter.plugin.common.MethodChannel;
import io.flutter.plugin.common.MethodChannel.MethodCallHandler;
Expand All @@ -27,15 +31,30 @@
import java.util.Map;

/** FirebaseDynamicLinksPlugin */
public class FirebaseDynamicLinksPlugin implements MethodCallHandler, NewIntentListener {
private final Registrar registrar;
private final MethodChannel channel;
public class FirebaseDynamicLinksPlugin
implements FlutterPlugin, ActivityAware, MethodCallHandler, NewIntentListener {
private Registrar registrar;
private MethodChannel channel;
private ActivityPluginBinding activityBinding;

private FirebaseDynamicLinksPlugin(Registrar registrar, MethodChannel channel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You added annotations right? Tag Registrar nonnull. Even better, since you only pull the activity out of the registrar, just change the instance variable to activity so future maintainers don't accidentally do registrar.messenger() and expand the encapsulation in the future and trip on a null half of the time.

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 removed the annotations in this PR because the ecosystem team decided that we won't include androidx in any plugin that doesn't require it. It's so that a Flutter app that doesn't include androidx can use this plugin.

However, this is a good point about someone accidently using the registrar. What about adding the @Deprecated annotation with an explanation about registrar remaining until we decide to only support v2 embedding flutter apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does remind me that I also need to add documentation for the plugin now. Updated PR

Copy link

@mklim mklim Nov 13, 2019

Choose a reason for hiding this comment

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

Sorry, this tab was open and I missed the new comments. I agree with Maurice's summary about annotation, commented here.

For @Deprecated, I vaguely remember floating the idea by @amirh offline and he was against it. If I'm remembering right the reason being was that for now the static registrar registerWith method is fully supported, and we don't want to flag it as deprecated before we're actually really deprecating it. I think those uses will show up as compile time warnings too, so probably better not to bring them in.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to @Deprecate the actual Registrar API in the engine like @mklim said.
If you meant @Deprecate the instance variable, that sounds ok. I don't have a strong opinion. I was just making a general comment (regardless of what the class is) to minimize access. If your plugin instance wants an activity, just keep an activity and not its provider.

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 agree. I remember that we originally kept an instance of Registrar and not Activity because the method Registrar.getActivity() would sometimes return different activities in add-to-app scenarios.

Since we're now migrating to v2 embedding, I don't this this would be the case anymore. I updated the plugin to hold a reference to Activity.

this.registrar = registrar;
this.channel = channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to pass this in right? The less potentially dangling nulls there are (depending on the path you take), the less likely there will be accidental null pointer exceptions

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 should need it. The FirebaseDynamicLinks plugins makes callbacks to dart: https://github.com/FirebaseExtended/flutterfire/pull/1372/files#diff-3f6c424218a2662c8ddc4b322f30bf66R81.

I could hold on to a BinaryMessenger and instantiate a MethodChannel every time we want to call invokeMethod, but I think this looks cleaner.

}

public FirebaseDynamicLinksPlugin() {}

public static void registerWith(Registrar registrar) {
final MethodChannel channel = createChannel(registrar.messenger());
final FirebaseDynamicLinksPlugin plugin = new FirebaseDynamicLinksPlugin(registrar, channel);
registrar.addNewIntentListener(plugin);
channel.setMethodCallHandler(plugin);
}

private static MethodChannel createChannel(final BinaryMessenger messenger) {
return new MethodChannel(messenger, "plugins.flutter.io/firebase_dynamic_links");
}

@Override
public boolean onNewIntent(Intent intent) {
FirebaseDynamicLinks.getInstance()
Expand All @@ -54,7 +73,7 @@ public void onSuccess(PendingDynamicLinkData pendingDynamicLinkData) {
.addOnFailureListener(
new OnFailureListener() {
@Override
public void onFailure(@NonNull Exception e) {
public void onFailure(Exception e) {
Map<String, Object> exception = new HashMap<>();
exception.put("code", e.getClass().getSimpleName());
exception.put("message", e.getMessage());
Expand All @@ -66,12 +85,39 @@ public void onFailure(@NonNull Exception e) {
return false;
}

public static void registerWith(Registrar registrar) {
final MethodChannel channel =
new MethodChannel(registrar.messenger(), "plugins.flutter.io/firebase_dynamic_links");
final FirebaseDynamicLinksPlugin plugin = new FirebaseDynamicLinksPlugin(registrar, channel);
registrar.addNewIntentListener(plugin);
channel.setMethodCallHandler(plugin);
@Override
public void onAttachedToEngine(FlutterPluginBinding binding) {
channel = createChannel(binding.getFlutterEngine().getDartExecutor());
channel.setMethodCallHandler(this);
}

@Override
public void onDetachedFromEngine(FlutterPluginBinding binding) {
channel.setMethodCallHandler(null);
}

@Override
public void onAttachedToActivity(ActivityPluginBinding binding) {
activityBinding = binding;
activityBinding.addOnNewIntentListener(this);
}

@Override
public void onDetachedFromActivityForConfigChanges() {
activityBinding.removeOnNewIntentListener(this);
activityBinding = null;
}

@Override
public void onReattachedToActivityForConfigChanges(ActivityPluginBinding binding) {
activityBinding = binding;
activityBinding.addOnNewIntentListener(this);
}

@Override
public void onDetachedFromActivity() {
activityBinding.removeOnNewIntentListener(this);
activityBinding = null;
}

@Override
Expand Down Expand Up @@ -116,15 +162,14 @@ private Map<String, Object> getMapFromPendingDynamicLinkData(

private void handleGetInitialDynamicLink(final Result result) {
// If there's no activity, then there's no initial dynamic link.
if (registrar.activity() == null) {
if ((registrar != null && registrar.activity() == null) && activityBinding == null) {
result.success(null);
return;
}

FirebaseDynamicLinks.getInstance()
.getDynamicLink(registrar.activity().getIntent())
.getDynamicLink(getActivity().getIntent())
.addOnSuccessListener(
registrar.activity(),
new OnSuccessListener<PendingDynamicLinkData>() {
@Override
public void onSuccess(PendingDynamicLinkData pendingDynamicLinkData) {
Expand All @@ -138,19 +183,22 @@ public void onSuccess(PendingDynamicLinkData pendingDynamicLinkData) {
}
})
.addOnFailureListener(
registrar.activity(),
new OnFailureListener() {
@Override
public void onFailure(@NonNull Exception e) {
public void onFailure(Exception e) {
result.error(e.getClass().getSimpleName(), e.getMessage(), null);
}
});
}

private Activity getActivity() {
return registrar != null ? registrar.activity() : activityBinding.getActivity();
}

private OnCompleteListener<ShortDynamicLink> createShortLinkListener(final Result result) {
return new OnCompleteListener<ShortDynamicLink>() {
@Override
public void onComplete(@NonNull Task<ShortDynamicLink> task) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these causing issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answered in #1372 (comment)

public void onComplete(Task<ShortDynamicLink> task) {
if (task.isSuccessful()) {
Map<String, Object> url = new HashMap<>();
url.put("url", task.getResult().getShortLink().toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ flutter {
}

dependencies {
androidTestImplementation 'androidx.test:runner:1.2.0'
androidTestImplementation 'androidx.test:rules:1.2.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
}

apply plugin: 'com.google.gms.google-services'
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package io.flutter.plugins.firebasedynamiclinks;

import androidx.test.rule.ActivityTestRule;
import dev.flutter.plugins.e2e.FlutterRunner;
import io.flutter.plugins.firebasedynamiclinksexample.EmbeddingV1Activity;
import org.junit.Rule;
import org.junit.runner.RunWith;

@RunWith(FlutterRunner.class)
public class EmbeddingV1ActivityTest {
@Rule
public ActivityTestRule<EmbeddingV1Activity> rule =
new ActivityTestRule<>(EmbeddingV1Activity.class);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package io.flutter.plugins.firebasedynamiclinks;

import androidx.test.rule.ActivityTestRule;
import dev.flutter.plugins.e2e.FlutterRunner;
import io.flutter.plugins.firebasedynamiclinksexample.MainActivity;
import org.junit.Rule;
import org.junit.runner.RunWith;

@RunWith(FlutterRunner.class)
public class MainActivityTest {
@Rule public ActivityTestRule<MainActivity> rule = new ActivityTestRule<>(MainActivity.class);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,17 @@
android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|layoutDirection|fontScale|screenLayout|density"
android:hardwareAccelerated="true"
android:windowSoftInputMode="adjustResize">
<meta-data
android:name="io.flutter.app.android.SplashScreenUntilFirstFrame"
android:value="true" />
<intent-filter>
<action android:name="android.intent.action.MAIN"/>
<category android:name="android.intent.category.LAUNCHER"/>
</intent-filter>
</activity>
<activity
android:name=".EmbeddingV1Activity"
android:theme="@style/LaunchTheme"
android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|layoutDirection|fontScale"
android:hardwareAccelerated="true"
android:windowSoftInputMode="adjustResize">
</activity>
</application>
</manifest>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package io.flutter.plugins.firebasedynamiclinksexample;

import android.os.Bundle;
import io.flutter.app.FlutterActivity;
import io.flutter.plugins.GeneratedPluginRegistrant;

public class EmbeddingV1Activity extends FlutterActivity {
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
GeneratedPluginRegistrant.registerWith(this);
}
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package io.flutter.plugins.firebasedynamiclinksexample;

import android.os.Bundle;
import io.flutter.app.FlutterActivity;
import io.flutter.plugins.GeneratedPluginRegistrant;
import dev.flutter.plugins.e2e.E2EPlugin;
import io.flutter.embedding.android.FlutterActivity;
import io.flutter.embedding.engine.FlutterEngine;
import io.flutter.plugins.firebasedynamiclinks.FirebaseDynamicLinksPlugin;

public class MainActivity extends FlutterActivity {
// TODO(bparrishMines): Remove this once v2 of GeneratedPluginRegistrant rolls to stable. https://github.com/flutter/flutter/issues/42694
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
GeneratedPluginRegistrant.registerWith(this);
public void configureFlutterEngine(FlutterEngine flutterEngine) {
flutterEngine.getPlugins().add(new FirebaseDynamicLinksPlugin());
flutterEngine.getPlugins().add(new E2EPlugin());
}
}
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
org.gradle.jvmargs=-Xmx1536M
android.enableR8=true
android.useAndroidX=true
android.enableJetifier=true
5 changes: 5 additions & 0 deletions packages/firebase_dynamic_links/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,10 @@ dependencies:

url_launcher: ^5.1.6

dev_dependencies:
e2e: ^0.2.1
flutter_driver:
sdk: flutter

flutter:
uses-material-design: true
7 changes: 5 additions & 2 deletions packages/firebase_dynamic_links/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: firebase_dynamic_links
description: Flutter plugin for Google Dynamic Links for Firebase, an app solution for creating
and handling links across multiple platforms.
version: 0.5.0+6
version: 0.5.0+7

author: Flutter Team <[email protected]>
homepage: https://github.com/FirebaseExtended/flutterfire/tree/master/packages/firebase_dynamic_links
Expand All @@ -11,6 +11,9 @@ dependencies:
sdk: flutter

dev_dependencies:
e2e: ^0.2.1
flutter_driver:
sdk: flutter
flutter_test:
sdk: flutter
url_launcher: ^4.2.0
Expand All @@ -24,4 +27,4 @@ flutter:

environment:
sdk: ">=2.0.0-dev.28.0 <3.0.0"
flutter: ">=1.5.0 <2.0.0"
flutter: ">=1.9.1+hotfix.4 <2.0.0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import 'package:flutter_test/flutter_test.dart';
import 'package:e2e/e2e.dart';

import '../lib/firebase_dynamic_links.dart';

void main() {
E2EWidgetsFlutterBinding.ensureInitialized();

testWidgets('buildUrl', (WidgetTester tester) async {
final DynamicLinkParameters parameters = DynamicLinkParameters(
uriPrefix: 'https://cx4k7.app.goo.gl',
link: Uri.parse('https://dynamic.link.example/helloworld'),
androidParameters: AndroidParameters(
packageName: 'io.flutter.plugins.firebasedynamiclinksexample',
minimumVersion: 0,
),
dynamicLinkParametersOptions: DynamicLinkParametersOptions(
shortDynamicLinkPathLength: ShortDynamicLinkPathLength.short,
),
iosParameters: IosParameters(
bundleId: 'com.google.FirebaseCppDynamicLinksTestApp.dev',
minimumVersion: '0',
),
);

final Uri uri = await parameters.buildUrl();
expect(
uri.toString(),
'https://cx4k7.app.goo.gl?amv=0&apn=io.flutter.plugins.firebasedynamiclinksexample&ibi=com.google.FirebaseCppDynamicLinksTestApp.dev&imv=0&link=https%3A%2F%2Fdynamic.link.example%2Fhelloworld',
);
});
}