Get new Annotations 2.0 running again

[jywarren]

This was running in a recent version, so it shouldn't be a big deal.

Note: http://publiclab.org/notes/justinmanley/09-02-2014/mapknitter-annotations-first-live-release

[jywarren]

@manleyjster - sorry, i know you're busy, but this one is turning out to be a little obscure to me, and I wonder if it's something about recent versions of Leaflet.Toolbar -- do you have a moment to take a look?

[jywarren]

It's failing on line 25 of Annotations.js -- at addTo(map):

        new MapKnitter.Annotations.Toolbar({
        position: 'topright',
        edit: { featureGroup: this._drawnItems },
        draw: drawOptions
    }).addTo(map);

with Uncaught TypeError: Cannot set property 'textbox' of undefined at line 2048 of leaflet.draw-src.js

[justinmanley]

As far as I can tell, MapKnitter isn't using Leaflet.toolbar at all.

The bower.json file loads Leaflet/Leaflet.draw#0.2.3, which doesn't use Leaflet.toolbar. This is borne out by the way that the toolbar is defined in Annotations.Toolbar.js - MapKnitter.Annotations.Toolbar is created to extend L.Control.Draw and L.DrawToolbar - neither of which are defined by Leaflet.toolbar.

The Leaflet.toolbar-based toolbar for Leaflet.draw is defined in manleyjster/Leaflet.draw#leaflet-toolbar-plugin, and I have an outstanding pull request (Leaflet/Leaflet.draw#354) for merging it into Leaflet/Leaflet.draw. If you want to use the Leaflet.toolbar-based toolbar, you'll need to set manleyjster/Leaflet.draw#leaflet-toolbar-plugin as the remote for leaflet-draw in the bower.json file.

This is not a Leaflet.toolbar issue, but I'm happy to take another look at it on Saturday. This issue certainly involves Leaflet.Illustrate.

[jywarren]

Ah, I guess your Leaflet.Illustrate work in MapKnitter predates your
Leaflet.toolbar work, perhaps?

It'd be great if you could take a look Saturday. I'm currently working on
Ajaxified comment posting. Thanks, Justin!

On Fri, Feb 20, 2015 at 2:25 AM, Justin Manley [email protected]
wrote:

As far as I can tell, MapKnitter isn't using Leaflet.toolbar at all.

The bower.json file loads Leaflet/Leaflet.draw#0.2.3, which doesn't use
Leaflet.toolbar. This is borne out by the way that the toolbar is defined
in Annotations.Toolbar.js - MapKnitter.Annotations.Toolbar is created to
extend L.Control.Draw and L.DrawToolbar - neither of which are defined by
Leaflet.toolbar.

The Leaflet.toolbar-based toolbar for Leaflet.draw is defined in
manleyjster/Leaflet.draw#leaflet-toolbar-plugin, and I have an
outstanding pull request (Leaflet/Leaflet.draw#354
Leaflet/Leaflet.draw#354) for merging it into
Leaflet/Leaflet.draw. If you want to use the Leaflet.toolbar-based
toolbar, you'll need to set
manleyjster/Leaflet.draw#leaflet-toolbar-plugin as the remote for
leaflet-draw in the bower.json file.

This is not a Leaflet.toolbar issue, but I'm happy to take another look at
it on Saturday. This issue certainly involves Leaflet.Illustrate.

—
Reply to this email directly or view it on GitHub
#87 (comment).

[jywarren]

Managed to make time to dig into this again; I found that Leaflet.Draw's L.DrawToolbar's initialize method is not calling it's inherited L.Toolbar's initialize method, which sets up this._modes (see here). So later, around line 2045 of leaflet.draw-src.js, when _modes is accessed with the key "textbox", we get the Uncaught TypeError: Cannot set property 'textbox' of undefined error.

Not sure how this kind of error is cropping up in Leaflet.Draw (v0.2.3) due to our code, but I have to wrap up for the day. Will look tomorrow.

[jywarren]

OK I figured it out! Gosh that was obvious in retrospect, like so many bugs. L.Toolbar is overridden by Leaflet.Toolbar, which is used in Leaflet.DistortableImage -- so if we then load Annotations on top of that, it tries to use the OLD L.Toolbar from Leaflet.Draw.

Commenting leaflet.toolbar.js in application.js makes Annotations 2.0 work again -- although it breaks Leaflet.DistorableImage, which no longer has the new Leaflet.Toolbar to rely on. Now going to try to ensure the original L.Toolbar isn't overridden.

[btbonval]

That doesn't sound at all obvious. Good work!
On Apr 16, 2015 1:29 PM, "Jeffrey Warren" [email protected] wrote:

OK I figured it out! Gosh that was obvious in retrospect, like so many
bugs. L.Toolbar is overridden by Leaflet.Toolbar, which is used in
Leaflet.DistortableImage -- so if we then load Annotations on top of that,
it tries to use the OLD L.Toolbar from Leaflet.Draw.

Commenting leaflet.toolbar.js in application.js makes Annotations 2.0 work
again -- although it breaks Leaflet.DistorableImage, which no longer has
the new Leaflet.Toolbar to rely on. Now going to try to ensure the original
L.Toolbar isn't overridden.

—
Reply to this email directly or view it on GitHub
#87 (comment).

[jywarren]

OK, i switched all annotation code off (in a separate compiled js file) and only turned it on for the Annotations page, and simultaneously turned off interactivity (readOnly = true) for Leaflet.DistortableImage. The two no longer conflict (and also you can't warp images while writing annotations).

Now I have to get annotation writing working -- the code is loading existing annotations but not saving new ones. Then styling, UI issues, etc. but we are doing pretty well vs a couple hours ago!

[jywarren]

Closing this and moving to #163 to break it out. Nice!