Leaflet toolbar plugin

[justinmanley]

This PR replaces all of the toolbars in Leaflet.draw with toolbars from Leaflet.Toolbar as proposed in #324.

In particular, this pull request:

• Has each draw handler inherit from L.ToolbarAction instead of L.Handler. L.ToolbarAction is a new class from L.Toolbar, inheriting from L.Handler and including defaults for the toolbar icon and methods for building the icon and submenu.
• Replaces src/draw/DrawToolbar.js with a control-style drawing toolbar in src/draw/control.
• Replaces src/edit/EditToolbar.js with a control-style editing toolbar in src/edit/control.
• Adds a new popup-style editing toolbar in src/edit/popup.
• Prunes Leaflet.draw CSS, as much of the toolbar styles can now be pulled in from Leaflet.Toolbar.

There are live examples of the new toolbar functionality for Leaflet.draw at:
http://manleyjster.github.io/Leaflet.Toolbar/examples/control.html
http://manleyjster.github.io/Leaflet.Toolbar/examples/popup.html.

I also wrote a short blog post about designing Leaflet.Toolbar and porting the Leaflet.draw toolbars over to the new plugin - you can check that out here: http://justinmanley.io/blog.

Documentation for Leaflet.Toolbar is in a wiki: https://github.com/manleyjster/Leaflet.Toolbar/wiki/API-Reference.

I'm excited to get a conversation started about this PR!

[mourner]

Awesome work! Had a quick look but will wait for @jacobtoye to weigh in.
I think you should rebase the pull so that it merges to master cleanly.

[justinmanley]

Yup, will do. Didn't want to rebase prematurely in case there was development going on in the master branch. I'm happy to rebase for a clean merge once everyone's had a chance to look through the PR and we've resolved any questions / issues.

[jacobtoye]

Awesome. I will give this the attention it deserves as soon as possible. Hopefully over the next few days.

[jacobtoye]

Leaflet.draw uses a bit of a hack to support localization. See https://github.com/Leaflet/Leaflet.draw/blob/master/src/Leaflet.draw.js So for the tooltip text you would use Leaflet.draw.circle.tooltip.start.

[justinmanley]

Fixed. Thanks for catching this - I had forgotten about Leaflet.draw's localization support :).

[jacobtoye]

I'm thinking we chould add Leaflet.toolbar as a submodule for Leaflet.draw, since Leaflet.draw will now have a dependency on Leaflet.toolbar. This would mean people who are already using Leaflet.draw can just download the built code still. Other wise they would need to also download Leaflet.toolbar.

A potential issue with this would be that if people wanted to use both Leaflet.draw and roll their own toolbar for something else they may unknowingly include Leaflet.toolbar twice.

What are you thoughts?

Other than that could you back out the changes to built files. We don't include changes to these in pull requests.

[kyletolle]

The names of the similar edit control files are in the format edit/control/Edit.Control.Edit.js.

Would these two draw controls be better named and have more consistency if they were draw/control/Draw.Control.Cancel.js and draw/control/Draw.Control.RemoveLastPoint.js?

[justinmanley]

@jacobtoye - Ooops - I had meant to keep the built files out of the PR. Not sure how they made it in, but I'll remove them when I rebase.

What do you have in mind when you say "submodule"? Are you suggesting that we git-submodule Leaflet.toolbar, or that we add Leaflet.toolbar to the build/deps.js file for Leaflet.draw and include it in the built Leaflet.draw distribution? I don't think the first is a good idea, but I'm fine with including Leaflet.toolbar in the build.deps. Application developers who want to use both .draw and .toolbar should be able to avoid loadingLeaflet.toolbar twice by simply loading Leaflet.draw before any other scripts that use Leaflet.toolbar.

@kyletolle - Agree that the naming is not completely consistent. The difference is that L.Edit.Control.Edit is a top-level action, while L.Draw.Cancel and L.Draw.RemoveLastPoint are sub-actions that are exposed once their parent top-level action is triggered. L.Draw.Cancel and L.Draw.RemoveLastPoint are analogous to L.EditToolbar.Save and L.EditToolbar.Undo, which are contained in src/edit/control/EditToolbar.Control.js.

I think maybe the best way to resolve this is to put subhandlers into their own directory. So I would create src/edit/control/subhandler/ and move L.EditToolbar.Save and L.EditToolbar.Undo into subhandlers (probably renaming them), and do the same for the Draw.Control subhandlers. Thoughts?

[kyletolle]

Thanks for the additional explanation. The L.Draw.Cancel and L.Draw.RemoveLastPoint are definitely sub actions like L.EditToolbar.Save and L.EditToolbar.Undo.

In the code, you've moved from L.Handler to L.ToolbarAction. What about renaming the folders to have the action name? So, the folders could be src/edit/control/actions and src/edit/control/subactions instead of handlers and subhandlers. Same thing could be done for the Draw actions and subactions, like you mentioned.

[kyletolle]

Additionally, Leaflet.draw currently highlights the currently edited marker and gives a tooltip to the cursor:

