Review Request: Add a class ContactGridWidget (mostly imported from ktp-send-file)

[Daniele Elmo Domenichelli]

> On Jan. 15, 2012, 2:14 p.m., David Edmundson wrote:
> > I'm not sure it needs a new lib, it could just be a "Widgets" folder inside the KTp lib.
> > I think models is separated purely because they're a bit sucky rather than something we should do for everything in KTp.
> > 
> > Could you check with Dario's opinion and go with whatever he says.

The problem with that is that the models library depends on the common library, so the common library cannot depend on the models library...


> On Jan. 15, 2012, 2:14 p.m., David Edmundson wrote:
> > KTp/Widgets/contact-grid-widget.h, line 67
> > <http://git.reviewboard.kde.org/r/103702/diff/1/?file=46612#file46612line67>
> >
> >     I think in future we should expose our filter proxy model directly, that way we don't have to put everything in 3 places
> >     
> >     (the proxy, here and the contacts listview)
> >     
> >     Just brainstorming, this is fine for now.

Since this is a brainstorming... I think it is actually nice that you just have to add the widget and use the methods from that... But whatever you choose it's fine for me.


> On Jan. 15, 2012, 2:14 p.m., David Edmundson wrote:
> > KTp/Widgets/contact-grid-widget.cpp, line 65
> > <http://git.reviewboard.kde.org/r/103702/diff/1/?file=46613#file46613line65>
> >
> >     Try and avoid a hardcoded 80. I think we should use the icon size specified by whoever is using this class (set by QAbstractItemView::iconSize).
> >     
> >     Does it read clearer as this:
> >     
> >     avatar = avatar.scaled(avatar.size().boundedTo(avatarRect));
> >     
> >

This is actually copy&pasted code, but I will change it...


> On Jan. 15, 2012, 2:14 p.m., David Edmundson wrote:
> > KTp/Widgets/contact-grid-widget.cpp, line 185
> > <http://git.reviewboard.kde.org/r/103702/diff/1/?file=46613#file46613line185>
> >
> >     I think this will crash if called without a selection.
> >     
> >     .value() returns 0 and then you call ->contact() on it.
> >     
> >     Even if no-one should call this without a selection, we should still avoid crashing.

I will add a check here and in the selectedAccount method


> On Jan. 15, 2012, 2:14 p.m., David Edmundson wrote:
> > KTp/Widgets/flat-model-proxy.h, line 2
> > <http://git.reviewboard.kde.org/r/103702/diff/1/?file=46614#file46614line2>
> >
> >     This arguably belongs in "Models" rather than Widgets.

Agreed...


- Daniele Elmo


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


On Jan. 15, 2012, 10 a.m., Daniele Elmo Domenichelli wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103702/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2012, 10 a.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> This patch adds a class ContactGridWidget to the common module.
> The class is basically the widget used in ktp-send-file for displaying the contacts (grid view + filter bar)
> 
> I had to add a new directory "Widgets" and a new library, because the class depends both on the "common" and the "models" modules.
> 
> 
> Diffs
> -----
> 
>   KTp/CMakeLists.txt f17ff8c3bb54408d3bfaf7773bae0e736836b9b2 
>   KTp/Widgets/CMakeLists.txt PRE-CREATION 
>   KTp/Widgets/contact-grid-widget.h PRE-CREATION 
>   KTp/Widgets/contact-grid-widget.cpp PRE-CREATION 
>   KTp/Widgets/flat-model-proxy.h PRE-CREATION 
>   KTp/Widgets/flat-model-proxy.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103702/diff/diff
> 
> 
> Testing
> -------
> 
> Succesfully used in the unreleased telepathy kipi plugin
> 
> 
> Thanks,
> 
> Daniele Elmo Domenichelli
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120115/faba0355/attachment-0001.html>