[google_maps_flutter] Issue165999 - add `onActiveLevelChanged` callback to `GoogleMap` #8923

ChopinDavid · 2025-03-28T13:17:52Z

This PR would introduce a listener on GoogleMap that notifies anytime the active floor changes on a building when the GoogleMap is indoorViewEnabled. This includes handling a null level when a building is unfocused.

Sample code:

String? _selectedFloor;

@override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(_selectedFloor != null
            ? 'Selected floor: $_selectedFloor'
            : 'No floor selected'),
      ),
      body: GoogleMap(
        onActiveLevelChanged: (selectedLevel) {
          setState(() {
            _selectedFloor = selectedLevel?.shortName;
          });
        },
      ),
    );
  }

This would fix #165999

Pre-Review Checklist

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

Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

ChopinDavid · 2025-03-28T13:19:31Z

I still need to update CHANGELOG, update versions, write new and run existing tests, but wanted to get some eyes on this. Will probably try to do some of the outstanding tasks over this weekend.

I am also curious whether adding this to google_maps_flutter_web would be required for the PR to be merged. I've not worked with flutter web before, but could give it a shot.

stuartmorgan-g · 2025-03-28T13:34:05Z

Thanks for the contribution! I did an initial skim of the app-facing and platform interface package changes, and the structure all looks good.

I am also curious whether adding this to google_maps_flutter_web would be required for the PR to be merged.

No, this would be fine to land without web support, and that be left as a follow-up issue for someone else to implement. It's clear that web wouldn't change the core design of this API (which is the main potential concern, per our policy), and having the callback not fire on web wouldn't be fundamentally confusing in the same way that, say, a significant new API throwing UnsupportedError on web could be.

stuartmorgan-g · 2025-03-28T13:34:49Z

packages/google_maps_flutter/google_maps_flutter/pubspec.yaml

+    git:
+      url: git@github.com:ChopinDavid/flutter_packages.git
+      ref: Issue165999
+      path: packages/google_maps_flutter/google_maps_flutter_platform_interface


Please use our tooling for multi-package changes, as our CI will recognize it and adjust some checks accordingly.

Yes, this was one of the changes I was going to make this weekend. Due to the structure of the packages, and the way they all depend on eachother, we had to sort of use these git references for some dependencies to get things building.

Due to the structure of the packages, and the way they all depend on eachother, we had to sort of use these git references for some dependencies to get things building.

That's exactly what make-deps-path-based is designed for, but without using git references.

ChopinDavid · 2025-03-28T13:39:13Z

@stuartmorgan I also noticed that there were a lot of changes in packages/google_maps_flutter/google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Messages.java that were introduced by my running of google-java-format. Is it the case that this file just wasn't properly formatted in previous PRs, or does it seem I used this formatter incorrectly?

I basically ran

brew install google-java-format
find . -name "*.java" | xargs google-java-format -I

stuartmorgan-g · 2025-03-28T14:30:19Z

Is it the case that this file just wasn't properly formatted in previous PRs

Our CI enforces (with a few rare exceptions) autoformatting; what is checked in is by definition correct, since otherwise the PR wouldn't have passed CI.

I basically ran

brew install google-java-format
find . -name "*.java" | xargs google-java-format -I

You should run the repo tooling format command, which ensures that the correct Java formatter is installed, and calls it with the correct arguments.

ChopinDavid · 2025-03-31T13:39:26Z

@stuartmorgan-g I think this PR is in a pretty good place now, but please let me know. I noticed in the contributing README that "changes that span multiple packages will need to be done in multiple PRs", so should this PR be closed and should individual PRs be made for each of the 4 changed packages?

stuartmorgan-g · 2025-03-31T14:41:30Z

I noticed in the contributing README that "changes that span multiple packages will need to be done in multiple PRs", so should this PR be closed and should individual PRs be made for each of the 4 changed packages?

The PR needs to follow the step by step process that is described in the paragraph right after the sentence you quoted. Step 1 hasn't been completed yet, which is why this is failing all the CI checks.

…_maps_flutter_ios - DC

… be path-based - DC

ChopinDavid · 2025-04-04T19:37:59Z

It sounds like this PR isn't responsible for ensuring the tree-status check passes, does that track?

stuartmorgan-g · 2025-04-08T17:28:41Z

It sounds like this PR isn't responsible for ensuring the tree-status check passes, does that track?

Yes, that's correct. tree-status is a live check tracking the state of the tree (currently red due to some frequently-occurring flake we're working on).

ChopinDavid · 2025-04-09T13:43:26Z

@stuartmorgan-g cool, in that case I believe this story is ready for review once the tree status is addressed. Please let me know if that is incorrect.

stuartmorgan-g

I don't see any testing of the actual implementations, only the Dart passthroughs. The native code should have native unit tests, and if possible (it looks like both the Android and iOS SDKs have a way of programmatically setting building levels), end-to-end integration testing.

