When I expose more than one map with features in the page, and the zoom view is setted to fit the features bounds, well, the last loaded map get the bounds of the previous one (if wider than its features bounds) … as the Drupal.leaflet.bounds array is always pushed in with all the features of (all) the page maps.

So it might happen what is shown in my attached screenshot, with 3 maps loaded and the widest as the first …

Everything is fixed if we reset/empty the Drupal.leaflet.bounds array after the Drupal.leaflet.fitbounds(lMap); has run on each map, like with the following added code in the leaflet.drupal.js file


// fit to bounds
else {
Drupal.leaflet.fitbounds(lMap);
+ Drupal.leaflet.bounds = []; //Resets the bounds array, ready again to be used for next map's features
}

The second screenshot shows that now every things works ok.

I attach also the new "leaflet.drupal.js" (zipped) file I am using,
and I hope will be implemented soon in the next module release.

Note: Same matter should be implemented in the Leaflet Markercluster module, as highlighted and requested here: https://drupal.org/node/2040639

CommentFileSizeAuthor
#6 more_leaflet_maps_fit_bounds.jpg170.12 KBitamair
#6 leaflet.drupal.js_.zip2.94 KBitamair
#2 2040627-2.patch424 bytesthedavidmeister
leaflet.drupal.js_.zip2.98 KBitamair
more_maps_leaflet_bounds_ok.jpg287.39 KBitamair
more_maps_leaflet_bounds_bad.jpg288.33 KBitamair

Comments

EndEd’s picture

EndEd commented

Hi, I just want to say that I was having the same problem by having multiple maps in the same page (a list of teasers each with an address and a map).

This is fixed with itamair's solution. Note that you have to patch both Leaflet and Leaflet MarkerCluster modules, as he points out.

Thanx a lot, great module!

thedavidmeister’s picture

thedavidmeister commented
Title: Fit to Features Bounds get Bounds increments of all map's features, when more than one map in the page (Fixed!) » Fit to Features Bounds get Bounds increments of all map's features, when more than one map in the page
Status: Needs review » Needs work
StatusFileSize
new 2040627-2.patch424 bytes

The fix proposed in the issue summary is incomplete, it will fail if:

- A zoom level for this map is provided
- This map has a center provided in this.map.center

So, it only has a 33% chance of working for any random given set of configuration for a map.

A way better place to put the bounds reset is inside Drupal.leaflet.fitbounds as:

- it will actually always be done
- it resets a property of the current object context rather than some other object, so this is a more natural place to put the reset

Here's a patch that addresses these points by moving the bounds reset inside Drupal.leaflet.fitbounds.

This patch is still just a hack though. Ideally we want to refactor Drupal.behaviors.leaflet and Drupal.leaflet so that fitbounds can pull the points it is using to calculate map bounds from the lMap object passed in rather than a global "bounds" array.

The issue with the way we're doing things now with a single, global "bounds" array is that it will break as soon as we dynamically add or remove points to any map when we display more than one map on a page, both before and after the patch I've attached. Storing the points we want to calculate bounds for in a global variable is just a bad idea in general.

So, in summary:

- currently centering is completely broken for any page displaying more than one map, this patch fixes that. Up to the module maintainers whether they want to commit this as a stopgap.
- currently centring is even more completely broken for any page displaying more than one map when points are added/removed to maps asynchronously or in any order other than "apply all points to one map, then move to the next map", this patch does not help with that situation.

Also, @itamair, please do not write "fixed" in the name of an issue. All you have to do is mark the status as "Needs review" when you have provided a patch. When the patch has been reviewed it will be marked "reviewed and tested by the community", when it has been committed to the module it will be marked as "fixed". If you mark an issue as "fixed" before the fix has been committed to the module you will confuse people who do not realise that they need to download the provided patch and apply it manually to their code.

Please provided patches in git format and not in .zip files, as compressed patches are impossible for other people to review through the drupal.org UI, making it harder/more time consuming for the module maintainers to get the patch into production.

thedavidmeister’s picture

thedavidmeister commented
Assigned: itamair » Unassigned
Priority: Normal » Major

this is a major bug as displaying multiple maps on a single page is a very common requirement.

itamair’s picture

