Merge layers branch to master?

[shawnbot]

I think it's time. I've battle tested the layers stuff on a couple of projects and made some tweaks of my own, and it would be a shame if this work continued to hide in a branch.

Are we ready for a big version bump yet?

[tmcw]

I'll +1. I haven't used the layers branch yet, so might want to kick the tires & review, but we should add this as a 0.22.0 or so, and have a little time to really refine it before 1.x.x

[RandomEtc]

Tentative +1! It's an essential feature and it's good work, but...

According to the branches view it's heavily behind master: https://github.com/stamen/modestmaps-js/branches (22 ahead, 108 behind at current time of writing).

I think the best way to deal with this is to make a new branch from the current layers branch and then git rebase it against master. That's how I merged the last big rolled up branch and I think it worked well and made the changelog much easier to write. But it's going to take a bit of time to resolve the various commits and make sure stuff from master doesn't get clobbered in the process.

Right now the comparison is quite hard to follow - master...layers - but my initial observations are:

• the setupDimensions function breaks the fixes that removed the default size in v0.18.5
• the draw function needs a very careful read, because it's moved into layer.js and may have missed fixes since the branch was created (e.g. doesn't use moveElement)
• (could start a holy war, but) the new layer stuff uses a different brace style to the rest of the library which is otherwise pretty consistent
• the two new provider api features getTile and releaseTile could use some tyre kicking from everyone I think - IIRC they were added to support other kinds of tile (e.g. rendering canvases), if so there should be an example of doing that to make sure that the functionality isn't overlooked in future versions

I've got some time set aside in two weeks to work on MM stuff with @migurski and @tmcw and others, I'll try to find a time to prepare a merge branch then if nobody gets to it first.

[shawnbot]

Awesome. I'm happy to help with the merge as well. I've implemented a GeoJSON provider (which renders Point geometries as HTML markers) and a sprite-based image layer using the new Tile interface and it seems to work well, but it would be good to have more than one person using it and add some more test cases.

[shawnbot]

Er, the new Layer interface.

[RandomEtc]

Status from yesterday's hacking session:

• layers-merge branch ready for tyre kicking
• MM.Layer is a first class member of the MM hierarchy, but the API is still in flux...
• ...a layer is currently anything with a draw method, a destroy method, a map property (set by the parent Map) and a parent property (must be a valid DOM element when the Layer is added to the Map).
• instead of adding providers directly, you now add layers. At the moment, MapProviders (and template strings) can be coerced into Layers to make it simple, but set/getProvider is gone in favor of a bunch of add/remove/insert/set/get Layer methods, to be documented.
• because of the above API change, this will be version 1.0.0 so we should be sure to get the API right before pushing to master :)

If you're following along, please try the layers-merge branch and the layers example (as well as the other examples, and your own projects) and file new issues or comment here if anything seems amiss. A few niche bits of functionality like pyramid loading are not working right now, but those should be easy fixes.

Map cleanup (the new .destroy method) may be affected too - if anyone knows a good tool for finding memory leaks, please let us know.

Also @tmcw reported problems in Opera - any word on what the error is? The layers sample seems to work for me, I haven't tried others.

[tmcw]

Opera problems were fixed in 27eb31c

[tmcw]

Okay, did another pass at the branch again, and things look okay, though, barely having used pyramid loading or been in that part of the code, I'm not confident to fix that part.

What are the main steps we need to take before merging into master, as a 1.0.0-alpha or something of that sort?

[RandomEtc]

For 1.0.0:

• API docs updated on wiki
• tests updated for new APIs (pretty sure I broke some stuff, haven't checked yet)
• package.json/CHANGELOG/start.js update
• remove TilePaintingProvider bits, for now
• as much of https://github.com/stamen/modestmaps-js/issues?milestone=1&state=open as anyone can get to

I'd also like to propose MM, already used throughout examples and source, rather than mm for the new short namespace.

[shawnbot]

+1 on the MM namespace.

Possibly unrelated, but important if we're considering the inclusion of the newer examples (GeoJSON markers and sprite tiles):

I had to modify the sprite tiles example pretty heavily after pulling the latest from layers-merge because, in the new API, providers no longer appear to know anything about tile sizes. They could when tileSize lived in the provider, but the recent change suggests that maps can only have a single tile size.

Is this right? Because it feels oddly limiting that the provider know nothing about tile sizes.

[tmcw]

Cool, renamed mm to MM in a905543 - not a big difference.

Currently tests are passing, but they don't have much coverage of the new code.

[RandomEtc]

Regarding the home of tileSize - I'm not sure to be honest.

My thinking was that tileSize and projection are more coupled than tileSize and provider, because it's tileSize and projection that dictate which coordinates are loaded for a given lat-lon. If you make different tile sizes for each layer then you also need a different projection because your zoom levels won't match.

[All assuming zoom level 0 is a single tileSize tile.]

Basically I settled on having tileSize and projection live in Map because the Map uses a coordinate for its current center. If we allow different tile sizes for each layer (or provider) then we need to allow for a way to convert the center coordinate between layers. Something somewhere has to get smarter... so I just chose the dumb thing and oddly limited it :)

