Review Request: Initial avatars support

[Martin Klapetek]

> On April 7, 2011, 4:22 p.m., David Edmundson wrote:
> > Pretty solid patch.
> > 
> > You don't have anything that updates the image when it changes externally. This needs doing (you can make that a future thing, as long as you write it down somewhere).
> > Does this work when the first account is offline?

It should load the updated avatar as well - see line 286 - this loads the avatar from account and displays it in the menu. It does not update the button icon though and that's because I think when you once set the avatar with that contact list, you intend to keep it (and you have the possibility to select the new one from menu).

I think it should work even when the account is offline, because the avatars are cached, so as long as the account is ready, it should work. I think.


> On April 7, 2011, 4:22 p.m., David Edmundson wrote:
> > main-widget.h, line 71
> > <http://git.reviewboard.kde.org/r/101048/diff/1/?file=13908#file13908line71>
> >
> >     This needs merging with onGenericOperationFinished. (which you don't seem to have - update)

Yes, I'll merge it.


> On April 7, 2011, 4:22 p.m., David Edmundson wrote:
> > main-widget.cpp, line 760
> > <http://git.reviewboard.kde.org/r/101048/diff/1/?file=13909#file13909line760>
> >
> >     avatat -> avatar

Damn I should turn on a spell checker.


> On April 7, 2011, 4:22 p.m., David Edmundson wrote:
> > main-widget.cpp, line 823
> > <http://git.reviewboard.kde.org/r/101048/diff/1/?file=13909#file13909line823>
> >
> >     Really you should do some KIO stuff here as KFileDialog can return something on the network, but QFile won't open it.
> >     
> >     For now I'd be happy with a comment that says it needs doing.

True, haven't thought of that. I'll add a FIXME.


> On April 7, 2011, 4:22 p.m., David Edmundson wrote:
> > main-widget.cpp, line 842
> > <http://git.reviewboard.kde.org/r/101048/diff/1/?file=13909#file13909line842>
> >
> >     You don't close the image if everything processes correctly.
> >     
> >     Fortunately a QFile closes itself in the destructor, which means you don't actually need to do it all.

Uhm...happens :)


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101048/#review2463
-----------------------------------------------------------


On April 7, 2011, 3:33 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101048/
> -----------------------------------------------------------
> 
> (Updated April 7, 2011, 3:33 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This patch adds initial avatars support plus KConfig, which holds the selected avatar.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 548e8c880b5ef921606c7407b92e2e1650faef86 
>   accounts-model-item.cpp 1d207e7eebb6d286d0fc075dfa4e90628ac92bcc 
>   main-widget.h e310b35d475509f5683a0abe15f21bbb1d8f122e 
>   main-widget.cpp 2e049a13561a1dc4ca0b7117a52ed713d775b9bd 
>   main-widget.ui bc1fb9bf030a927cf635ed1f73d71d6ee33675b2 
> 
> Diff: http://git.reviewboard.kde.org/r/101048/diff
> 
> 
> Testing
> -------
> 
> Tried to change the avatar and it worked.
> 
> 
> Screenshots
> -----------
> 
> Avatars menu
>   http://git.reviewboard.kde.org/r/101048/s/114/
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110407/8578394f/attachment.htm