Enabling and disabling draw seem hard to do transactionally safe

[averas]

I have a single page application where I want to enable and disable draw based on navigational context. Entering some specific context may enable drawing while also updating some other state on the map (such as zoom, center, pitch etc.). As it seems, enabling and adding draw (and its inherent dependencies) may be queued after such other operations, which makes it hard to determine when draw has actually been successfully enabled (i.e. controls, sources and layers good to go).

Since it's hard to tell when draw has been successfully enabled it's also hard to determine when/if one can safely remove draw without entering a degenerate state.

If you go to this codepen you will have a setup where draw has not yet been enabled. If you just enable/disable draw by clicking on the add/remove buttons it works well, but if you have draw disabled and click the fly button and then try to enable draw it will add the controls, but the necessary layers will not be added until the fly is finished (which takes a while!). If you click on remove at this point you will get exceptions and enter a state hard to recover from (controls are still there but there are no sources/layers on the map).

If you think this scenario is extreme/unlikely you may actually provoke the same anomaly just by dragging the map and hit add/remove before the panning has finished. It seems that any current operations being performed by the map queues up the enabling of draw.

As you probably understand this behaviour makes it really hard to add/remove draw safely in a single page application where other map interactions are carried out simultaneously. Sometimes it works, sometimes it doesn't.

What I believe is required is either some kind of "promise" that tells when draw actually has been "completely" enabled, meaning that it's safe to remove (map.on("draw.load")?). Or that the removeControl operations can handle a situation where draw has not yet been completely enabled and still remove whatever is there.

[averas]

I think the basic problem is that draw is waiting for the map to be loaded:

      if (map.loaded()) {
        connect();
      } else {
        map.on('load', connect);
        intervalId = setInterval(() => { if (map.loaded()) connect(); }, 16);
      }

During any kind of interaction or animation map.loaded() will return false. During this period disabling draw will lead to a degenerate state. This is even more obvious when using raster layers where there seems to be a map.loaded() bug: mapbox/mapbox-gl-js#3872 (now resolved in master)

If you go to http://codepen.io/anon/pen/QGeGWx and click the fly button and check the status of map.loaded() you will see that it returns false during the entire animation (check javascript console).

Is the draw code intention really to wait for animations/interactions to be completed or is it really a style load that was the intent here? Is there a better way for wait for style loads?

cc @lucaswoj

[mcwhittemore]

@averas - yea... I've seen these kinds of errors. We've taken a few stabs before to help make this better, but there seems to be a bunch of edge cases. Any work you can do to help put this to rest would be great.

[averas]

@mcwhittemore Perhaps it makes sense to approach this from two angles, one being the ability to easily enable/disable draw while draw resources might remain persistent in the background, the other being transaction safe add/removal of draw. The reason I initially went with add/remove was the performance penalty draw added to my maps, even without anything drawn (as you may recall), but that seems to have improved since.

[mcwhittemore]

I'm not 100% sure what the enable/disable draw result would be. Does this hide all controls and put draw into a non-editable state?

Assuming Draw had no performance cost, when would a user want to remove it if there was an easy way to disable it?

[averas]

I'm not 100% sure what the enable/disable draw result would be. Does this hide all controls and put draw into a non-editable state?

I'd say:

• Disable/hide controls
• Disable keybindings (I have other keybindings that otherwise might conflict)
• Hide draw layers
• Clear sources (since this is possible via the API it could be left to the client)

Assuming Draw had no performance cost, when would a user want to remove it if there was an easy way to disable it?

In a single page application like mine where it's enabled/disabled frequently I would say that you would never have to remove it. In an application where draw is rarely used perhaps it makes sense to remove it to conserve resources when not used, but it would probably still not be necessary.

[mcwhittemore]

So enable/disable appears to do the same thing as adding or removing draw from the map. The major difference is that in your code you can simply say Draw.enable() and Draw.disable() rather than map.addControl(new MapboxDraw()).

This makes sense to me. The docs need to be clear that this will remove features from the map.

When Draw is disabled, we'll have to decide how to handle requests to the other Draw api methods.

[davidtheclark]

we'll have to decide how to handle requests to the other Draw api methods

I wonder if an error should be thrown. It seems that if you are calling a Draw API method when Draw is disabled, something is probably wrong with your program. Your code is not accomplishing what it expected to accomplish. If there is a UI element involved, the user is not accomplishing what she expected to accomplish.

[mcwhittemore]

Yea, I agree that anything that has to do with drawing should never be called when Draw is disabled. But if you are using Draw as the main data storage option for geojson features you might want to get and set features even if Draw is disabled.

[averas]

Looks as if the Mapbox GL JS API is updated in regards to the "loaded" state:

mapbox/mapbox-gl-js#3941

If loaded is changed to in fact mean loaded (as in styles etc. and not necessary "is moving" like now) I believe this issue will be less prevalent, though not completely eliminated.

[z0d14c]

Yeah this bug kinda sucks :( background: we use mapbox-gl-draw in our app, which uses a react wrapper (react-mapbox-gl) and a component which adds/removes mapbox-gl-draw control to the map on mount/unmount. The problem is that mapbox-gl-draw's onRemove throws errors if layers/styles/whatever have not actually been added to the map yet, and onAdd throws errors if layers/whatever have already been added, and it is unclear to me how to check them.

finally something like this works, but kinda sucks (this is in the componentWillUnmount):

    const unmountCb = () => {
      console.log('unmount cb');
      if (map.getLayer(LAYERCHECK_HACK_ID)) {
        console.log('calling remove control ');
        map.removeControl(this.drawControl);
        map.off('draw.create', this.props.onDrawCreate);
        map.off('draw.delete', this.props.onDrawDelete);
        map.off('draw.update', this.props.onDrawUpdate);
        map.off('draw.selectionchange', this.props.onDrawSelectionChange);
        map.off('draw.modechange', this.props.onDrawModeChange);
    }
   };

as you can see, the hack we are using checks for some specific layer that gets added by mapbox-gl-draw but there should probably be an API command like drawControl.loaded() which signals that these have been added in a less hacky way.

[mcwhittemore]

@z0d14c thanks for detailing this out. I'm pretty deep in another project right now, but if you want to take a stab at solving this via a PR, I think it would really help a bunch of people.

Just FYI, the above code looks a lot like how we handle mapbox-gl-draw in react too. :(

[mcwhittemore]

I'm not 100% sure #685 covers everything that was raised in this issue so if something is missing, please open a NEW issue and link back to this one. Hopefully that helps create some issue clarity.