Added support for customzing the data source of the downsample bitmap

[aadnk]

I don't know if you were planing on adding this yourself, but I figured I might as well take a shot.

The simplest and probably most consistent metod is to reuse MapTileDecoder for this purpose, using a "setDownsampeDecoder" method in MapView. It's a bit wierd for a tile decoder to be used in this manner, but it doesn't have any specific knowledge about tiles or zoom levels. Perhaps the interface really should have been called BitmapLoader or AssetLoader?

In fact, it might have been better if the MapTileDecoder interface had been written like so:

public interface MapTileDecoder {
    public Bitmap decode(ZoomLevel level, int row, int column, 
                         Context context);
}

And then the default tile decoder would be a AssetTileDecoder that delegates to a AssetLoader with a signature similar to today's MapTileDecoder:

public class AssetTileDecoder implements MapTileDecoder {
    public AssetTileDecoder(AssetLoader loader) ...

    // Every %col% and %row% in the given path is processed as now
    public registerZoomLevel(ZoomLevel level, String path) ... 

    public Bitmap decode(ZoomLevel level, int row, int column, Context context) {
        String path = formatMap(getFormatFromLevel(level), row, column);
        return loader.decode(path, context);
    }
}

public interface AssetLoader {
    public Bitmap decode(String asset, Context context);
}

This rewritten MapTileDecoder would have been useful for me when I implemented a Tile Decoder that reads tiles directly from a large PNG file on disk (using BitmapRegionDecoder), but it's still possible currently - I just had to parse the string to get the zoom level, row and column index.

[moagrius]

I agree that downsamples should probably be customizable as well (if someone had a dynamically assigned tile set on a remote server, for example, they'd be unable to provide a downsample in the .apk).

I agree with many/most of your assertions. My initial thoughts were that it should re-use as much logic as possible, and not require two separate method invocations (just one call to set the decoder). I'd imagined the decoder would have (require) another, separate method signature for the downsample (e.g., decodeDownsample), which would get passed whatever arguments were appropriate (at least the string and a context reference).

Reinstating downsample support and custom decoding were late adds, so I'm sure it probably could have been done more consistently the first time. I'd really like to keep the public API from breaking existing installs, and your pull does that, but it does have the extra method call requirement, which I'd also hoped to avoid.

Let me sit on this one for a while and consider, and see if anyone else wants to chime in.

Thanks for contributing - your points are well considered and presented.

[aadnk]

Yeah, you should never rush API design - if in doubt, just leave it out.

I didn't want to modify MapTileDecoder for fear of breaking backwards compatibility, but I don't mind if you do go that route and add a new decodeDownsample method. It could mean less reuse, but at least you don't accidentally use your tile decoder to for the downsampler.

In fact, it's my interface proposal that constitute the most breaking change - you would have to be very careful and ensure addZoomLevel() worked as before and so on. And it's not that important - it's just better to pass strongly typed objects as opposed to strings in my opinion.

[moagrius]

maybe something like

MapTileDecoder interface remains the same add another interface DownsampleDecoder with it's own decode signature (either named or parameterized to avoid ambiguity with MapTileDecoder.decode)

Have the built-in decoder implementations (assets, and http) implement both.

If the decoder set to the mapview implements MapTileDecoder, pass that to TileManager. If it implements DownsampleDecoder, pass that to the DownsampleManager.

[aadnk]

It would be a bit confusing if setTileDecoder() also updated the downsample decoder, but it would work.

It would be slightly better if the method was called setDecoder() though,

[moagrius]

yeah... we could keep both and just add a new signature setDecoder (that did the same thing), maybe deprecate setTileDecoder

[moagrius]

For the next minor updated, I've added some basic support for this kind of operation, but for the next major revision (TileView, #57) I'm going to rewrite this API completely. Will close for now, to be taken up then (in another repo) - thanks for your contribution.

[aadnk]

Sounds good. I'll keep an eye out for when you release TileView. :)