-
-
Notifications
You must be signed in to change notification settings - Fork 25
Google Map Tiles layers #208
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.
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
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
];
};
}
}There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 itscreateSession_function. - Since we know that it is a function (if set) we can override the
getAttributionsmethod and decorate that function to modify its return value.
Let me know if that helps...
|
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".
…On Mon, May 26, 2025, 8:04 AM Symbioquine ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/control/Google/GoogleAttribution.js
<#208 (comment)>:
> @@ -0,0 +1,64 @@
+import Control from 'ol/control/Control';
+import Google from 'ol/source/Google';
+import forEachLayer from '../../utils/forEachLayer';
+
+/**
+ * @classdesc
+ * OpenLayers GoogleAttribution Control.
+ *
+ * @api
+ */
+class GoogleAttribution extends Control {
I'm pretty sure this custom control isn't needed.
Ordinarily we could set an AttributionLike
<https://openlayers.org/en/latest/apidoc/module-ol_source_Source.html#~AttributionLike>
instance on TileLayer sources (the TileImage
<https://openlayers.org/en/latest/apidoc/module-ol_source_TileImage-TileImage.html>
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?
------------------------------
On src/control/Google/GoogleAttribution.js
<#208 (comment)>:
I think we can avoid all that.
—
Reply to this email directly, view it on GitHub
<#208 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXY7QYWG3BQYAFCIZR5GJ33AMUQNAVCNFSM6AAAAABZ6GOWP2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQNRYGY4DSNZRGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
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.mp4Another 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. |
Thanks @symbioquine , merged and included here. Going to request your review to make sure I didn't goof anything.
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. |
symbioquine
left a comment
There was a problem hiding this 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...
mstenta
left a comment
There was a problem hiding this 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!!
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. |
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. |
jgaehring
left a comment
There was a problem hiding this 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
Thanks @jgaehring! Left some feedback on that PR |
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
jgaehring
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
|
thanks all! |
Closes #207