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

[google_maps_flutter] Android Code Inspection and Clean up #3112

Merged
merged 17 commits into from
Nov 20, 2020
Merged

[google_maps_flutter] Android Code Inspection and Clean up #3112

merged 17 commits into from
Nov 20, 2020

Conversation

@hamdikahloun
Copy link
Member

@hamdikahloun hamdikahloun commented Oct 5, 2020

Description

  • Handle deprecation & unchecked warning as error

  • Avoiding uses or overrides a deprecated API

Related Issues

flutter/flutter#65970

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@hamdikahloun hamdikahloun requested a review from cyanglaz as a code owner October 5, 2020 14:57
@hamdikahloun hamdikahloun mentioned this pull request Oct 5, 2020
Closed
dnfield
dnfield reviewed Oct 5, 2020
View reviewed changes
...oogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapBuilder.java Outdated
import com.google.android.gms.maps.model.LatLngBounds;
import io.flutter.plugin.common.BinaryMessenger;
import io.flutter.plugin.common.PluginRegistry;
import io.flutter.plugin.common.*;
Copy link
Contributor

@dnfield dnfield Oct 5, 2020

Choose a reason for hiding this comment

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

Please use specific imports, avoid *

dnfield
dnfield reviewed Oct 5, 2020
View reviewed changes
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java Outdated
import io.flutter.plugin.common.MethodCall;
import io.flutter.plugin.common.MethodChannel;
import io.flutter.plugin.common.PluginRegistry;
import io.flutter.plugin.common.*;
Copy link
Contributor

@dnfield dnfield Oct 5, 2020

Choose a reason for hiding this comment

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

Ditto

Copy link
Member Author

@hamdikahloun hamdikahloun Oct 5, 2020

Choose a reason for hiding this comment

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

i have this warning if i just import PluginRegistry :

warning: [deprecation] PluginRegistry in io.flutter.plugin.common has been deprecated
import io.flutter.plugin.common.PluginRegistry;

Copy link
Contributor

@dnfield dnfield Oct 5, 2020

Choose a reason for hiding this comment

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

@xster @amirh @blasten - do any of you know if the maps plugin still needs deprecated functionality from PluginRegistry? I have some vague memory of it being special for some reason.

Copy link
Member Author

@hamdikahloun hamdikahloun Oct 5, 2020

Choose a reason for hiding this comment

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

@dnfield i fixed using io.flutter.plugin.common.PluginRegistry.Registrar

Copy link
Member

@xster xster Nov 4, 2020

Choose a reason for hiding this comment

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

Correct, you can't import deprecated classes. You have to use it by the fully qualified name and add the lint ignore statement on the same line.

@hamdikahloun hamdikahloun requested a review from dnfield October 5, 2020 16:56
dnfield
dnfield reviewed Oct 14, 2020
View reviewed changes
...oogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapBuilder.java Outdated
Application application,
Lifecycle lifecycle,
PluginRegistry.Registrar registrar,
@SuppressWarnings("deprecation") io.flutter.plugin.common.PluginRegistry.Registrar registrar,
Copy link
Contributor

@dnfield dnfield Oct 14, 2020

Choose a reason for hiding this comment

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

@cyanglaz or @blasten what's the plan for this longer term? I remember something funky about this plugin and the v2 embedding but not specifics.

Copy link
Member Author

@hamdikahloun hamdikahloun Nov 4, 2020

Choose a reason for hiding this comment

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

@dnfield it's fixed

@hamdikahloun hamdikahloun requested a review from blasten October 27, 2020 09:58
@hamdikahloun hamdikahloun requested a review from dnfield November 3, 2020 12:47
@hamdikahloun hamdikahloun requested a review from xster November 17, 2020 19:14
xster
xster reviewed Nov 18, 2020
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

The API dedeprecation usage LGTM

...flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated
if (data.size() == 2) {
return BitmapDescriptorFactory.fromAsset(
FlutterMain.getLookupKeyForAsset(toString(data.get(1))));
FlutterInjector.instance()
Copy link
Member

@xster xster Nov 18, 2020

Choose a reason for hiding this comment

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

nit, just keep a local reference to the flutter loader to keep it shorter

@hamdikahloun hamdikahloun requested review from xster and removed request for blasten and cyanglaz November 18, 2020 08:29
dnfield
dnfield reviewed Nov 18, 2020
View reviewed changes
...flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated
class Convert {

private static BitmapDescriptor toBitmapDescriptor(Object o) {
final FlutterLoader flutterLoader = FlutterInjector.instance().flutterLoader();
Copy link
Contributor

@dnfield dnfield Nov 18, 2020

Choose a reason for hiding this comment

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

This probably isn't present on stable, or if it is, we have to increase the minimum version Flutter SDK constraint most likelyt o capture it. /cc @xster

Copy link
Contributor

@dnfield dnfield Nov 18, 2020

Choose a reason for hiding this comment

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

We should also consider whether we just ignore a depprecation warning for this or not - xster will have more context there.

Copy link
Member

@xster xster Nov 18, 2020

Choose a reason for hiding this comment

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

phew, glad you caught me. Forgot about targeting stable. We should indeed leave an ignore like https://github.com/flutter/plugins/pull/3072/files#diff-e57a1a7ffb729ad174b597626202429c1c87e4168e19b2aa9decc1be84e8777bR56 with a TODO to remove when the next stable ships. For now we have to use the old APIs.

dnfield
dnfield suggested changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Need to figure out the Flutter SDK constraint or used deprecated API

...flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated
class Convert {

private static BitmapDescriptor toBitmapDescriptor(Object o) {
final FlutterLoader flutterLoader = FlutterInjector.instance().flutterLoader();
Copy link
Contributor

@dnfield dnfield Nov 18, 2020

Choose a reason for hiding this comment

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

We should also consider whether we just ignore a depprecation warning for this or not - xster will have more context there.

@hamdikahloun hamdikahloun requested a review from dnfield November 19, 2020 12:30
dnfield
dnfield approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM modulo filing an issue for the TODO

...flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated
class Convert {

// TODO(hamdikahloun): FlutterMain has been deprecated and should be replaced with FlutterLoader
// when it's available in Stable channel.
Copy link
Contributor

@dnfield dnfield Nov 19, 2020

Choose a reason for hiding this comment

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

Please file an issue if there isn't one already and link it here.

@hamdikahloun hamdikahloun merged commit f15a800 into flutter:master Nov 20, 2020
amantoux pushed a commit to amantoux/plugins that referenced this pull request Feb 8, 2021
@hamdikahloun @amantoux
[google_maps_flutter] Android Code Inspection and Clean up (flutter#3112
c1c63e7 
)

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Import PluginRegistry

* Import PluginRegistry

* Update GoogleMapController.java

* google_maps_flutter

* Update build.gradle

* Update Convert.java

* Update Convert.java

Add TODO comment

* Update Convert.java
adsonpleal pushed a commit to nubank/plugins that referenced this pull request Feb 26, 2021
@hamdikahloun @adsonpleal
[google_maps_flutter] Android Code Inspection and Clean up (flutter#3112
ae054b3 
)

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Android Code Inspection and Clean up

* Import PluginRegistry

* Import PluginRegistry

* Update GoogleMapController.java

* google_maps_flutter

* Update build.gradle

* Update Convert.java

* Update Convert.java

Add TODO comment

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

Reviewers

@xster xster Awaiting requested review from xster

1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hamdikahloun @xster @dnfield @googlebot