Re: [Merge request] New map source infrastructure

On Mon, 2010-01-18 at 23:17 +0100, Jiří Techet wrote:
> > 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

Sounds good.

> (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)
A default constructor solves this indeed.

> >      * 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 :-)

Also there is no "style guide" lines for that. :)


Attachment: signature.asc
Description: This is a digitally signed message part

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]