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

[google_maps_flutter] Breaking change fix for bitmap scaling #1821

Merged
cyanglaz merged 21 commits into from
Mar 20, 2020
Merged

[google_maps_flutter] Breaking change fix for bitmap scaling #1821

cyanglaz merged 21 commits into from
Mar 20, 2020

Conversation

@duzenko
Copy link
Contributor

@duzenko duzenko commented Jul 9, 2019

Description

Fixes the breaking change introduced in #1737

Related Issues

flutter/flutter#34386

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.

@duzenko duzenko requested review from amirh and iskakaushik as code owners July 9, 2019 09:34
@duzenko duzenko changed the title [google_maps_flutter] Maps scaling bitmap fix [google_maps_flutter] Breaking change fix for bitmap scaling Jul 9, 2019
@asanoki
Copy link

asanoki commented Jul 15, 2019

Friendly ping. What's the status of this pull request?

@duzenko
Merge remote-tracking branch 'upstream/master' into maps-scaling-bitm…
aeb52b2 
…ap-fix

# Conflicts:
#	packages/google_maps_flutter/CHANGELOG.md
#	packages/google_maps_flutter/pubspec.yaml
@duzenko

This comment has been minimized.

Sign in to view
@iskakaushik
Copy link
Contributor

iskakaushik commented Jul 15, 2019

@duzenko is it possible to add a test that would've caused this to fail? or at least an example that would trigger this code path?

@duzenko

This comment has been minimized.

Sign in to view
@iskakaushik
Copy link
Contributor

iskakaushik commented Jul 15, 2019

I could probably write one though. The example was given by @asanoki in the parent PR.

Thanks!

@duzenko duzenko mentioned this pull request Jul 16, 2019
Closed
@duzenko

This comment has been minimized.

Sign in to view
@duzenko duzenko mentioned this pull request Aug 20, 2019
Merged
12 tasks
@rafaelferreirapt
Copy link

rafaelferreirapt commented Sep 10, 2019

any updates?

@duzenko

This comment has been minimized.

Sign in to view
@rafaelferreirapt
Copy link

rafaelferreirapt commented Oct 5, 2019

@asanoki @iskakaushik why the review is taking so long? We are using this PR and it works fine, can it be approved?

@rafaelferreirapt rafaelferreirapt mentioned this pull request Oct 16, 2019
Closed
@rafaelferreirapt
Copy link

rafaelferreirapt commented Oct 16, 2019

@asanoki @iskakaushik why the review is taking so long? We are using this PR and it works fine, can it be approved?

@bogdannedelcu
Copy link

bogdannedelcu commented Oct 18, 2019

Let's push the fix please

1 similar comment
@rafaelferreirapt
Copy link

rafaelferreirapt commented Nov 22, 2019

Let's push the fix please

@mauhcs
Copy link

mauhcs commented Dec 3, 2019

A friendly reminder to check this PR and solve the issue in this discussion:
flutter/flutter#24865

@workerbee22
Copy link

workerbee22 commented Feb 22, 2020

@duzenko Does this now help enough for you to be able to finish doing the tests, so we can get your great work on this PR done?

@duzenko

This comment has been minimized.

Sign in to view
@duzenko

This comment has been minimized.

Sign in to view
cyanglaz
cyanglaz reviewed Feb 24, 2020
View reviewed changes
packages/google_maps_flutter/lib/src/bitmap.dart
bool mipmaps = true,
}) async {
if (configuration.devicePixelRatio != null) {
if (!mipmaps && configuration.devicePixelRatio != null) {
Copy link
Contributor

@cyanglaz cyanglaz Feb 24, 2020

Choose a reason for hiding this comment

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

!mipmaps would be false by default and we changed the behavior here right? I think you meant to set mipmaps to false by default?

This comment has been minimized.

Sign in to view

packages/google_maps_flutter/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## 0.5.21

* Breaking change fix for BitmapDescriptor scaling override
Copy link
Contributor

@cyanglaz cyanglaz Feb 24, 2020

Choose a reason for hiding this comment

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

Add an optional param mipmaps for BitmapDescriptor. fromAssetImage .

packages/google_maps_flutter/lib/src/bitmap.dart
String assetName, {
AssetBundle bundle,
String package,
bool mipmaps = true,
Copy link
Contributor

@cyanglaz cyanglaz Feb 24, 2020

Choose a reason for hiding this comment

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

Something like:
///
/// Set mipmaps to ... to allow ...., mipmaps is ... by default.

packages/google_maps_flutter/lib/src/bitmap.dart
String assetName, {
AssetBundle bundle,
String package,
bool mipmaps = true,
Copy link
Contributor

@cyanglaz cyanglaz Feb 24, 2020

Choose a reason for hiding this comment

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

Since the ecosystem might have already adopted the last change, compares to the latest code base, setting mipmaps to true would be a breaking change, right?

@tzvc
Copy link
Contributor

tzvc commented Mar 10, 2020

Any updates on this guys ? :)

@duzenko

This comment has been minimized.

Sign in to view
Chris Yang and others added 2 commits March 10, 2020 09:52
cyanglaz
cyanglaz reviewed Mar 10, 2020
View reviewed changes
packages/google_maps_flutter/google_maps_flutter/example/test_driver/google_maps_e2e.dart Outdated
Comment on lines 837 to 838
mip.toJson().forEach((dynamic x) => print(x));
scaled.toJson().forEach((dynamic x) => print(x));
Copy link
Contributor

@cyanglaz cyanglaz Mar 10, 2020

Choose a reason for hiding this comment

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

@duzenko forgot to ask, what is this test testing for? It seems to print stuff but never actually test anything?

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

Copy link
Contributor

@cyanglaz cyanglaz Mar 10, 2020

Choose a reason for hiding this comment

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

I see, thanks. Do you want to try to make it work? I have resolved all the conflicts in this PR, it should be easier to work with now.

@cyanglaz
Copy link
Contributor

cyanglaz commented Mar 20, 2020

the submit-queue check seems to be stuck. Will close this PR and reopen to rerun the check.

@cyanglaz cyanglaz closed this Mar 20, 2020
@cyanglaz cyanglaz reopened this Mar 20, 2020
@cyanglaz cyanglaz merged commit 242a261 into flutter:master Mar 20, 2020
@cyanglaz cyanglaz mentioned this pull request Mar 20, 2020
Closed
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
@duzenko @EdwinRomelta
[google_maps_flutter] Fix for bitmap scaling (flutter#1821)
736c064
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
@duzenko @jorgefspereira
[google_maps_flutter] Fix for bitmap scaling (flutter#1821)
9f8686b
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@duzenko
[google_maps_flutter] Fix for bitmap scaling (flutter#1821)
ec31997
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@amirh amirh Awaiting requested review from amirh

1 more reviewer

@cyanglaz cyanglaz cyanglaz left review comments

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.