Split GridLayer into Grid (DOM-less) & GridLayer

[mourner]

Following @patrickarlt suggestions in #2344, I made a quick experiment in the grid branch, splitting the current GridLayer implementation (534 lines) into two classes — Grid (pure DOM-less grid logic with wrapping/bounding/etc, 299 lines) & GridLayer (handles DOM tiles, positioning, anim etc., 277 lines).

I'm not yet satisfied with how it looks because of too much super calls (L.Grid.prototype.method.call(this, a, b)) and a bit bulkier code overall (31 more sloc), but the direction might be promising, as it logically splits the huge GridLayer implementation in half, and the current 534 lines for one class is too much and looks pretty messy.

I'm sure @jfirebaugh won't like it though, he hates inheritance. :) I don't currently see a clean way to deal with GridLayer complexity without introducing another class in inheritance chain though. cc @tmcw

[tmcw]

Could this be refactored as encapsulation, so a GridLayer has a Grid as a member, instead of inheriting from it?

[mourner]

@tmcw I'll try that, although most of my attempts at using composition in place of inheritance usually result in a much more verbose code, which I try to avoid in Leaflet.

[patrickarlt]

@mourner did doing DOM related stuff in addTile, removeTile methods not work out?

I like the look of this, even with the extra super calls, maybe just adding a few comments to the super calls for clarification would help the readability.

[mourner]

@patrickarlt as you can see here, there's a lot more DOM-related logic involved than just creating and removing tiles, like animation, upscaling etc.

[patrickarlt]

I gave the encapsulation approach a try this weekend and ran into a problem. If you try to make Grid a member of GridLayer. The grid property doesn't know where to look for createTile since it isn't in the prototype chain anymore.

The only way I can see around this is to pass the createTile function to Grid as an option which feels a little weird and would probably require refactoring a lot of other classes.

I think this might result in more verbose code as @mourner suggested.

[mourner]

GridLayer was rewritten substantially so we should do it from scratch anyway after the pyramid tiles stuff, so closing here.

[patrickarlt]

Sounds good the pyramid tiles stuff looks sweet. Any comment on if L.Grid will be in 0.8? I was planning on using it for some refactoring in Esri Leaflet. If its not planned I might just take the code out of here and port it to my L.VirtualGrid plugin (https://github.com/patrickarlt/leaflet-virtual-grid) and maintain it there.

[mourner]

@patrickarlt not sure yet, we'll see how it looks when it's done in the #2382 branch.

[patrickarlt]

@mourner sounds good I'll keep an eye on that branch

[patrickarlt]

A bit of an update on this. I started by refactors which I wanted to use L.Grid for and ended up needing more functionality then was provided in L.Grid. I'm going to roll the changes I made to the grid with my extra features into my L.VirtualGrid plugin

[patrickarlt]

I've got this integrated into L.VirtualGrid now. I needed to be able to track when cells (tiles) enter and leave the map view which is the major thing L.VirtualGrid adds.