Bug in "Allow zoom-to-layer to apply to multiple layers" patch [#1708514]

Opening another issue as the (super helpful!) patch in #928244: Allow zoom-to-layer to apply to multiple layers was already committed. I think there is a missing line in this patch that causes a bug.

I had two maps

one with multiple layers and multiple points - which worked brilliantly with the patch;
the second with a single layer and single point - which worked before the patch, and failed directly after the patch was committed (reverted back and forth a few times to confirm)

Disclaimer is the OpenLayers code itself - especially on the JS side - is a black box for me as this is the first I've dove into it, so not sure exactly what map.zoomToExtent(), map.fullExtent.extend(), and map.zoomTo() are really doing, so I could be way off base.

However, when I compared the old code and the new kml and geojson code against the new else for non-kml/geojson layers, there was a line missing. I added that in and voila, now my single-layer, single-point map works (along with my multi-layer, multi-point still working).

Maybe this is a larger issue, so just posting the teensy patch here so others more familiar can decide.

For quick review:

Current dev branch code with new line this patch adds highlighted with comment at the bottom:

      if (layers[i].layer_handler == 'kml' || layers[i].layer_handler == 'geojson') {
        layers[i].events.register('loadend', layers[i], function() {
          layerextent = layers[i].getDataExtent().scale(zoomtolayer_scale);
          map.fullExtent.extend(layerextent);
          map.zoomToExtent(map.fullExtent);
        });
      }
      else {
        layerextent = layers[i].getDataExtent();
        // Check for valid layer extent
        if (layerextent != null) {
          map.fullExtent.extend(layerextent);
          map.zoomToExtent(map.fullExtent); /****** NEW LINE ******/
        }
      }

I know there is the new call in the final else, but with a single point that is never hit.

Comment	File	Size	Author
#2	bug-fix-for--openlayers-zoom_multiple_layers-928244-22.patch	660 bytes	sokrplare
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Pol

the/that

Brussels

CreditAttribution: Pol commented 1 August 2012 at 21:12

Status:

Needs review

» Needs work

Hi !

Thanks for the finding ;-)
Can you provide a patch ?

Thanks !

Comment #2

sokrplare CreditAttribution: sokrplare commented 2 August 2012 at 14:55

Status:

Needs work

» Needs review

File	Size
bug-fix-for--openlayers-zoom_multiple_layers-928244-22.patch	660 bytes

Oops, patch was missing extension so d.o. rejected the upload - here we go!

Comment #3

Pol

the/that

Brussels

CreditAttribution: Pol commented 2 August 2012 at 15:09

Status:

Needs review

» Fixed

Thanks, committed !

http://drupalcode.org/project/openlayers.git/commit/0613bed

Comment #4

sokrplare CreditAttribution: sokrplare commented 2 August 2012 at 15:13

Wow, you're fast - thanks!

Comment #5

becw CreditAttribution: becw commented 2 August 2012 at 15:16

Oops, I was commenting as you committed :)

The calls to map.fullExtent.extend() expand the OpenLayers.Bounds() area to include another set of bounds (ie, the bounding box around everything in a layer, obtained with layer.getExtent()). (OpenLayers docs for OpenLayers.Bounds.extend()).

Basically, this code is stepping through all of the layers and building a "Bounds" object that contains all of the things. To avoid zooming the map multiple times, map.zoomToExtent(map.fullExtent) shouldn't be called before all of the extents are gathered--so instead of putting the missing call inside the for loop, try it at the end. (For json and KML layers, we can't avoid zooming multiple times, because we don't know when all of the layers will be finished loading.)

... and it looks like it's already being called at the end, but maybe the .scale() method isn't functioning as expected here:

  ...
  // If unable to find width due to single point,
  // zoom in with point_zoom_level option.
  if (map.fullExtent.getWidth() == 0.0) {
    map.zoomTo(options.point_zoom_level);
  }
  else {
    map.zoomToExtent(map.fullExtent.scale(zoomtolayer_scale));      // line 53
  }
});

Comment #6

sokrplare CreditAttribution: sokrplare commented 2 August 2012 at 15:23

Yeah, I was wondering about that - the odd thing I found was that final else was never being called with a single-point. Instead the map.zoomTo() was hit - otherwise I think that call at the end would have done it (and more efficiently as you mention).