Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[google_maps_flutter] Custom marker size improvements #4055

Merged
merged 1 commit into from
Jun 5, 2024
Merged

[google_maps_flutter] Custom marker size improvements #4055

merged 1 commit into from
Jun 5, 2024

Conversation

jokerttu
Copy link
Contributor

@jokerttu jokerttu commented May 22, 2023 •
edited
Loading

This PR adds and improves marker scaling support for Android, iOS and Web.
With this change, custom marker icons are drawn with same logical size on all platforms, keeping the visual style as close as possible.

This PR is created from previous PR: flutter/plugins#6805

To avoid breaking change, this PR makes the following changes while keeping the old behavior intact:

  • Deprecates BitmapDescriptor.fromAssetImage in favor of BitmapDescriptor.asset and AssetMapBitmap
  • Deprecates BitmapDescriptor.fromBytes in favor of BitmapDescriptor.bytes and BytesMapBitmap

Android:
Android Screenshot

iOS:
iOS Screenshot

Web:
Web Screenshot

Resolves flutter/flutter#34657

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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@jokerttu jokerttu changed the title [google_maps_flutter] custom marker size [google_maps_flutter] Custom marker size improvements May 22, 2023
@jokerttu jokerttu marked this pull request as draft May 22, 2023 10:52
@jokerttu jokerttu marked this pull request as ready for review May 23, 2023 10:54
stuartmorgan
stuartmorgan requested changes May 31, 2023
View reviewed changes
packages/google_maps_flutter/google_maps_flutter/example/integration_test/google_maps_test.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/custom_marker_icon.dart Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.m Outdated Show resolved Hide resolved
@ilDeffo
Copy link

ilDeffo commented Jun 22, 2023

Any update on when this will be merged?

@jokerttu jokerttu force-pushed the google_maps_flutter_custom_marker_size branch from 616895e to 1fc2e5b Compare June 22, 2023 12:58
@jokerttu jokerttu marked this pull request as draft June 23, 2023 08:50
@jokerttu jokerttu marked this pull request as ready for review June 26, 2023 14:47
@reidbaker reidbaker removed their request for review July 14, 2023 17:09
@jokerttu jokerttu marked this pull request as draft August 16, 2023 09:02
@jokerttu jokerttu marked this pull request as ready for review August 16, 2023 18:09
stuartmorgan
stuartmorgan reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

A few minor nits, on the cross-platform parts, but generally they look good, and the native test coverage looks good now from a high level, so this should be ready for full platform reviews by @cyanglaz, @ditman, and @reidbaker.

...aps_flutter/google_maps_flutter_ios/example/ios11/ios/RunnerTests/ExtractIconFromDataTests.m Outdated Show resolved Hide resolved
...aps_flutter/google_maps_flutter_ios/example/ios13/ios/RunnerTests/ExtractIconFromDataTests.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/README.md Outdated Show resolved Hide resolved
@stuartmorgan stuartmorgan requested a review from reidbaker August 28, 2023 15:30
@jokerttu jokerttu requested a review from stuartmorgan August 31, 2023 09:50
@stuartmorgan
Copy link
Contributor

@cyanglaz, @ditman, @reidbaker: ping on the platform portions of this review.

reidbaker
reidbaker reviewed Sep 8, 2023
View reviewed changes
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated Show resolved Hide resolved
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated Show resolved Hide resolved
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated Show resolved Hide resolved
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java Outdated Show resolved Hide resolved
...le_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java Outdated Show resolved Hide resolved
...le_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java Outdated Show resolved Hide resolved
...le_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java Outdated Show resolved Hide resolved
...utter_android/android/src/test/java/io/flutter/plugins/googlemaps/MarkersControllerTest.java Outdated Show resolved Hide resolved
ditman
ditman reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

