+[[!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.
-<https://github.com/cbaines/ikiwiki/commit/4294b4c24a56c7103c48250dd9d833b42838a472>
+
+> 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]]
sipb-www / ikiwiki.git / blobdiff