Conversation
|
In order to contribute to the MapStore project, the CLA (Contributor License agreement) should be sent signed to GeoSolutions. Please consult contributing rules at: https://github.com/geosolutions-it/MapStore2/wiki/Contributing-to-MapStore#contributing-code |
|
Thank you very much for your contribution to the MapStore codebase. We will review it as soon as possible. Many thanks. |
|
Thank you so much for this! |
I work for the company (my GH's profile is up to date), arx iT, we already send the CLA. We can update it if necessary. |
|
Hi @tdipisa |
MV88
left a comment
There was a problem hiding this comment.
First of all thanks for this contribution Here a few feedbacks about this PR:
Handling the auto-refresh
We noticed that you are introducing a lot of logic in order to handle the refresh inside the core components like:
- web/client/components/map/openlayers/* and other mapping library
- web/client/utils/openlayers/* and other mapping library
we think that this might be too much and we propose a different direction you can take:
- there is already for WFS and WMS a mechanism to trigger an update which is called refreshLayerVersion
MapStore2/web/client/actions/layers.js
Lines 222 to 230 in 1de66c1
that changes a param _v_ dedicated to "refresh" recreate the layer instance hence updating it. So it is enough to trigger this action instead of the AUTOREFRESH_TICK one. This is also a more abstracted way to handle refresh since layers doesn't need to be aware of ticks and if another plugin needs to trigger a refresh we can reuse refreshLayerVersion logic, so instead of having a refresh method inside layers you can reuse the logic and add the missing logic on _v_ parameter in the update method, therefore a cleanup here is needed, implementing this logic in the remaining layers.
All the ticks data and logic probably can be removed using derived data from layers object, triggering the action in the epic itself.
Manage changes and different intervals
In order to manage visibility or param changes and activate/deactivate the epic you need to recollect the possible trigger actions that require a reset,
We suggest a set of TRIGGERS actions, and a set of timers that triggers the refresh action on each timeout.
When one Trigger changes one of the intervals, or one of the layers arrives/goes out, the timer is added/recreated/destroyed.
Here a sample code:
const TRIGGERS = [
CHANGE_LAYER_PROPERTIES, // visibility toggle
CHANGE_LAYER_PARAMS,
UPDATE_NODE, // autoRefreshInterval update
ADD_LAYER,
REMOVE_NODE,
MAP_CONFIG_LOADED // init
AUTOREFRESH_START, // initiate the flow
AUTOREFRESH_STOP, // stops the flow
AUTOREFRESH_PAUSE // ADD PAUSE ACTION THAT blocks execution if when feature editor is in edit mode, this action will be triggered by the refresh plugin itself that will listen for changes to the featuregrid.mode property and trigger it when that the mode value changes like a toggle action, the pause value can be stored inside autorefresh reducer
];
const layersAutoRefreshEpic = (action$, store) => {
const getLayersMap = () =>
isPaused(store.getState()) || !isActiveRefresh(store.getState()) ? {} : refreshableLayers(store.getState())
.reduce((acc, l) => ({ ...acc, [l.id]: l.interval }), {});
// this stream should emit refreshable layers map on every possible change, but not when paused or refesh is not active (toggled on)
const active$ = action$.ofType(...TRIGGERS)
.map(getLayersMap)
.startWith(getLayersMap())
.distinctUntilChanged(isEqual) // if some of the layer interval changed (or list change) triggers
.publishReplay(1).refCount(); // activate multicast
return active$
.mergeMap(m => Object.keys(m).map(id => ({ id, interval: m[id] })))
.groupBy(d => d.id)
// this merges N stream actions, 1 for each layer that needs a refresh, each one with its own timer
.mergeMap(group$ => {
const id = group$.key;
const removed$ = active$.filter(m => !m[id]); // this intercepts when a layer is not in the map anymore
return group$
.map(d => d.interval)
.distinctUntilChanged() // if change interval, rebuild the timer
.switchMap(interval =>
Rx.Observable.timer(interval, interval)
.map(() => refreshLayerVersion(id, Date().getTime()))
)
.takeUntil(removed$); // teardown of the single timer
});
};refreshableLayersSelector will exclude layers being edited from attribute table or from widget wizard
State handling
Considering that autorefreshInterval is stored within layers object, you don't need availableLayers: {}, or activeLayers: {} in reducer. with a dedicated selector you can fetch layers with autorefreshInterval defined from the layers state and then filter the array of layers with only the one with visibility set to true, so they can trigger the refreshLayerVersion action. Try to reuse the layers selectors
User Interface
It is not clear from how the settings panel should look like, for example how to handle multiple layers inside a single widget? Probably not all layers inside a widget must refresh? When a layer is selected to be refreshed, (in layers state) it will receive the autorefreshInterval property. Considering that widgets have their own state, then also this state should receive the autorefreshInterval param for all the layers instances. If unselected in all places (widgets & layers state) it will remove the autorefreshInterval param from layer object. it would be nice to visually know if layer is coming from the map or from which widget
Map Widgets
The refreshLayerVersion action trigger an update of _v_ and by keeping track of this change we can retrigger the update on widgets. By monitoring changes to _v_ param in this part we can trigger a refresh on the widget
Dashboards
Regarding dashboards we have some concerns on how to handle connections between widgets(dependencies managemente) even after introducing the logic to listen for changes on _v_ param it might not be enough. Therefore we suggest to create a dedicated issue to better investigate this part after the others are done.
| import Rx from 'rxjs'; | ||
| import { isNumber } from 'lodash'; | ||
|
|
||
| import propsStreamFactory from '../../misc/enhancers/propsStreamFactory'; |
There was a problem hiding this comment.
avoid using recompose for the new development as it is going to be removed at all in the future
|
|
||
| const AutoRefreshForm = ({ | ||
| defaultRefreshInterval, | ||
| minRefreshInterval, |
There was a problem hiding this comment.
this is not named minimumRefreshInterval
Description
This is an early stage version of the plugin to validate the approach.
Mainly works with Openlayers on few layer types (the ones who implements the
refresh's method).Footer Plugin next to the CRS Plugin.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
Feature not available
What is the new behavior?
#11887
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)