Closed danrademacher closed 5 years ago
control onRemove
is throwing an error. Sounds like there needs to a some dummy onremove
within the control. The Timeslider doesn't have an onRemove
handler, so if we added one, maybe that would solve the issue:
In demo, we can test and fix this to hide the layer/map and then repro the error and resolve.
This seems to be caused by anytime we're trying to include a Date Missing or Date Invalid layer and see the timeslider work.
Could also b a Mapbox API version issue.
This is the actual style JSON that @jimmyrocks has been working with: https://github.com/OpenHistoricalMap/ohm-website/blob/ohm-mapbox-gl-style/app/assets/javascripts/ohm.style.js
[x] preliminary test of 0.53.1
[x] work the onRemove
issue
onRemove
error happens; and that this is in the MBGL map because the TimeSlider has no onRemove
handler (wasn't intended for that)onRemove
handleronRemove
remove filters[0]
from all layers, since that's the one known to be date-filtering?[x] work the filtering compatibility issue
dateinvalid
and datemissing
classes[ ] I would still recommend that the Historical map layer, not bother including the dateinvalid
and datemissing
classes; after all, the features have broken dates so they would not match any filters, and are thus inherently incompatible with the concept of filtering
First item: Tested timeslider with 0.53.1 PASS.
The mbgl-control-timeslider works A-OK with 0.53.1 So that is highly unlikely to be a cause of the problems that were had with integrating this into the other map style.
Second item: onRemove
Confirmed that using just the mbgl-control-timeslider demo, that removing the timeslider control from the MBGL Map does indeed raise an error: Invalid argument to map.removeControl(). Argument must be a control with onAdd and onRemove methods.
Commit cf45592 brings a onRemove()
handler, which seems to work well for a start:
In my test using a MBGL map and the MBGL control, I was able to:
Still need to test when Leaflet is added, which adds a few new rings to this whole onion...
Note that when re-adding the time slider control to the map, you're re-adding it which means it will apply all of its settings as passed to the constructor options, and it won't have a way of "remembering" settings from its previous existence. That is to say, it would be up to the caller to store the min/max/selected date and to re-apply them after re-adding the control, because the control is starting over from its options.
That would go double for using it in a Leaflet context, in which removing the MBGL layer removes the entire MBGL Map and its controls from existence. The new MBGL layer and new control are truly new, and again it would be on the caller to store the settings and re-apply them.
If I'm reading this right, it sounds like the time slider gets destroyed every time that it is removed. So it would be useful to maintain its state elsewhere, and when the time slider is recreated, it would be initialized with those state settings. Does this sound correct?
I added to the UrlHashReader
control as onRemove
handler, so it may now be gracefully removed from the map.
For most needs this wouldn't be relevant, since it did its work when it was added and does nothing else later.
However, in a Leaflet context e.g. leaflet-control-mbgltimeslider demo it is relevant: removing the MBGL layer, means it will individually remove each control, and if they're also using UrlHashReader
then this would be an error.
So, that now works properly. In the leaflet-control-mbgltimeslider demo, I can now MAP.removeLayer(ohmlayer)
and it removes the layer (GL map) and its controls just fine. I can then re-create and re-add a ohmlayer and timeslider again, as expected.
@jimmyrocks Yes, that's right.
Even if you had kept a reference to the old L.Control.MBGLTimeSlider
, it was connected to a L.mapboxGL
which is now stale. I wouldn't say it's flatly impossible to massage around that and get the old control and layer working, but it would be implementation-specific.
You would do well to store a copy of the timeslider control's selections someplace else (global vars, URL params, up to you) and then upon reinstating the "Historical" layer (L.mapboxGL
and L.Control.MBGLTimeSlider
) re-apply that state to the L.Control.MBGLTimeSlider constructor options.
Alternately, you may be able avoid the problems of adding/removing layers (meaning, adding/removing a MBGL map, and implicitly losing state information) by simply hiding the L.mapboxGL
layer in Leaflet. A given GL layer's Leaflet DIV will be themblayer._glContainer
and one could apply a CSS class to it, causing it to have 0 opacity or a display: none
... That may be a different angle, toward hiding a GL layer without losing it.
Looking into the old datemissing
styles, to see why they would trigger errors when trying to use them with the time slider control. I wouldn't expect these features to display, since they have no good dates, but we should expect that their filters wouldn't generate syntax errors!
Looks as if MBGL is becoming confused after the time slider's clause is prepended, but this isn't strictly because of the time slider. When MBGL sees the filter syntax being used (an older style, since it was developed a year ago) it switches over to an older mode in which "match" was not supported.
For example, a MBGL map with no time slider, consisting of this one single layer, gives that same error because MBGL just plain picks the wrong filtering system.
{
"id": "building-badfilter",
"type": "fill",
"source": "ohm-data",
"source-layer": "building",
"filter": [
'all',
[ 'any', ['!has', 'osm_id'] ],
[ '==', ['geometry-type'], "Polygon" ],
[ 'has', 'osm_id' ],
[ 'match', ['get', 'osm_id'], [ -1 ], true, false ],
],
"paint": {
"fill-color": "red"
},
},
Ideally, we could revisit the filtering in the time slider and in the old demos, and upgrade everything to the later syntax (unknown effort, a few days?). But that has not been a priority.
Try using this filter instead.
This excludes features lacking a osm_id
and thus presumed to be non-OHM such as Natural Earth for coastlines and the like, and then matches features which have a start_date
but not a decimal start_decdate
version which it could guess from that date, and likewise for the end_date
{
"id": "building-badfilter",
"type": "fill",
"source": "ohm-data",
"source-layer": "building",
"filter": [
'all',
[ 'has', 'osm_id' ],
[
'any',
[ 'all', [ 'has', 'start_date' ], [ '!has', 'start_decdate' ] ],
[ 'all', [ 'has', 'end_date' ], [ '!has', 'end_decdate' ] ],
]
],
"paint": {
"fill-color": "red"
},
},
Again, by definition features passing this filter would not also be affected by the timeslider, but it is a way forward if you for some reason need one map style that supports both feature types.
Got the OK to close this, as the work been moving along.
@jimmyrocks has been chipping away at the integration of the timeslider into the Rails app and he's run into two issues that seem like they require changes in timeslider code:
Let's document these items more clearly in this issue, so we can get them resolved. Assigning first to Jim for documentation, then likely move to Gregor for resolution