itamair commented

I tested quickly your patch, thedavidmeister, with more maps and Leaflet Markercluster module … and its seems ok: at least it still behaves properly in my use case (every maps should fit bounds on its markers … with any fixed zoom and center).
So far so good and better, for me … Probably some more testing would be needed …

Tnx!

thedavidmeister’s picture

thedavidmeister commented

Thanks for the testing @itamair. Unfortunately, as long as the points used calculate map bounds are stored in a global variable shared by all maps on the page and not stored as data for each map individually, there will always be bugs with map zoom/centering functionality :(

itamair’s picture

itamair commented
StatusFileSize
new leaflet.drupal.js_.zip2.94 KB
new more_leaflet_maps_fit_bounds.jpg170.12 KB

I did more deep testing … and found a (hope) better and (seems to me) more logical solution.
Yes, thedavidmeister, I agree. As long as we store the points used to calculate map bounds in that global variable,
we need to empty it all the times after the map with its features is built and stored.

So the perfect place to do this is (seems to be) just right after each map features are destroyed for the same purposes,
and I mean with this code diff (from the actual dev version of leaflet.drupal.js file):


        // Destroy features so that an AJAX reload does not get parts of the old set.
        // Required when the View has "Use AJAX" set to Yes.
        this.features = null;

+        //Resets the bounds array, ready again to be used for next map's features
+        Drupal.leaflet.bounds = []; 
+

This throws every time each map processing is done, and set those global variables ready for a new map. Isn't it?
I did some testing with more maps, with and without Leaflet Markeclustering too, and seems working fine for me.

Attached a screenshot and the new leaflet.drupal.js (zipped) file that works for me now …

Same emending should be done in the leaflet_markercluster.drupal.js file in the Leaflet Markecluster Module (https://drupal.org/node/2040639#comment-7673557)

thedavidmeister’s picture

thedavidmeister commented

What about this sequence of events?

1. Add 2 points to map A
2. Add 1 point to map B
3. Add 1 point to map A (so that it now has 3 points)

The point I was making wasn't that the global variable is hard to reset (it isn't) but that resetting helps manage new maps but completely destroys the ability to update existing maps without rebuilding them from scratch. To be robust, the fitbounds logic needs to use points that can be extracted from a lMap object, not from points that just float around in a global array.

Please provide patches in git diff format, not zip files!

itamair’s picture

itamair commented

Hi, thedavidmeister, of course would be even better "to associate" features and its map.fitbounds to each lMap …
as this logic would allow the bounds to be changed after the map object (and its features) has been constructed … let's say dinamically.

But in this case this doesn't seem to be strictly necessary, to correctly manage more maps and each bounds of them.
The use case you mind (adds to map A, then map B and then map A again …) seems to me quite impossible to happen with the logic the leaflet (and leaflet marker cluster) views are managed. Correctly and logically managed ...

If there is more than one map in the page, the module will manage/process one (whole) map after the other, as its js code clearly suggests:

$(settings.leaflet).each(function () {
…….
}

So in this case seems to be correct to delete/empty/reset the global arrays that handles features and bounds …

Am I wrong? Probably … but I don't really get the real scenario your use case might or is going to happen.
You load all the web page, with the maps, and their features … then, afterward, you want to add some more features to one of them (?! how ?) and even dinamically adjust the center of the map on its features' center of gravity, based on all location. Is it?
Or what else?

At least the posted fix seems to face the more standard and usual scenario: correctly load different maps and fit their bounds on their features …

PS: I didn't provide any patch. It is the js file itself … I will provide patches in those format for sure, in that case. Thanks for your advice …

thedavidmeister’s picture

thedavidmeister commented

Well the question is whether leaflet is intended to work as an API, in which case I can think of a lot of situations where I'd want to be able to write my own scripts to update the maps based on dynamic data.

itamair’s picture

itamair commented

Have a look to this: https://drupal.org/node/2056551

itamair’s picture

itamair commented
Issue summary: View changes

update

levelos’s picture

levelos commented
Issue summary: View changes
Status: Needs work » Closed (fixed)

I believe this has been fixed in commit 1eb92e14baea190ad32030ad286f56f67f33c599, which addressed a similar issue.