Re: GMail plugin extension

["Adi J. Sieker" <adi sieker info>]

Hi Ulrik,

On 08.09.2010 13:17, Ulrik Sverdrup wrote:
> 2010/9/7 Adi J. Sieker<adi sieker info>:
> > I just pushed an update to the GMail plugin to
> > git://github.com/adsworth/kupfer.git
> > The plugin now downloads all data and the contact leaf now provides a
> > content source,
> > which contains most data from the contact. Currently that's all email
> > addresses as an EmailContact,
> > all phone numbers, postal addresses and IM account's as TextLeafs.
> >
> > All feedback welcome
>
> Hi Adi,
>
> In general, it's a good idea. However as it is now, this change
> conflicts with contacts grouping (since grouping uses the contents
> "slot" to show the contacts grouped together); this patch does away
> with grouping all together of course (no longer using EmailContact).
> So there is no direct conflict, but it needs to be explicitly
> addressed somehow. It doesn't mean that this can't be solved, or that
> this information could not be exposed in a different way.
I tried to get my head around what a GroupingLeaf does, but I didn't 
find any documentation on that. Google just shows commit messages.
So if I could get an introduction to GroupingLeaves or you have 
suggestion as to how I can rework it.

> If you don't mind, the code review reads like this:

I did ask for it. :)

> The commit log message must adhere to git standards
> (the first line must not be longer than 60-70 letters ideally).
Since I pushed the commit already. I recreated my github fork, cloned, 
commited and pushed again. So if someone added me as a remote and 
fetched it would probably be best to delete the remote and add it again.

> The patch also has some whitespace
> problem (git diff  should highlight these, if you have git configured
> for color, else use git diff --check);

Corrected the trailing whitespaces.

> and get_items is better written
> as a generator.
And reworked get_items.

Adi