GoogleMaps support

[mdastous-bentley]

New imagery format / proviter to support Google maps 2D tiles:

More details available in NextVersion.md.

[mdastous-bentley]

@pmconne Part of this PR, I need to have an async version of TileTreeReference.addLogoCards, mainly because a request need to Google web API is required to retrieve copyright strings associated with selected tiles. Currently it's being invoked through Viewport.forEachTileTreeRef, which is not async.

My only solution right now, would consist in created an async version of the forEach methods, but I feel like it will result in a ugly API.

Any thoughts?

[pmconne]

My only solution right now, would consist in created an async version of the forEach methods, but I feel like it will result in a ugly API.

The forEach methods are a relic of the days when we were still learning how to do TypeScript.
I'd suggest supplanting them with synchronous method(s) that return Iterable<TileTreeReference>. Then the caller can worry about calling async methods on the references, halting iteration partway through, doing something with the return value of whatever he's calling on the references, etc etc.

[mdastous-bentley]

@pmconne @aruniverse @matmarchand Looking for feedback on the actual implementation state:

1. The new maplayer format / provider was added to @itwin/map-layers-formats package. Meaning, the package has to be installed in your iTwin application to get GoogleMaps support.
2. Also, there's no new value built-in value for BackgroundMapProviderName to use with BaseMapLayerSettings.fromProvider
3. Introduction of a new properties field on MapLayerSettings to pass provider specific properties, which will be part of the display style serialization.
4. An utility GoogleMaps object is provided (could also be merged on GoogleImageryFormat class), is provided to easily create MapLayerSettings from GoogleMaps API parameters (i.e. you don't have to provide the property bag or the service URL by hand).
5. We don't provided any GoogleMaps API key, each product will have to provider their own.

[mdastous-bentley]

@pmconne An async version of TileTreeReference.addLogoCards will be needed, what strategy do you suggest to replace an existing public API?

[pmconne]

An async version of TileTreeReference.addLogoCards will be needed, what strategy do you suggest to replace an existing public API?

The approach you've taken (deprecate it, add new addAttributions replacement) looks ok on the face of it but will introduce bugs. Existing implementations (outside of itiwnjs-core, where you've added the new method) will not have overridden the new method, so if you update callers of addLogoCards to call addAttributions instead, suddenly some attributions will go missing.

The simplest solution would be to change addLogoCards' return type to void | Promise<void> but that's a breaking change.

All but one of your addAttributions methods is just a copy-paste of addLogoCards. You could instead do something like:

class TileTreeReference {
  // Everyone who provides attributions today overrides this method.
  /** @deprecated in 5.0. Use [[addAttributions]] instead. */
  public addLogoCards(_cards: HTMLTableELement, _vp: ScreenViewport): void { }

  // People who need async attributions, or those reacting to deprecation warnings, would override this method instead of addLogoCards.
  protected addAttributions(cards: HTMLTableElement, vp: ScreenViewport): Promise<void> {
    return Promise.resolve(this.addLogoCards(cards, vp);
  }
}

[mdastous-bentley]

An async version of TileTreeReference.addLogoCards will be needed, what strategy do you suggest to replace an existing public API?

The approach you've taken (deprecate it, add new addAttributions replacement) looks ok on the face of it but will introduce bugs. Existing implementations (outside of itiwnjs-core, where you've added the new method) will not have overridden the new method, so if you update callers of addLogoCards to call addAttributions instead, suddenly some attributions will go missing.

The simplest solution would be to change addLogoCards' return type to void | Promise<void> but that's a breaking change.

All but one of your addAttributions methods is just a copy-paste of addLogoCards. You could instead do something like:

class TileTreeReference {
  // Everyone who provides attributions today overrides this method.
  /** @deprecated in 5.0. Use [[addAttributions]] instead. */
  public addLogoCards(_cards: HTMLTableELement, _vp: ScreenViewport): void { }

  // People who need async attributions, or those reacting to deprecation warnings, would override this method instead of addLogoCards.
  protected addAttributions(cards: HTMLTableElement, vp: ScreenViewport): Promise<void> {
    return Promise.resolve(this.addLogoCards(cards, vp);
  }
}

Thanks for the suggestion, I totally agree too much code duplication in previous approach.

[pmconne]

Looking for feedback on the actual implementation state

I guess the user interface is going to have to expose the provider-specific config options (property bag)? Should the provider expose a schema describing the available properties and their types?

I note you've basically reintroduced the equivalent of BackgroundMapProps.providerData which was originally a bag of provider-specific config properties, going back to MicroStation. That's probably where the "map type" belongs (and where it used to live), since each provider can have its own set of map types (or only one, and no need to specify), but unfortunately that'd be a breaking change.

[mdastous-bentley]

Looking for feedback on the actual implementation state

I guess the user interface is going to have to expose the provider-specific config options (property bag)? Should the provider expose a schema describing the available properties and their types?

Currently it's the job of the GoogleMaps object: it provides an interfaced name CreateGoogleMapsSessionOptions, and utility methods such as createPropertiesFromSessionOptions and createMapLayerSettings so that you never have to deal directly with the properties bag.

It doesn't offer though, a set of abstract types that could be shared among future providers interested in using properties bag.

[mdastous-bentley]

@pmconne There are function inside GoogleMaps.ts that I would to keep private, meaning it should not be discoverable in the typing. Do we have any utilities in this repo to achieve that?

[pmconne]

@pmconne There are function inside GoogleMaps.ts that I would to keep private, meaning it should not be discoverable in the typing. Do we have any utilities in this repo to achieve that?

Guidelines for internal APIs. Is this what you mean?

[mdastous-bentley]

@pmconne There are function inside GoogleMaps.ts that I would to keep private, meaning it should not be discoverable in the typing. Do we have any utilities in this repo to achieve that?

Guidelines for internal APIs. Is this what you mean?

Not quite, @internal items are still visible when consuming the API, which I find rather confusing for our API consumers.

[pmconne]

@internal items are still visible when consuming the API

Only if you export them from the package.
Be more specific about which APIs you are trying to keep private, and what you mean by "private" (only accessible in the same package? only in monorepo? only for other special repos?)

[mdastous-bentley]

@aruniverse @eringram One of you need to approve I think

[aruniverse]

newly added beta apis exposed to users seem fine to me in map-layers-format

i think a lot is left to be desired here for formatting of the code but trying to not be picky, and hope we come back to it later

[mdastous-bentley]

newly added beta apis exposed to users seem fine to me in map-layers-format

i think a lot is left to be desired here for formatting of the code but trying to not be picky, and hope we come back to it later

Any example of what you are referring to ?

[mdastous-bentley]

@Mergifyio backport release/4.11.x

[mergify]

backport release/4.11.x

✅ Backports have been created

Details

• #7779 GoogleMaps support (backport #7604) [release/4.11.x] has been created for branch release/4.11.x but encountered conflicts

[aruniverse]

bad copy and paste,

Suggested change

	* IMJS_BING_MAPS_KEY
	* IMJS_GOOGLE_MAPS_KEY