1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
|
[[!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]]
|