move the comments in the right place, add my comments
anarcat
admin
parent
52131b2120
commit
7a9b088839
|
|
@ -12,3 +12,98 @@ I have produced a patch for this issue, but beware, while it appears to fix the
|
||||||
>> over on the todo page for that branch. Feel free to move my
|
>> 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]]
|
>> review comments for it here if you want to split the discussion. --[[smcv]]
|
||||||
>> [[!tag reviewed]]
|
>> [[!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]]
|
||||||
|
|
||||||
|
> + @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 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]]
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue