[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kopete-devel
Subject:    Re: [kopete-devel] Review Request: Add the ability of take pictures
From:       "Alex Fiestas" <alex () eyeos ! org>
Date:       2009-09-30 11:40:12
Message-ID: 20090930114012.9055.55269 () localhost
[Download RAW message or body]



> On None, Matt Rogers wrote:
> > Could we call the dialog class AvatarWebcamDialog instead?

Dialog renamed.


> On None, Matt Rogers wrote:
> > /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarselectorwidget.cpp, lines 211-224
> > <http://reviewboard.kde.org/r/1573/diff/2/?file=11260#file11260line211>
> > 
> > you use d->mainWidget.listUserAvatar in multiple places. You might want to make a \
> > variable out of that.

I haven't change this because all the file is accessing listUserAvatar in the same \
way, so I'd like to keep the coding style. If you want we can change all the accesses \
adding it to the private class for example.

Thanks for all the review!


> On None, Matt Rogers wrote:
> > /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarselectorwidget.cpp, line 256
> > <http://reviewboard.kde.org/r/1573/diff/2/?file=11260#file11260line256>
> > 
> > Yeah, there's no point in modifying something that isn't already modified by this \
> > patch. The whitespace could be cleaned up in a separate patch.

Removed in a posterior commit.


> On None, Matt Rogers wrote:
> > /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarselectorwidget.cpp, lines 118-121
> > <http://reviewboard.kde.org/r/1573/diff/2/?file=11260#file11260line118>
> > 
> > it's cleaner, IMHO, to do the following:
> > 
> > using Kopete::AV;
> > VideoDevicePool* devicePool = VideoDevicePool::self();
> > devicePool->scanDevices();
> > devicePool->size();
> > 
> > etc.
> > Seems more readable to me.

Done!


> On None, Matt Rogers wrote:
> > /trunk/KDE/kdenetwork/kopete/libkopete/CMakeLists.txt, lines 149-153
> > <http://reviewboard.kde.org/r/1573/diff/2/?file=11256#file11256line149>
> > 
> > X11_LIBRARIES expands to nothing on Windows, so there's no need for the \
> > conditional. I think they disable the whole avdevice directory on windows \
> > anyways.

Conditional removed, thanks


- Alex


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


On 2009-09-12 01:50:35, Alex Fiestas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1573/
> -----------------------------------------------------------
> 
> (Updated 2009-09-12 01:50:35)
> 
> 
> Review request for Kopete.
> 
> 
> Summary
> -------
> 
> If the user has a webcam, a button will appear in his AvatarManager, once it is \
> clicked the avatarfromwebcamdialog will appear, showing the frames captured from \
> the device. Then the user will have to click on OK button and crop the image.  
> A few points:
> 1-I've created a new internal method called cropAndSaveAvatar which shows the crop \
> dialog and then, save the avatar. This method is shared between "from webcam" and \
> "add picture" methods. 2-I wonder if the dialog should be renamed to "webcamdialog" \
> or something like this, so we can open it a bit more to be used by everybody. 
> 
> Diffs
> -----
> 
> /trunk/KDE/kdenetwork/kopete/libkopete/CMakeLists.txt 1022342 
> /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarfromwebcamdialog.h PRE-CREATION 
> /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarfromwebcamdialog.cpp PRE-CREATION 
> /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarselectorwidget.h 1022342 
> /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarselectorwidget.cpp 1022342 
> /trunk/KDE/kdenetwork/kopete/libkopete/ui/avatarselectorwidget.ui 1022342 
> 
> Diff: http://reviewboard.kde.org/r/1573/diff
> 
> 
> Testing
> -------
> 
> I've tested almost all possible cases, except build it in windows. I've tried to be \
> windows aware, so the new videodevice usage won't break anything, but I can't test \
> it!. 
> 
> Thanks,
> 
> Alex
> 
> 

_______________________________________________
kopete-devel mailing list
kopete-devel@kde.org
https://mail.kde.org/mailman/listinfo/kopete-devel


[prev in list] [next in list] [prev in thread] [next in thread]