Comments (10)
@fnicollet Thanks for the in-depth analysis and writeup. Do you think you'd be able to provide at least some of the suggested improvements as pull request? I agree with all of them. Our attribution code can handle viewport dependent attributions already, it is used for the Bing Maps source.
from openlayers.
Sure, I haven't contributed to OpenLayers just yet, but i'll try to come up with a PR
from openlayers.
@ahocevar I need a bit of advice on how to do this PR please.
For the missing logo, I added in custom control ("GoogleLogoControl", which extends Control) to the google.js example, so that devs can use whichever image is best for their basemap (color logo or the white one for darker backgrounds), instead of adding a custom control to the OL Google source, which would imply bundling the logo and I don't think it's worth adding to OL's bundle size just for this.
But at least, it will give a good example of how to add the Google logo to the map, directly in the example page, ready to be copy-pasted.
For the attribution, I cannot figure out how to listen to the "moveend" event from the Google source. It doesn't seem to have a back reference to the View, and the getView function returns a Promise ... which is never fulfilled.
I tried looking at the Bing Maps Source code, but the attribution is only retrieved on startup, and not when the extent changes, as it seems not to depend on the current viewport.
The only way to retrieve the map instance would be to pass it to the constructor, but that seems like a bad practice. Handling the "moveend" in the dev's own source and calling a function on the source seems like a bad idea (it should be handled by default).
Also, I thought I would add a "debounce" on the moveend handler, but there doesn't seem to be a debounce util function already in the OL code
How would you approach this issue?
from openlayers.
Thanks for your effort on this, @fnicollet!
There is no need to listen to moveend
for the attributions. If you look at the BingMaps
source, you'll see that setAttributions()
is called with a function that expects a FrameState
as argument and returns an array of attributions. That function will be called on every render frame, and you can use frameState.extent
to get the extent, and all the view properties from frameState.viewState
. You won't need a debounce
. Instead, check frameState.animate
, and only send a request for attributions when frameState.animate === false
. This way, the attributions will only be updated when animations and user interactions with the map have stopped.
Regarding the logo, your approach makes sense for now.
from openlayers.
@ahocevar Thanks for the hints !
I almost have something working but got two things which are not working.
First, I check the "animate" boolean on the frameState, but it is not really what I expected. Like if I pan the map, I will get many "animate=false" calls, even though i never "released" the mouse. Same for a zoom in/out, I get many calls to the function with animate=false, so it turns into many XHR calls (not sure they counted in Google's API quota, but you never know ...).
If possible, I would like to only have a call when the map is "resting", just like the "moveend" event, that would reduce the number of calls a lot.
Then, I have a bigger problem with the fact that the setAttributions callback expects a "synchronous" response, and I have to make an (async) XHR call. I don't think I can use async/await here as when I try to "await" the XHR call, I get an error:
Cannot use keyword 'await' outside an async function
And I tried messing about and passing and async function to the setAttribution function, but then I get as an attribution:
As sychronously, a Promise is what is returned by the async function.
The documentation:
https://openlayers.org/en/latest/apidoc/module-ol_source_Source.html#~AttributionLike
States that you can return a function, but I think that means a "synchronous" function (for formatting), not an async one.
I looked in the rest of the source code, but it seems like the setAttributions callback is always synchronous.
Do you have an idea on how I could proceed ?
Thanks !
from openlayers.
Yes you're right, currently that function needs to return the attributions and not a promise thereof. However, this could be easily changed. We have a ol/functions.toPromise()
helper function which can be used to make this whole process work with async functions in ol/control/Attribution
:
--- a/src/ol/control/Attribution.js
+++ b/src/ol/control/Attribution.js
@@ -6,6 +6,7 @@ import EventType from '../events/EventType.js';
import {CLASS_COLLAPSED, CLASS_CONTROL, CLASS_UNSELECTABLE} from '../css.js';
import {equals} from '../array.js';
import {removeChildren, replaceNode} from '../dom.js';
+import {toPromise} from '../functions.js';
/**
* @typedef {Object} Options
@@ -215,7 +216,7 @@ class Attribution extends Control {
* @private
* @param {?import("../Map.js").FrameState} frameState Frame state.
*/
- updateElement_(frameState) {
+ async updateElement_(frameState) {
if (!frameState) {
if (this.renderedVisible_) {
this.element.style.display = 'none';
@@ -224,7 +225,11 @@ class Attribution extends Control {
return;
}
- const attributions = this.collectSourceAttributions_(frameState);
+ const attributions = await Promise.all(
+ this.collectSourceAttributions_(frameState).map((attribution) =>
+ toPromise(() => attribution),
+ ),
+ );
const visible = attributions.length > 0;
if (this.renderedVisible_ != visible) {
It might be more efficient to do the toPromise()
conversion earlier (in ol/Source
), but the above should do just fine in a first iteration.
Regarding frameState.animate
, to achieve what you want (i.e. only request attribution when the map has settled), use this instead:
if (!frameState.viewHints[ViewHint.ANIMATING] &&
!frameState.viewHints[ViewHint.INTERACTING]) {
// request attribution
}
from openlayers.
Thanks ! i'll look into that. Should the changes to Attribution.js be part of the same PR ? Because it might have a bigger impact
from openlayers.
Thanks ! i'll look into that. Should the changes to Attribution.js be part of the same PR ? Because it might have a bigger impact
I'll leave this decision up to you. I'm fine either way. If the diff above is applied, there is a single test that will fail, but it's fine to just fix that test because it doesn't break any advertised API.
--- a/test/browser/spec/ol/control/attribution.test.js
+++ b/test/browser/spec/ol/control/attribution.test.js
@@ -72,10 +72,15 @@ describe('ol.control.Attribution', function () {
map = null;
});
- it('does not add duplicate attributions', function () {
+ it('does not add duplicate attributions', function (done) {
map.renderSync();
- const attribution = map.getTarget().querySelectorAll('.ol-attribution li');
- expect(attribution.length).to.be(2);
+ setTimeout(() => {
+ const attribution = map
+ .getTarget()
+ .querySelectorAll('.ol-attribution li');
+ expect(attribution.length).to.be(2);
+ done();
+ }, 0);
});
it('renders attributions as non-collapsible if source is configured with attributionsCollapsible set to false', function () {
from openlayers.
@ahocevar I created the PR :
#15598
Thanks for all the advice on this!
from openlayers.
Thanks for reporting this. I'll close as fixed by #15598.
from openlayers.
Related Issues (20)
- layerState is undefined in inView function HOT 1
- Idea: Make clustering more configurable HOT 4
- Small improvement in ol.interaction.DragAndDrop HOT 1
- OpenLayers unable to render Float32 GeoTiff HOT 4
- tileGrid key is not an available option in OGCVectorTile HOT 1
- WFS#writeTransaction() fails for undefined properties
- Add KMZ file into Openlayers map HOT 4
- Error when getting attributions of a BingMaps layer
- Vector tile source format type MVT doesn't accept - format: new MVT({ featureClass: Feature }) HOT 1
- KML with Nested MultiGeometry is not Rendered HOT 4
- Failed to resolve module specifier "ol/Map.js". Relative references must start with either "/", "./", or "../".
- how to render a planar coordinate FlatGeobuf data? HOT 2
- Flaky WebGL style parsing test
- webgl - stroke-pattern-start-offset HOT 3
- Graticule ,set latLabelPosition to 0.5 ,not always in the middle
- Replace polyfills link to safe one HOT 1
- Issue with Drawing and Modifying Geodesic Lines in MultiWorld Mode HOT 4
- SVG performance issues in Firefox HOT 2
- Polyfill vulnerability issues HOT 4
- Zooming without loading tiles HOT 12
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from openlayers.