Re: [Merge request] New map source infrastructure

[Jiří Techet <techet gmail com>]

> 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.

Agree - this was actually a third possibility I had in my mind but
forgot to write :-). It could be something like

champlain_map_source_factory_create_cached_source

(if you want something less verbose, come with a better name :-)

>      * 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.

The only problem is that you have to specify these during construction
(we create the cache directory in the path with sqlite database in it
when it doesn't exist yet). But apart from _new_full() we can have
_new() with some sane default values (100MB database [or rather 60day
database now], .cache/champlain as the cache directory and permanent
cache)

>      * Regarding coding styles the only errors I've seen are missing
>        spaces between function names and ().

I used astyle --style=gnu, but it doesn't make the spaces. I also
looked at indent, but the gnu style is a bit different as well. Do you
use some pretty printer (and which options) or do everything manually
(if not, I think it would be easier to start using some pretty printer
instead of checking the sources manually).

>      * champlain_map_source_chain_push_map_source is a bit verbose.
>        Let's go for champlain_map_source_chain_push. :)

OK, the hardest part about coding is inventing the function and
variable names :-)

>
>> 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.

Ah, great, this is what I have been waiting for - instead of modifying
my patch to fit the existing work, people start modifying their
patches to fit my work :-).

>
>> (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!
>

Jiri