On Mon, 2010-01-18 at 16:00 +0100, Jiří Techet wrote:
>
> OK, done. I had to modify MemphisMapSource much more than I originally
> expected and basically took your branch and (with a great help of
> kdiff3) merged my changes with it and made all extra necessary changes
> (so it was a very manual --onto ;-). So now it appears as a single
> commit because I wasn't able to preserve the git history.
Cool. Well, keeping your different commits would have been nice
though ;)
> There are two issues remaining:
>
> Memphis doesn't indicate when a tile cannot be rendered because of
> missing vector data for that area and generates empty tile. This
> however means that a chain like
>
> MemphisMapSource -> NetworkTileSource
>
> won't work as expected (i.e. to load tiles from network from the areas
> for which we don't have vector data). I plan to file a bug report for
> this.
Yes it should behave as other sources.
> The second issue is it that I'm not quite sure now what the factory
> should return when you create a map source. Before it was a map source
> together with cache (plus error tile was drawn when the tile couldn't
> be loaded). Now we have 3 separate map sources for all these things
> (FileCache, NetworkTileSource, ErrorTileSource). We have two options
> what to return now:
>
> 1. A complete chain FileCache->NetworkTileSource->ErrorTileSource -
> the main disadvantage is that user applications will see the chain as
> a whole but it won't be possible for them to change properties of the
> sub- map sources individually (this is particularly problematic with
> MemphisTileSource).
>
> 2. Only the requested map source. This however means that users have
> to create the chain manually. Also if they upgrade to 0.6 without
> reading the documentation, there will be millions of questions like
> "why isn't the map source cached any more?" (because it'll create
> NetworkTileSource only without any cache).
>
> Despite the incompatibility with previous libchamplain versions, I
> tend to prefer the second solution. The first one hides everything
> from the user, which might cause problems like when the user is
> casting the map source to e.g. NetworkTileSource (because he thinks
> that he uses this source), but in fact the source is the complete
> chain. I changed the demos to use the second approach so you can see
> what needs to be changed in the client applications (see
> launcher-gtk).
OK looking at the demos' code I have 4 comments:
* the api seems fine but we should provide helper functions that
will build this chain in one call. I doubt so many users will
have to toy with the internals of the chains.
* I don't like how you have to specify the cache's settings on
creation, especially the cache's path. Those should be
configurable but should not be passed every time you have to
create a cache.
* Regarding coding styles the only errors I've seen are missing
spaces between function names and ().
* champlain_map_source_chain_push_map_source is a bit verbose.
Let's go for champlain_map_source_chain_push. :)
> Unless you have some interface-related comments to my patch, I'll
> start documenting it and after this is done, I plan to optimize the
> cache a bit.
I am currently making changes to ChamplainCache in 0.4.x, we should make
sure the 0.5 api reflect those:
* The cache is now date based. Instead of fixing a size for the
cache, we are fixing how old can tiles be on disk. So I am
changing the purge function to list all files and delete all
files older than 60 days. This is to be more friendly with
other tile based app on the Maemo as we are going to share the
same cached files. I have written an email about that, I'll
send it soon. And don't worry, I'll port my changes to 0.5
myself.
> (By the way, the problem why the local rendering demo wasn't running
> before was the change of MapSource base class to InitiallyUnowned -
> the demo just unref()ed it after passing to view which destroyed it
> because it was just sink()ed by the view.)
Good catch! those are changes that have been done since the original
branch was written. That's the price to pay for not merging the code
ASAP.
Thanks!
Attachment:
signature.asc
Description: This is a digitally signed message part