Skip to content

Conversation

@paul121
Copy link
Member

@paul121 paul121 commented Mar 27, 2025

Closes #207

Copy link
Member Author

Choose a reason for hiding this comment

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

For Google Map Tiles attribution we are required to add the Google logo to the bottom left corner of the map. The OL Google Example does this using a very simple OL Control: https://openlayers.org/en/latest/examples/google.html

Unfortunately we need something slightly more complicated so that we only show the Google attribution when a Google base layer is selected. After seeing the rememberLayer behavior I realized we could listen to layer changes and react accordingly to toggle visibility of the Google logo.

So I extended the OL control example to include some logic that listens for when base layers are added & toggled on the map. Any layer that has a Google source will display the Google logo when turned on; all other base layers will hide the Google logo when turned on. I found this was easier than only reacting to on+off changes of the Google base layers themselves because there are race conditions to consider with the order the on/off events are fired (in my testing I saw the events could be fired multiple times?)

I implemented this as a default control that will be enabled on all farmOS-maps. This means that users of farmOS-map don't need to worry about attribution, it will always work regardless of the Google source layer they add. If for some reason they want to manage attribution themselves, they could configure the map controls to not use defaults. Ideally we would only add this when Google layers are being added... but it's pretty simple and shouldn't affect other layers anyways? Maybe there is a way to be more clever but here is a first pass to keep things simple.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can avoid all that.

@paul121 paul121 changed the base branch from 2.x to main April 1, 2025 18:09
@paul121 paul121 marked this pull request as ready for review April 1, 2025 18:09
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can avoid all that.

*
* @api
*/
class GoogleAttribution extends Control {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this custom control isn't needed.

Ordinarily we could set an AttributionLike instance on TileLayer sources (the TileImage base class accepts an attributions constructor arg) and then OpenLayers takes care of showing/hiding the attributions when the layers are visible (I believe even honoring extents and stuff).

But from skimming the sources, it looks like they went out of their way not to allow passing attributions through. I think that's because attribution might already be built-in: https://github.com/openlayers/openlayers/blob/df3117097e5dbd748132bdd438401363d7eb116f/src/ol/source/Google.js#L261-L306

Have you tested this change to see if we get the attributions functionality out of the box?

Copy link
Collaborator

@symbioquine symbioquine May 27, 2025

Choose a reason for hiding this comment

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

Have you tested this change to see if we get the attributions functionality out of the box?

Yes, we do get the dynamic image source data attribution out of the box. These attributions require some logic and API calls since the image sources change while you move on the map. So it's nice we don't need to implement this!

What we don't get, however, is the Google logo in the bottom left corner. This is required in the Maps API Policies. You can see the in the google maps example that they add the Google logo as a "static" control in a separate step. This is fine for the example, but doesn't take into account if the Google layer is turned on or off and always displays the logo https://openlayers.org/en/latest/examples/google.html

Maybe calling this control "GoogleAttribution" is a little misleading. I see the example did call it "GoogleLogoControl".

Ah, I see what you mean... the provided implementation uses OpenLayers' attributions functionality internally to provide some of the required "attribution" - per the Maps API Policies. However, in doing so it makes it harder to use the OpenLayers' attributions functionality to comply with the remaining policy requirement.

That said, I'm a bit nervous with the proposed implementation as-is. There's just a bunch of cases to keep track of in terms of layer visibility/existence - i.e. it's perfectly valid to remove and re-add layers to the map directly via the OpenLayers API.

I'd prefer if we let OpenLayers handle the attribution (including the logo) display since I think that pattern should cover all relevant scenarios much more automatically.

I wonder if something like this would work?

/**
 * The attribution containing the Google logo image.
 * @const
 * @type {string}
 */
const LOGO_ATTRIBUTION =
  '<img style="pointer-events: none; position: absolute; bottom: 5px; left: 5px;" ' +
  'src="https://developers.google.com/static/maps/documentation/images/google_on_white.png">'';

class GoogleSourceWithLogo extends Google {
  constructor(options) {
    super(options);
  }

  /**
   * Return a decorated version of the base Google attributions function that also adds
   * the Google logo as required by the Maps API Policies.
   * @return {function} The decorated attributions function.
   * @override
   */
  getAttributions() {
    // The parent class returns a function which we will decorate (if set)
    // https://github.com/openlayers/openlayers/blob/df3117097e5dbd748132bdd438401363d7eb116f/src/ol/source/Google.js#L261
    const superAttribFn = super.getAttributions();
    if (!superAttribFn) {
      return (frameState) => [LOGO_ATTRIBUTION];
    }

    return (frameState) => {
      // This should be a single text string
      // See https://developers.google.com/maps/documentation/tile/streetview#metadata_response
      // and https://github.com/openlayers/openlayers/blob/df3117097e5dbd748132bdd438401363d7eb116f/src/ol/source/Google.js#L303C28-L303C37
      const superAttrib = superAttribFn(frameState);
      return [
        superAttrib,
        LOGO_ATTRIBUTION,
      ];
    };
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer if we let OpenLayers handle the attribution (including the logo) display since I think that pattern should cover all relevant scenarios much more automatically.

I agree this would be ideal!

I wonder if something like this would work?

That is interesting. Basically just hacking how OL uses the image source attribution text area to wedge in an additional <img> tag? Seems like that could work. This could probably be an upstream contribution :-)

I guess I just worry about what you mentioned earlier, that the included Google source might not allow passing in/modifying the attributions? I'm not sure how getAttributions() works here, would we need to extend the fetchAttributions_ function instead?

Copy link
Collaborator

@symbioquine symbioquine May 28, 2025

Choose a reason for hiding this comment

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

This could probably be an upstream contribution :-)

If you look at the PR that adds Google maps support, they left the logo part out because they didn't want to choose between the white/black/high-dpi versions of the logo on behalf of the application.

I guess I just worry about what you mentioned earlier, that the included Google source might not allow passing in/modifying the attributions?

It doesn't provide the ability to pass in additional attributions, but it can't "not allow" modifying them as my (untested 🤪) proposal demonstrates.

I'm not sure how getAttributions() works here, would we need to extend the fetchAttributions_ function instead?

I tried to include references to why my proposal should work as inline comments, but I can expand on that here:

  • The Source base class provides a getter/setter for the "attributions" property of a layer "source". If it's not a function (and truthy), it gets coerced into one in the setter.
  • The Google implementation sets this to its fetchAttributions_ method as part of its createSession_ function.
  • Since we know that it is a function (if set) we can override the getAttributions method and decorate that function to modify its return value.

Let me know if that helps...

@paul121
Copy link
Member Author

paul121 commented May 26, 2025 via email

@symbioquine
Copy link
Collaborator

Following up from our dev call this last Thursday, I tested my proposed strategy... The general idea does work - with a bit of fancy CSS it is possible to make the logo permanently fixed in the bottom left corner of the map like this pull request already does.

That said, I think we can do better... Having it permanently fixed in the bottom left appears to technically meet the Google Maps API policies, but I think it would be more in the spirit of the policies to obscure the logo with our controls as little as possible.

Unfortunately, the existing OpenLayers attribution API is really only intended to display attributions in one place on the map at a time and the fancy CSS I got working also prevents the logo interacting as cleanly with the sidebar or other controls.

All that together brought me back to using a custom control like @paul121 originally proposed, but with a slightly improved event handling logic and some improvements to the naming and styling.

I've opened a PR to include my changes in @paul121's branch here: paul121#1

Google.Maps.Logo.Attribution.2025-05-31.11-14.mp4

Another issue that we probably need to address before this can be merged is making sure the Google attributions and logo appear in the output of the snapshot functionality.

@paul121
Copy link
Member Author

paul121 commented Jun 10, 2025

All that together brought me back to using a custom control like @paul121 originally proposed, but with a slightly improved event handling logic and some improvements to the naming and styling.

Thanks @symbioquine , merged and included here. Going to request your review to make sure I didn't goof anything.

Another issue that we probably need to address before this can be merged is making sure the Google attributions and logo appear in the output of the snapshot functionality.

Ah interesting. I'm afraid that the screenshot functionality only includes content from the canvas so this might be challenging. We should probably include attributions other layer sources, too. Hmm.

@paul121 paul121 requested a review from symbioquine June 10, 2025 00:11
@symbioquine symbioquine requested review from jgaehring and mstenta June 11, 2025 18:10
Copy link
Collaborator

@symbioquine symbioquine left a comment

Choose a reason for hiding this comment

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

LGTM - but I'm also now a partial author of these changes. Added @mstenta & @jgaehring since we should have another set of eyes on this...

Copy link
Member

@mstenta mstenta left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @paul121 and @symbioquine!!

@symbioquine
Copy link
Collaborator

symbioquine commented Jun 11, 2025

All that together brought me back to using a custom control like @paul121 originally proposed, but with a slightly improved event handling logic and some improvements to the naming and styling.

Thanks @symbioquine , merged and included here. Going to request your review to make sure I didn't goof anything.

Another issue that we probably need to address before this can be merged is making sure the Google attributions and logo appear in the output of the snapshot functionality.

Ah interesting. I'm afraid that the screenshot functionality only includes content from the canvas so this might be challenging. We should probably include attributions other layer sources, too. Hmm.

Agreed that it looks challenging: https://stackoverflow.com/questions/12652769/rendering-html-elements-to-canvas

There may actually be security implications to allowing that (in general) which may make it likely to be impossible.

@jgaehring
Copy link
Member

Added @mstenta & @jgaehring since we should have another set of eyes on this...

LGTM. I really don't know the codebase very well these days, but with that in mind, it struck me worthwhile to add a bit more detail to the README concerning the API keys, since that seemed to be one of the main takeaways from yesterday's conversation. I couldn't commit straight to your branch, @paul121, but just opened a PR on it.

Copy link
Member

@jgaehring jgaehring left a comment

Choose a reason for hiding this comment

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

See PR paul121#2

@symbioquine
Copy link
Collaborator

See PR paul121#2

Thanks @jgaehring! Left some feedback on that PR

jgaehring and others added 3 commits June 13, 2025 12:33
Note that the example API key is the one displayed in the official
Google tutorial video, the same one linked in the documentation:
https://www.youtube.com/watch?v=2_HZObVbe-g&t=72s
Copy link
Member

@jgaehring jgaehring left a comment

Choose a reason for hiding this comment

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

🚀

@paul121 paul121 merged commit e24586c into farmOS:main Jun 13, 2025
2 checks passed
@paul121 paul121 deleted the 2.x-google branch June 13, 2025 19:41
@paul121
Copy link
Member Author

paul121 commented Jun 13, 2025

thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add support for Google Map Tiles layers

4 participants