On the API itself, after looking at the implementations here and the native SDKs: levels are a component of buildings, and yet this is only reflecting levels; what's the reason for that? It seems like the API would make a lot more sense if it included buildings and the building-change notifications as well.

stuartmorgan-g · 2025-04-17T17:39:37Z

packages/google_maps_flutter/google_maps_flutter/pubspec.yaml

@@ -17,13 +17,14 @@ flutter:
        default_package: google_maps_flutter_ios
      web:
        default_package: google_maps_flutter_web
+publish_to: none


This needs to be removed, as it is disabling some CI checks.

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

stuartmorgan-g · 2025-04-17T18:11:56Z

...tter_android/android/src/main/java/io/flutter/plugins/googlemaps/TileProviderController.java

@@ -54,7 +54,8 @@ Tile getTile() {
          new Messages.PlatformPoint.Builder().setX((long) x).setY((long) y).build();
      handler.post(() -> flutterApi.getTileOverlayTile(tileOverlayId, location, (long) zoom, this));
      try {
-        // `flutterApi.getTileOverlayTile` is async, so use a `countDownLatch` to make it synchronized.
+        // `flutterApi.getTileOverlayTile` is async, so use a `countDownLatch` to make it


Why are there formatting changes in a bunch of unrelated files?

I can resolve these. Not sure, could've ran the wrong formatter maybe? But I figured a CI check would fail if I had.

Code formatters generally don't remove line breaks in comments, so adding extra line breaks won't fail CI.

stuartmorgan-g · 2025-04-17T18:15:01Z

...s/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/indoor_level.dart

+
+/// Describes a single level in a building.
+///
+/// Multiple buildings can share a level - in this case the level instances will compare as equal,


What code in the class below makes this comment true?

stuartmorgan-g · 2025-04-17T18:16:13Z

...ps_flutter_platform_interface/lib/src/method_channel/method_channel_google_maps_flutter.dart

@@ -167,6 +167,12 @@ class MethodChannelGoogleMapsFlutter extends GoogleMapsFlutterPlatform {
    return _events(mapId).whereType<MapLongPressEvent>();
  }

+  @override
+  Stream<MapActiveLevelChangedEvent> onActiveLevelChanged(


The changes to this file should be reverted; the method channel code is only for compatibility with any legacy third-party implementations, and by definition legacy implementations won't have support for new features.

' from pubspec - DC

…to be more simple - DC

ash2moon · 2025-04-29T18:34:06Z

@ChopinDavid are you still waiting on something or can this just be rebased and reviewed?

ChopinDavid · 2025-04-30T15:03:57Z

@ash2moon Sorry, been meaning to get around to @stuartmorgan-g's other comments but have been having a hard time finding free time to do so. I believed this PR was ready to review, but it sounds like Stuart had some concerns so I'd hold off from merging until I address those, but I'd say feel free to review. As much feedback as possible is appreciated.

ChopinDavid · 2025-05-17T14:50:31Z

On the API itself, after looking at the implementations here and the native SDKs: levels are a component of buildings, and yet this is only reflecting levels; what's the reason for that? It seems like the API would make a lot more sense if it included buildings and the building-change notifications as well.

@stuartmorgan-g
I see that on the Android side, we are tying into both onIndoorBuildingFocused and onIndoorLevelActivated. This is probably what prompted the suggestion to reflect the indoor building as well as the indoor level.

The idea originally was that, at least on the iOS side, the only native methods we are tying into are reflecting changes in the indoor level and not the indoor building. I thought we were doing the same on Android, but now see we are for some reason tying into both building and floor listeners. I will try to make it so that we do not need to override onIndoorBuildingFocused as this was not the original intention of this PR.

I would think this PR makes sense to merge if we can get it to a point where we are only tying into native methods reflecting changes in indoor levels and not buildings as well. If I can make it so we no longer tie into onIndoorBuildingFocused in GoogleMapController.java, I would think it makes sense to move forward with only passing the indoor level object to the dart-side methods.

If it is the case that we do, in fact, need to continue to tie into onIndoorBuildingFocused to accurately reflect all changes in indoor levels on the Android side, it probably does make sense to have buildings being reflected be a requirement for this PR being merged. That being said, this fix no longer represents a need for my company and I will have very little bandwidth to make those changes.

I'm curious what others think re: whether we move this forward if we can make it so that we are no longer tying into native methods reflecting indoor building changes. If we don't think this is the right path forward, I'm afraid this PR will more than likely sit in an open state for a long time without contributions from others. As you can see, its taken me 3 weeks to find the time to even take another look at these requested changes and I'm still trying to work through them.

vashworth · 2025-05-20T02:33:38Z

The idea originally was that, at least on the iOS side, the only native methods we are tying into are reflecting changes in the indoor level and not the indoor building.

It seems weird to me to only partially implement the GMSIndoorDisplayDelegate. However, I understand that's all you need for your particular use case.

From the iOS side, other than it being confusing on why we don't implement the entire GMSIndoorDisplayDelegate, I don't see anything inherently wrong with this.

As mentioned before, though, this would need more tests.

stuartmorgan-g · 2025-05-21T18:29:11Z

I would think this PR makes sense to merge if we can get it to a point where we are only tying into native methods reflecting changes in indoor levels and not buildings as well.

Could you elaborate (either here or in the issue) on what your use case is? I'm trying to understand under what circumstances it's useful to know what floor of a building is being displayed without knowing what the building is.

vashworth · 2025-06-11T21:09:41Z

@ChopinDavid Are you still working on this?

ChopinDavid requested review from cbracken, reidbaker and stuartmorgan-g as code owners March 28, 2025 13:17

github-actions bot added p: google_maps_flutter platform-android platform-ios labels Mar 28, 2025

ChopinDavid force-pushed the Issue165999 branch 4 times, most recently from 96babe0 to 7f32eeb Compare March 28, 2025 13:32

stuartmorgan-g reviewed Mar 28, 2025

View reviewed changes

ChopinDavid requested a review from hellohuanlin as a code owner March 31, 2025 13:01

github-actions bot added p: camera platform-macos p: webview_flutter labels Mar 31, 2025

ChopinDavid force-pushed the Issue165999 branch 2 times, most recently from 02def77 to 4eeb99f Compare March 31, 2025 13:38

ChopinDavid requested a review from ditman as a code owner April 1, 2025 00:48

github-actions bot added the platform-web label Apr 1, 2025

ChopinDavid force-pushed the Issue165999 branch 2 times, most recently from c778e30 to d4dbf52 Compare April 1, 2025 01:46

github-actions bot removed the p: webview_flutter label Apr 1, 2025

ChopinDavid force-pushed the Issue165999 branch 2 times, most recently from 77c8523 to fe385e3 Compare April 1, 2025 02:29

ChopinDavid added 8 commits April 4, 2025 14:24

Upped version numbers and added CHANGELOG entires - DC

98fd0e2

Added example - DC

36bf1e7

Ran make-deps-path-based - DC

157c628

Fixed analyzer warnings - DC

b648088

Added missing license blocks - DC

d917916

Loosened google_maps_flutter_platform_interface constraints on google…

917eb3b

…_maps_flutter_ios - DC

Converted google_maps_flutter_web's google_maps_flutter dependency to…

9140f43

… be path-based - DC

Added mockFloorController to GoogleMapControllerTest.java - DC

54af461

ChopinDavid force-pushed the Issue165999 branch from 9f4b679 to 54af461 Compare April 4, 2025 19:24

github-actions bot added the platform-web label Apr 4, 2025

Reverted changes to google_maps_flutter_web - DC

9278285

github-actions bot removed the platform-web label Apr 4, 2025

Merge branch 'main' into Issue165999

bd32f0c

stuartmorgan-g removed the request for review from ditman April 17, 2025 17:36

stuartmorgan-g added triage-ios Should be looked at in iOS triage triage-android Should be looked at in Android triage labels Apr 17, 2025

stuartmorgan-g requested changes Apr 17, 2025

View reviewed changes

ChopinDavid added 2 commits April 20, 2025 19:21

Removed 'publish_to: none

9801989

' from pubspec - DC

Updated onIndoorBuildingFocused override in GoogleMapController.java …

0dbba40

…to be more simple - DC

jmagman requested review from vashworth and removed request for hellohuanlin May 15, 2025 21:17

[google_maps_flutter] Issue165999 - add onActiveLevelChanged callback to GoogleMap #8923

Are you sure you want to change the base?

[google_maps_flutter] Issue165999 - add onActiveLevelChanged callback to GoogleMap #8923

Uh oh!

Conversation

ChopinDavid commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-Review Checklist

Footnotes

Uh oh!

ChopinDavid commented Mar 28, 2025

Uh oh!

stuartmorgan-g commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid commented Mar 28, 2025

Uh oh!

stuartmorgan-g commented Mar 28, 2025

Uh oh!

ChopinDavid commented Mar 31, 2025

Uh oh!

stuartmorgan-g commented Mar 31, 2025

Uh oh!

ChopinDavid commented Apr 4, 2025

Uh oh!

stuartmorgan-g commented Apr 8, 2025

Uh oh!

ChopinDavid commented Apr 9, 2025

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash2moon commented Apr 29, 2025

Uh oh!

ChopinDavid commented Apr 30, 2025

Uh oh!

ChopinDavid commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vashworth commented May 20, 2025

Uh oh!

stuartmorgan-g commented May 21, 2025

Uh oh!

vashworth commented Jun 11, 2025

Uh oh!

Uh oh!

[google_maps_flutter] Issue165999 - add `onActiveLevelChanged` callback to `GoogleMap` #8923

[google_maps_flutter] Issue165999 - add `onActiveLevelChanged` callback to `GoogleMap` #8923

ChopinDavid commented Mar 28, 2025 •

edited

Loading

ChopinDavid commented May 17, 2025 •

edited

Loading