[[!template  id=gitbranch 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. 

[[!tag patch]]
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 [[todo/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]]
>> [[!tag reviewed]]

Here's [[smcv]]'s review from [[todo/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 [[ikiwiki/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]]