Available in a git repository branch.
Branch: cbaines/osm-layers-patch
Author: cbaines

Using the osm plugin with a simple [[!osm ]] directive does not seem to work, a "TypeError: mapProjection is null" is given. I believe this is because the client side Javascript uses the options.layers, which is always Null.

I have produced a patch for this issue, but beware, while it appears to fix the problem for me, I have little understanding of perl and the existing code base.

It looks sound, but I have yet to test it. --anarcat

I reviewed a version of this (possibly rebased or modified or something) that was in the osm plugin GeoJSON popup patch branch, over on the todo page for that branch. Feel free to move my review comments for it here if you want to split the discussion. --smcv

Here's smcv's review from osm plugin GeoJSON popup patch, annotated with my comments. --anarcat

It would be good if the commit added documentation for the new feature, probably in doc/ikiwiki/directive/osm.mdwn.

+ my @layers = [ 'OSM' ];

You mean $layers. [] is a scalar value (a reference to an array); @something is an array.

Or @layers = ( 'OSM' );. --anarcat

Yeah, and then layers => [@layers] or layers => \@layers to turn it into a reference when building %options. --s

+     @layers = [ split(/,/, $params{layers}) ];

Is comma-separated the best fit here? Would whitespace, or whitespace and/or commas, work better?

Why don't we simply keep it an array as it already is? I fail to see the reason behind that change.

This seems to be at least partially a feature request for [[!osm ]]: "allow individual [[!osm ]] maps to override $config{osm_layers}. Items in %config can be a reference to an array, so that's fine. However, parameters to a directive cannot be an array, so for the directive, we need a syntax for taking a scalar parameter and splitting it into an array - comma-separated, whitespace-separated, whatever. --s

This is the config I use right now on http://reseaulibre.ca/:

osm_layers:
- http://a.tile.stamen.com/toner/${z}/${x}/${y}.png
- OSM
- GoogleHybrid

It works fine. At the very least, we should default to the configuration set in the the .setup file, so this chunk of the patch should go:

-        $options{'layers'} = $config{osm_layers};

Maybe the best would be to use $config{osm_layers}; as a default? --anarcat

It's difficult to compare without knowing what the values would look like. What would be valid values? The documentation for $config{osm_layers} says "in a syntax acceptable for OpenLayers.Layer.OSM.url parameter" so perhaps:

# expected by current branch
[[!osm  layers="OSM,WTF,OMG"]]
[[!osm  layers="http://example.com/${z}/${x}/${y}.png,http://example.org/tiles/${z}/${x}/${y}.png"]]
# current branch would misbehave with this syntax but it could be
made to work
[[!osm  layers="OSM, WTF, OMG"]]
[[!osm  layers="""http://example.com/${z}/${x}/${y}.png,
  http://example.org/tiles/${z}/${x}/${y}.png"""]]
# I would personally suggest whitespace as separator (split(' ', ...))
[[!osm  layers="OSM WTF OMG"]]
[[!osm  layers="""http://example.com/${z}/${x}/${y}.png
  http://example.org/tiles/${z}/${x}/${y}.png"""]]

If you specify more than one layer, is it like "get tiles from OpenCycleMap server A or B or C as a round-robin", or "draw OpenCycleMap and then overlay county boundaries and then overlay locations of good pubs", or what?

Multiple layers support means that the user is shown the first layer by default, but can also choose to flip to another layer. See again http://reseaulibre.ca/ for an example. --anarcat

+     layers => @layers,

If @layers didn't have exactly one item, this would mess up argument-parsing; but it has exactly one item (a reference to an array), so it works. Again, if you replace @layers with $layers throughout, that would be better.

-        $options{'layers'} = $config{osm_layers};

Shouldn't the default if no $params{layers} are given be this, rather than a hard-coded ['OSM']?

Agreed. --anarcat

getsetup() says osm_layers is safe => 0, which approximately means "don't put this in the web UI, changing it could lead to a security flaw or an unusable website". Is that wrong? If it is indeed unsafe, then I would expect changing the same thing via [[!osm ]] parameters to be unsafe too.

I put that at safe=>0 as a security precaution, because I didn't exactly know what that setting did.

It is unclear to me whether this could lead to a security flaw. The osm_layers parameter, in particular, simply decides which tiles get loaded in OpenLayers, but it is unclear to me if this is safe to change or not. --anarcat

I notice that example => { 'OSM', 'GoogleSatellite' } is wrong: it should (probably) be example => [ 'OSM', 'GoogleSatellite' ] (a list of two example values, not a map with key 'OSM' corresponding to value 'GoogleSatellite'. That might be why you're having trouble with this.

That is an accurate statement.

This is old code, so my memory may be cold, but i think that the "layers" parameters used to be a hash, not an array, until two years ago (commit 636e04a). The javascript code certainly expects an array right now. --anarcat

OK, then I think this might be a mixture of a bug and a feature request:

  • bug: the configuration suggested by the example (or the default when unconfigured, or something) produces "TypeError: mapProjection is null"

  • feature request: per-[[!osm ]] configuration to complement the per-wiki configuration

--s

That is correct. --anarcat