A few comments about the web, and a small rant about the _json-backed BitmapDescriptor in Dart (which I'd love to see gone if possible :))

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/README.md Outdated Show resolved Hide resolved
@gmackall
Copy link
Member

@jokerttu It looks like there are still some outstanding comments on this PR, is this something you are still actively working on?

@jokerttu
Copy link
Contributor Author

Converted as draft to avoid unnecessary reviews on this PR before platform implementations are landed.
See the platform implementation portion of this PR here: #6826

This PR will be updated and marked ready for review with only app-facing plugin changes after the platform impls are landed.

@stuartmorgan stuartmorgan added the federated: all_changes PR that contains changes for all packages for a federated plugin change label Jun 4, 2024
auto-submit bot pushed a commit that referenced this pull request Jun 4, 2024
@jokerttu
[google_maps_flutter] Custom marker size improvements - platform impls (
dcd8586 
#6826)

Platform implementations portion of : #4055

Adds platform handling for new BitmapDescriptor classes `AssetMapBitmap` and `BytesMapBitmap` introduced in #6687

Containing only changes to packages
 * `google_maps_flutter_android`
 * `google_maps_flutter_ios`
 * `google_maps_flutter_web`

Follow up PR will hold the app-facing plugin implementations.

Linked issue: flutter/flutter#34657
@jokerttu jokerttu force-pushed the google_maps_flutter_custom_marker_size branch from f497251 to 7357325 Compare June 5, 2024 09:32
@jokerttu jokerttu marked this pull request as ready for review June 5, 2024 10:03
@jokerttu
Copy link
Contributor Author

jokerttu commented Jun 5, 2024

This PR now contains only the app-facing api changes in google_maps_flutter package.

@stuartmorgan what would be correct process to deprecate the old methods under BitmapDescriptor class.

stuartmorgan
stuartmorgan approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM!

You can send me a PR to re-deprecate the other bitmap constructors, and we'll land that after landing this one. (It should be safe to land before this too, but I think it's less confusing if have already re-exported the new types.)

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md Outdated Show resolved Hide resolved
@jokerttu jokerttu force-pushed the google_maps_flutter_custom_marker_size branch from 7357325 to bce83cd Compare June 5, 2024 11:55
@jokerttu jokerttu added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2024
@auto-submit auto-submit bot merged commit 3885ac2 into flutter:main Jun 5, 2024
74 checks passed
@jokerttu jokerttu deleted the google_maps_flutter_custom_marker_size branch June 5, 2024 12:40
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2024
@engine-flutter-autoroll
3885ac2 [google_maps_flutter] Custom marker size improvements (flutte…
1e1fb31 
…r/packages#4055)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2024
@engine-flutter-autoroll
3885ac2 [google_maps_flutter] Custom marker size improvements (flutte…
d987cc2 
…r/packages#4055)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2024
@engine-flutter-autoroll
3885ac2 [google_maps_flutter] Custom marker size improvements (flutte…
a5adffc 
…r/packages#4055)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 6, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2024
@engine-flutter-autoroll
3885ac2 [google_maps_flutter] Custom marker size improvements (flutte…
6cdd276 
…r/packages#4055)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 6, 2024
@engine-flutter-autoroll
Roll Packages from 11e192a to 586faa6 (13 revisions) (#149820)
b31781a 
flutter/packages@11e192a...586faa6

2024-06-05 [email protected] [google_sign_in_web] Update button_tester to use web_only library. (flutter/packages#6868)
2024-06-05 [email protected] Roll Flutter from c246ecd to 27e0656 (17 revisions) (flutter/packages#6875)
2024-06-05 [email protected] [path_provider] Skip verifying sample file on macOS (flutter/packages#6874)
2024-06-05 [email protected] [google_maps_flutter] Custom marker size improvements (flutter/packages#4055)
2024-06-05 [email protected] [rfw] Material slider widget (flutter/packages#6610)
2024-06-04 [email protected] [ci] Manual roll Flutter to c246ecd (84 revisions) + fixes (flutter/packages#6863)
2024-06-04 [email protected] Correcting the typo of Flutter in projects (flutter/packages#6850)
2024-06-04 [email protected] [google_maps_flutter] Custom marker size improvements - platform impls (flutter/packages#6826)
2024-06-04 [email protected] Avoid cumbersome formatter workaround (flutter/packages#6573)
2024-06-04 [email protected] Clean Xcode project before analyzing and testing (flutter/packages#6842)
2024-06-03 [email protected] [pigeon] Kotlin/Java method overloading for the `setUp` method (flutter/packages#6843)
2024-06-03 [email protected] [url_launcher] Add support for setting show title on Chrome Custom Tabs (flutter/packages#6097)
2024-06-03 [email protected] Revert "Roll Flutter from c85fa6a to 7eebe29 (#6836)" (flutter/packages#6860)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] 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
@jokerttu jokerttu mentioned this pull request Jun 11, 2024
Merged
11 tasks
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@jokerttu @arc-yong
[google_maps_flutter_platform_interface] Add improvements to BitmapDe…
802ba88 
…scriptor API (flutter#6687)

This PR adds improvements to BitmapDescriptor API for google_maps_flutter_platform_interface 

* Adds better support for marker size and scaling behaviour with `AssetMapBitmap` and `BytesMapBitmap`.
* Deprecates `BitmapDescriptor.fromAssetImage` in favor of `BitmapDescriptor.asset` and `AssetMapBitmap.create`.
* Deprecates `BitmapDescriptor.fromBytes` in favor of `BitmapDescriptor.bytes` and `BytesMapBitmap`

This is prequel PR for: flutter#4055
Containing only changes to `google_maps_flutter_platform_interface` package.

Follow up PR:s will hold the platform and app-facing plugin implementations.

Linked issue: flutter/flutter#34657
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@jokerttu @arc-yong
[google_maps_flutter] Custom marker size improvements - platform impls (
9882d36 
flutter#6826)

Platform implementations portion of : flutter#4055

Adds platform handling for new BitmapDescriptor classes `AssetMapBitmap` and `BytesMapBitmap` introduced in flutter#6687

Containing only changes to packages
 * `google_maps_flutter_android`
 * `google_maps_flutter_ios`
 * `google_maps_flutter_web`

Follow up PR will hold the app-facing plugin implementations.

Linked issue: flutter/flutter#34657
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@jokerttu @arc-yong
[google_maps_flutter] Custom marker size improvements (flutter#4055)
8a2ac2e 
This PR adds and improves marker scaling support for Android, iOS and Web.
With this change, custom marker icons are drawn with same logical size on all platforms, keeping the visual style as close as possible.

This PR is created from previous PR: flutter/plugins#6805

To avoid breaking change, this PR makes the following changes while keeping the old behavior intact:
- Deprecates BitmapDescriptor.fromAssetImage in favor of BitmapDescriptor.asset and AssetMapBitmap
- Deprecates BitmapDescriptor.fromBytes in favor of BitmapDescriptor.bytes and BytesMapBitmap

**Android:**
<img width="350" alt="Android Screenshot" src="https://github.com/flutter/packages/assets/5219613/a5b9d57b-5f6e-4c17-8f60-f397b2cf21f4">

**iOS:**
<img width="350" alt="iOS Screenshot" src="https://github.com/flutter/packages/assets/5219613/5edf9fc8-91c7-4927-a158-e71ab3081b96">

**Web:**
<img width="350" alt="Web Screenshot" src="https://github.com/flutter/packages/assets/5219613/74fb2ea0-deff-4bc7-94b3-fa4aa5d6bfd5">

Resolves flutter/flutter#34657
auto-submit bot pushed a commit that referenced this pull request Jun 20, 2024
@jokerttu
[google_maps_flutter] deprecate old BitmapDescriptor methods (#6905)
5ba990b 
Deprecates `BitmapDescriptor.fromAssetImage` in favor of `BitmapDescriptor.asset` and `AssetMapBitmap.create`.
Deprecates `BitmapDescriptor.fromBytes` in favor of `BitmapDescriptor.bytes` and `BytesMapBitmap`

This is part of the implementation of the following federated plugin PR: #4055

Related to issue: flutter/flutter#34657
@reidbaker reidbaker mentioned this pull request Jul 2, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ditman ditman ditman approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@reidbaker reidbaker Awaiting requested review from reidbaker

@cbracken cbracken Awaiting requested review from cbracken

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@hellohuanlin hellohuanlin Awaiting requested review from hellohuanlin

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App federated: all_changes PR that contains changes for all packages for a federated plugin change p: google_maps_flutter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[google_maps_flutter] Add support for custom marker size
10 participants
@jokerttu @ilDeffo @stuartmorgan @gmackall @camsim99 @goderbauer @ditman @reidbaker @cyanglaz @hellohuanlin