----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101135/#review2670 -----------------------------------------------------------
Ship it! Seems pretty damn good. Few very minor comments: main-widget.cpp <http://git.reviewboard.kde.org/r/101135/#comment2365> Should we pass the parent widget argument to KFileDialog::getImageOpenUrl so that the dialog loads next to the correct window. Also we should set a caption such as "Select an image to use an an avatar". main-widget.cpp <http://git.reviewboard.kde.org/r/101135/#comment2366> We should check (or Q_ASSERT) that the qobject_cast was successful before using fetchJob. main-widget.cpp <http://git.reviewboard.kde.org/r/101135/#comment2364> I know this isn't your code, but should this be here at all? I would have expected userAccountIconButton to already have code to change upon the account->avatarChanged() signal, which means we don't need to do it ourselves here too. - David On April 15, 2011, 5:59 p.m., Dario Freddi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101135/ > ----------------------------------------------------------- > > (Updated April 15, 2011, 5:59 p.m.) > > > Review request for Telepathy. > > > Summary > ------- > > This patch implements proper avatar fetching in the contact list, leaving out > a FIXME. The additional class is library-ready, as it needs to be used in > other places than the CL. It uses KIO to be able to fetch (asynchronously) > the file from every protocol KIO supports (so even http, ftp and stuff) > > > Diffs > ----- > > CMakeLists.txt 2b8506267a9966241d5dfb9ca2552afed11f7649 > fetch-avatar-job.h PRE-CREATION > fetch-avatar-job.cpp PRE-CREATION > main-widget.h 310dfb6864c3efdc27a97f587595f385b3b3b2a7 > main-widget.cpp 5fa5490ca950d2ffcaca18b1d56b7327cbc72906 > > Diff: http://git.reviewboard.kde.org/r/101135/diff > > > Testing > ------- > > Needs testing, as I couldn't test myself! > > > Thanks, > > Dario > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
