Made refreshClusters to re-draw markers in singleMarkerMode #583
Conversation
Following issue Leaflet#581, improved refreshClusters() method so that it detects singleMarkerMode and re-draws all passed markers as well, not only parent clusters. Also created the singleMarkerModeSpec test suite to test singleMarkerMode option, added an extra test in RefreshSpec test suite to cover this case, and added the new suite into spec/index.
into a dedicated private method, as it has nothing to do in `_flagParentsIconsNeedUpdate()`.
After many testing, it turns out that the beforeEach and afterEach functions do not cause that much trouble regarding the memory leak issue of PhantomJS. Just re-using objects as much as possible (in particular, avoiding re-creating the div and map between each individual test) already brings significant improvement. The custom `it2` function was working, but it was breaking the readability of the spec/index file report. So I switched the test suite back to using the native functions.
to use back the standard beforeEach and afterEach functions, but re-using objects as much as possible. This avoids too much memory leak in PhantomJS while still having a spec/index readable.
|
Do we need to worry about a marker that is not in the cluster being passed in to the refresh method? |
|
Hi, Indeed there is currently no check that the passed markers really belong to the MCG. In that case, the method would silently flag the parent clusters (if there are) as having a "dirty" icon, which will make them be re-drawn by the legitimate MCG when needed (but it will not immediately refresh the currently visible ones). Which is close to the "expected" behaviour, minus the temporary not refreshed visible clusters. We should probably make a decision on whether we should check internally if the passed arguments are legitimate (and possibly ignore them silently), or if we simply charge the application developer of this step. Another option would be to use a "static" function (like "L.MarkerClusterGroup.refresh") rather than an object method. It could easily determine the involved MCG's. That way, the application does not need to care about which MCG the markers belong to (if they belong to any). What are your thoughts? |
|
Sorry that's not what I meant. I mean if I make a new Marker, and don't add it to any MCG, then pass it to the icon refresh method, it will have its icon replaced with a (1) icon. |
|
Oh ok you mean when the method is called while the Indeed you are right about what would happen in that case: icons are also re-drawn. I agree the consequences are more worrying than the previous case, even if we would still blame the application developer... :-) It could be worth adding |
to avoid overriding icons of markers that do not belong to the current MCG. Otherwise, we change icons that have nothing to do with the group. Following comment Leaflet#583 (comment)
|
Should be good now. |
Made refreshClusters to re-draw markers in singleMarkerMode
|
Looks good thanks |
Following issue #581, improved
refreshClusters()method so that it detectssingleMarkerModeand re-draws all passed markers as well, not only parent clusters. Took the opportunity to factorize code with_addLayer()method, creating a new_overrideMarkerIcon()method that re-styles the marker using theiconCreateFunctionfrom clusters.Added an extra test in RefreshSpec test suite to cover this case. Also re-factored it using
it2wrapper to avoid hanging test process.Also created the singleMarkerModeSpec test suite to test
singleMarkerModeoption and included it into the new suite into spec/index.