Using the both the control and popup demos based on this PR, I didn't notice this functionality when editing a marker.

The marker doesn't change visually at all when in edit mode.

[kyletolle]

I also think the examples will need updating to work with these changes.

[jacobtoye]

@manleyjster Yeah I was thinking git-submodule. Was just trying to avoid people who are already using Leaflet.draw to have to know to download Leaflet.toolbar too. But we can just do a new version and include a note in the Readme.md.

@kyletolle thanks for finding that! I'll make sure I checkout @manleyjster's branch and double check all example are working.

[justinmanley]

@jacobtoye - I understand where you're coming from, but I don't think git-submodule is a good idea. I've wrestled with it enough to not want to do so any more - it's not always intuitive to work with, and it's a huge pain to get rid of.

I would suggest doing a new minor version release. We can add Leaflet.toolbar as a dependency in the package.json file and then include it in build/deps.js so that the source for Leaflet.toolbar is included in the source for Leaflet.draw. At the very least, that means that people won't have to add an extra script and link tag for the Leaflet.toolbar dist files. Would you be comfortable with that?

[jacobtoye]

@manleyjster yeah no problem. Just wanted to run the idea past you. I'm happy to do as you say. Will do it this weekend.

[jacobtoye]

@manleyjster could you please rebase with master and also back out the changes to the /dist files.

[justinmanley]