[tmcw]

• Big doc update (written against a potential v1.0.0, includes namespace change, expanded docs in general, layers)
• Fixed Non-strict-zoom setExtent method #62, Clean up setSize API #54, Add TouchHandler by default #75, Remove org. from modestmaps #26, add core support for multiple tile layers / providers #11
• Forward-version start.js in e490707

[shawnbot]

Re: tileSize living in Map, I totally understand. What I'm suggesting is that providers should at least have access to the tile size; I'm just not sure what that would look like. Could getTileUrl() and getTile() receive it as a second parameter?

[RandomEtc]

That seems right, and non-breaking since js supports optional args very cleanly. It would clean up the CloudMade provider too, which needs tile size for its URLs.

The only hesitation I have is that I think getTile might be a good candidate for a method which takes a callback, but that would be a bigger change than any of us is proposing right now, so I say go for it :)

[tmcw]

Okay - so can we split out a ticket for tileSize being passed as a parameter? I'll write one for a async getTile, which I actually think is pretty essential for flexibility (and very useful for my PhoneGap experiment)

[tmcw]

Okay, so I've started work in the async branch for a callback-enabled getTile. Got an early, slightly wonky test going on with canvas elements and Kothic js rendering.

But I think that can wait for a 1.1.0 release, since it'll be backwards-compatible and should be fine with the API design as-is. It shouldn't hold back a 1.0.0 - everyone else game for that?

[shawnbot]

+1 on putting async and experimental canvas stuff into 1.1.0.

My layers-merge branch is ahead of stamen/layers-merge by a few commits, including a merge from yours (see: network). The changes include:

• MM.MapExtent class for more easily defining and manipulating map extents, and an "extent-selector" example that shows it off
• MM.Hash handler for updating map positions from location.hash, a la org.polymaps.hash(), and a corresponding example (also, a build including this is now referenced in TileStache's preview output so that previews are navigable in iOS)
• constructor args support in MM.TouchHandler
• constructor args support in MM.MouseWheelHandler and a redo of the "anyscale" example using it (instead of the custom code)
• a standalone GeoJSON data example, which is currently out of commission because Oakland Crimespotting's API isn't returning data. (I could update this to use a static file for demonstration purposes, but having it use JSON-P could be really illustrative for potential users, so I'm torn.)

Okay to pull those into stamen/layers-merge in preparation for a 1.0.0 release?

[tmcw]

Can you explain more about the necessity of MapExtent? Just looked at it briefly and I'm not sure whether the utility functions are needed elsewhere in core.

I'd kind of be against MM.Hash. Not hugely, but in implementing a hash control, I'm convinced that it's not an area where you can have a one-size-fits-all implementation - given the odd support/nonsupport for hashchange and pushState across browsers. Also, the Wax hash implementation is quite a bit faster than MM.Hash by deferring hash changes, which are pretty expensive performance-wise. That should probably go in if a hash control does.

Besides that, the other changes look good.

[shawnbot]

MapExtent isn't necessary, but it's a lot easier to use than lists of Location instances. I've found myself using it a lot because it was an integral part of the AS3 implementation, and I think a lot of users would appreciate its inclusion.

I'm also willing to argue strongly for inclusion of Hash, especially if it can be made to work more like Wax's implementation. It's exactly the type of very useful (but slightly tricky to get right) thing that would be silly to force people to implement themselves or include a separate script to use.

[tmcw]

Saw you made the change in your layers-merge branch. Sweet! Just merged your changes into layers-merge and I'm game to merge into master.

[tmcw]

Okay, layers-merge merged, and I've updated the package.json to 1.0.0alpha1, added you & removed the expresso dependency. Cool to rename this to 1.0.0?

[tmcw]

Closing here - when 1.0.0 tickets are finished, we'll tag a release