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

[google_maps_flutter] Maps snapshot #1719

Closed
wants to merge 23 commits into from
Closed

[google_maps_flutter] Maps snapshot #1719

wants to merge 23 commits into from

Conversation

duzenko
Copy link
Contributor

@duzenko duzenko commented Jun 10, 2019

Description

Implementing the native snapshot() function call.

Related Issues

flutter/flutter#33557

Breaking Change

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

  • No, this is not a breaking change.

@duzenko duzenko closed this Jun 11, 2019
@duzenko

This comment has been minimized.

@duzenko duzenko reopened this Jun 11, 2019
@duzenko duzenko mentioned this pull request Jun 11, 2019
1 task
@duzenko duzenko changed the title Maps snapshot [[google_maps_flutter] Maps snapshot Jun 13, 2019
@duzenko duzenko changed the title [[google_maps_flutter] Maps snapshot [google_maps_flutter] Maps snapshot Jun 13, 2019
@iskakaushik
Copy link
Contributor

@duzenko thanks for the PR. We have now added support for on-device tests for google maps plugin. Please take a look at this file for an example test case. I will be happy to help you through any issue you run into while adding tests. Please add some relevant tests to your PR :-)

@duzenko

This comment has been minimized.

@iskakaushik
Copy link
Contributor

@duzenko one of the issues has been fixed, I will release 0.5.18 soon. For the plugin tools crash, i'm raising it with my team. Looks like stuart is looking into the slow builds issue.

Sorry that you are running into these troubles. Also, thanks for the continued contributions to google maps plugin, really appreciate it.

@klaszlo8207
Copy link

Any new on this PR?

@mtsrdr
Copy link

mtsrdr commented Jan 23, 2020

Any progress on this?

@duzenko

This comment has been minimized.

@cyanglaz
Copy link
Contributor

@duzenko There seems to be a lot of conflicts. It is hard to review as it is now. Could you rebase?

@duzenko

This comment has been minimized.

# Conflicts:
#	packages/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
#	packages/google_maps_flutter/example/lib/map_ui.dart
#	packages/google_maps_flutter/example/test_driver/google_maps.dart
#	packages/google_maps_flutter/lib/src/controller.dart
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thank you for the quick turn-around on rebasing :) This is awesome! The PR looks good overall. I have left some comments and thoughts.
Also, we would need to add some tests to land this new feature.

I would suggest to add an e2e test case to compare screenshot data.

@@ -1,4 +1,4 @@
org.gradle.jvmargs=-Xmx1536M
#org.gradle.jvmargs=-Xmx1536M
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

This comment has been minimized.

packages/google_maps_flutter/example/lib/map_ui.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/lib/src/controller.dart Outdated Show resolved Hide resolved
@cyanglaz
Copy link
Contributor

Why does the formatting test fail? I used the plugin tools to format the code before committing.

I assume you use https://github.com/flutter/plugin_tools to format? Try to update the plugin_tool to the latest and reactivate it see if it helps.

@duzenko

This comment has been minimized.

@cyanglaz
Copy link
Contributor

@duzenko Have you tried to use https://github.com/flutter/plugin_tools to format instead of android studio?

@duzenko

This comment has been minimized.

@duzenko

This comment has been minimized.

@cyanglaz
Copy link
Contributor

@duzenko works for me :) Thanks

@duzenko

This comment has been minimized.

@cyanglaz
Copy link
Contributor

@duzenko I have formatted the code. Thanks. I will wait on your response to my comments :)

Copy link
Contributor Author

@duzenko duzenko left a comment

Choose a reason for hiding this comment

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

Agree to all points

@@ -1,4 +1,4 @@
org.gradle.jvmargs=-Xmx1536M
#org.gradle.jvmargs=-Xmx1536M

This comment has been minimized.

@duzenko

This comment has been minimized.

@fabiorsilva
Copy link

Any progress on this?

@hinterlandcreative
Copy link

hinterlandcreative commented Feb 27, 2020

Any update on this PR? This is blocking our app release. It looks like this was close to being merged?

CC: @duzenko @cyanglaz

@cyanglaz
Copy link
Contributor

This feature is landed with #2607. I'm closing this PR :)

@cyanglaz cyanglaz closed this Mar 26, 2020
@duzenko

This comment has been minimized.

@cyanglaz
Copy link
Contributor

@duzenko I apologize not being active with your PR, it was simply because I didn't have much cycle working on this. Your PR is great. And in fact, I think the other PR mentioned your PR in the description as a reference. I landed the other PR simply because it was ready to merge, and we still had some work to do to make this PR ready to merge. I hope that answered your question :)

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

Successfully merging this pull request may close these issues.

8 participants