@jacobtoye - Don't worry - I haven't forgotten about this - it's on my to do list. I'd still like to resolve one issue with Leaflet.draw + Leaflet.Toolbar (see Leaflet/Leaflet.toolbar#18) before rebasing. I also need to merge https://github.com/Leaflet/Leaflet.toolbar/tree/single-toolbar-on-map (a Leaflet.Toolbar bugfix) into Leaflet.Toolbar master.

Once that's done, I'll back out the dist/ changes and rebase.

Sorry for the delay - it's been tough for me to find spare time to think about how to approach the outstanding bugs / issues on Leaflet.Toolbar. I hope to be able to resolve these soon.

[justinmanley]

Just rebased. I also just released a new version of Leaflet.toolbar (0.1.2), which fixes many of the bugs that folks have posted in the above comments and in the Leaflet.toolbar issue tracker. Should be ready to go!

One last impediment - I didn't include Leaflet.toolbar in the build.deps for Leaflet.draw. My concern was that Leaflet.toolbar is still in early stages, and I expect that important bugfixes will be coming out fairly regularly. I don't want you to have to update the version of Leaflet.toolbar included with Leaflet.draw every time that happens - that's going to quickly become unmanageable.

Correct me if I'm wrong, but I think that folks who want the latest version of Leaflet.toolbar should be able to simply include the source in a <script> tag after Leaflet.draw. Is that correct? If so, then I'm fine with including Leaflet.toolbar in the build for Leaflet.draw.

If we take this route, I suggest that (at the very least), we split Leaflet.toolbar out of Leaflet.draw once Leaflet.draw reaches a 1.0-level release.

[sugarjig]

Has there been any movement on this? I am very much interested in using Leaflet.draw with a customized toolbar.

[jacobtoye]

@manleyjster Sorry for the long time for getting to this. Only just found time.

The examples and documentation needs to be updated. I'm happy to do this.

Since L.Control.Draw has been removed there is no longer a way a control with both the Draw and Edit toolbars.

E.g.

map.addControl(new L.Control.Draw({
    edit: { featureGroup: drawnItems }
}));

I think re-creating L.Control.Draw as L.Draw.Control (like this naming convention better) where I create both the draw and edit toolbars would be sufficient. What do you think?

[justinmanley]

@jacobtoye - I'm happy to make the changes you suggest, but I'm a bit confused...

Did you actually merge this PR in? None of the changes I made are showing up on the tip of the master branch, either online, or when I git pull locally. Did you revert this PR after merging?

[jacobtoye]

This is tightly coupling the draw handlers to the toolbar :( The handlers need to be able to be used with or without the leaflet.draw controls. This enables people to only include one handler (or multiple) and do the manual activation as per usual Leaflet handlers.

[justinmanley]

Hmmm. I'm not sure I agree about this tightly coupling the draw handlers to the toolbar. A toolbar action is not the same as a toolbar. Toolbar handlers are basically just ordinary Handlers, except that they've got toolbarIcon defaults defined in their options object, so that we don't have to check if options.toolbarIcon is undefined all the time. Instances of ToolbarAction also have a few toolbar-related methods - _createIcon and _addSubToolbar` - but these methods aren't called until the toolbar action is added to a toolbar.

In particular, look at the code for ToolbarAction#enable and #disable. You'll see that the only difference between these methods, and those built in to Leaflet.Handler is that ToolbarAction checks to see if #addHooks and #removeHooks are actually defined before calling them, which I think is preferable behavior.

In short, even after the refactoring and the introduction of Leaflet.toolbar, Leaflet.draw handlers are perfectly happy to be called manually, outside of a toolbar, or embedded within a toolbar.

[jacobtoye]

Sorry maybe i should have said this will make the draw handlers require that Leaflet.toolbar is also included even if the features are not used.

[jacobtoye]

Work has finally allowed me some time for Leaflet.draw. I didn't realize the above restriction until I tried to get this pull working with the examples.

[jacobtoye]

These strings should be using the L.drawLocal object to handle people wanting to easily add other language support.

[justinmanley]

Good catch! I moved all of these strings into the L.drawLocal object in the new leaflet-toolbar-plugin-patches branch.

[jacobtoye]

What we want to have is essentially three parts to Leaflet.draw: the draw control, the toolbar and the draw/edit handlers. One day I think it would be better to create a new repo for the handlers.

To solve the dependency problem I think we need to create an intermediate ToolbarAction handlers that call the Draw/Edit handlers. E.g. L.Draw.polygon.ToolbarAction.

@manleyjster what do you think?

[jacobtoye]

This is how I hacked up the L.DrawToolbar.Control to pass through the options:

L.DrawToolbar.Control = L.Toolbar.Control.extend({
    options: {
        className: 'leaflet-draw-toolbar'
    }, 

    initialize: function (options) {
        L.setOptions(this.options, options);

        this.options.actions = {
            polygon: {
                action: L.Draw.Polygon,
                options: options.polygon
            },
            polyline: {
                action: L.Draw.Polyline,
                options: options.polyline
            },
            circle: {
                action: L.Draw.Circle,
                options: options.circle
            },
            rectangle: {
                action: L.Draw.Rectangle,
                options: options.rectangle
            },
            marker: {
                action: L.Draw.Marker,
                options: options.marker
            }
        };

        L.Toolbar.Control.prototype.initialize.call(this, options);
    },

    appendToContainer: function(container) {
        var baseClass = this.constructor.baseClass + '-' + this._calculateDepth(),
            className = baseClass + ' ' + this.options.className,
            Action, action,
            i, j, l, m;

        this._container = container;
        this._ul = L.DomUtil.create('ul', className, container);

        /* Ensure that clicks, drags, etc. don't bubble up to the map. */
        this._disabledEvents = ['click', 'mousemove', 'dblclick'];

        for (j = 0, m = this._disabledEvents.length; j < m; j++) {
            L.DomEvent.on(this._ul, this._disabledEvents[j], L.DomEvent.stopPropagation);
        }

        /* Instantiate each toolbar action and add its corresponding toolbar icon. */
        for (var actionIndex in this.options.actions) {
            Action = this._getActionConstructor(this.options.actions[actionIndex].action, this.options.actions[actionIndex].options);

            action = new Action();
            action._createIcon(this, this._ul, this._arguments);
        }
    },

    _getActionConstructor: function(Action, actionOptions) {
        var args = this._arguments,
            toolbar = this;

        return Action.extend({
            initialize: function() {
                Action.prototype.initialize.apply(this, args.concat(actionOptions));
            },
            enable: function() {
                /* Ensure that only one action in a toolbar will be active at a time. */
                if (toolbar._active) { toolbar._active.disable(); }
                toolbar._active = this;

                Action.prototype.enable.call(this);
            }
        });
    },
});

I need to pair the ToolbarAction with the corresponding options object.

How would you want to have Leaflet.toolbar support passing options into ToolbarAction's?

[justinmanley]

Hey @jacobtoye - how about this?

L.DrawToolbar.Control = L.Toolbar.Control.extend({
    options: {
        // default shape options
        polygon: {},
        polyline: {},
        circle: {},
        rectangle: {},
        marker: {},

        className: 'leaflet-draw-toolbar'
    },
    initialize: function(options) {
        L.setOptions(this.options, options);

        var actions = [
            L.Draw.Polygon.extend({ options: this.options.polygon }),
            L.Draw.Polyline.extend({ options: this.options.polyline }),
            L.Draw.Marker.extend({ options: this.options.marker }),
            L.Draw.Rectangle.extend({ options: this.options.rectangle }),
            L.Draw.Circle.extend({ options: this.options.circle })
        ];

        this.options.actions = options.actions || actions;
    }
});

[justinmanley]

To solve the dependency problem I think we need to create an intermediate ToolbarAction handlers that call the Draw/Edit handlers. E.g. L.Draw.polygon.ToolbarAction.

Can you explain what you mean by this?

[jacobtoye]

I've started too complete the integration of Leaflet.toolbar with Leaflet.draw in branch: https://github.com/Leaflet/Leaflet.draw/tree/integrating-toolbar

It's not yet complete and I hope I've used Leaflet.toolbar correctly. But it's getting there! I had to monkey patch a few toolbar methods. See DrawToolbar.Control.js. Basically because Leaflet.draw allows user to create a single control and specifiy options we need to pass these down to the actions.

I'm currently half way through moving out the references to Leaflet.toolbar from the draw handlers (like the edit handlers). The philosophy behind the handlers was that we want them to be stand alone. Originally polyline editing was part of Leaflet core.

[mourner]

@jacobtoye very cool to see you working on this again!