Review Request: Merge request for telepathy-kde-contactlist

Wed Mar 30 12:01:29 CEST 2011

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > Follows a review. Protip: smaller diffs next time, I surely missed something :D
> > 
> > Btw, I see this removes all the bit of Nepomuk integration me and George did time ago: I don't know if you guys already discussed about that, but I'd like at least to have George chiming in and have a look.
> > 
> > Beware of QVariant misuse: besides being wrong, it can be dangerous!
> 
> David Edmundson wrote:
>     Just to help clarify things:
>     
>     We have discussed the nepomuk side, George G was involved in the discussion. It's part of a large plan, hence the absolutely massive diff. As it's such a large set of changes I really want to get this merged as this diff keeps getting larger and larger.
>     
>     The QVariant mis-use and the coding style switch comes from a sort of upstream (Tp-Yell) for some files which we currently have a local copy of. We've already had to do several bug fixes to it, so I guess we can add this one too.
>     
>     
>

Ok, I'll check for the QVariant misuses and then I will add the relevant parts to our almost-upstream-repo too, so the upstream can pick it up.

Also I see you catched a lot of upstream issues, I commented just "Upstream" on them. I think this should be solved upstream first (I'll promote the changes there). I leave the inclusion of those files to the meeting.

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > account-button.cpp, lines 65-72
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12726#file12726line65>
> >
> >     Use KAction instead of QAction here

Ok.

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > account-button.cpp, line 99
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12726#file12726line99>
> >
> >     For clarity, try using QVariant::value() instead of qVariantFromValue, which was meant as a workaround for old compilers. Your code would become:
> >     
> >     a->data().value<Tp::Presence>().status()
> >     
> >     Check all the occurrences throughout the code

Wel...don't we want to support older compilers too? ;) It was mostly for some MS compilers iirc, but sure, I can change that.

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > account-button.cpp, lines 102-104
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12726#file12726line102>
> >
> >     It is not very clear to me what you are doing here, but this routine can be extremely slow. Can you please tell me what you are trying to achieve?
> >     
> >     I also see there is more than one occurrence of that, so this should indeed optimized if possible.

This paints a small overlay over the account icon. It paints the presence icon (online, offline etc) and I think this is a bit of the original code. What would be a better way to achieve painting a small emblem over an icon?

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > account-filter-model.h, line 4
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12727#file12727line4>
> >
> >     Watch out for correct copyrights, unless you copied this code from somewhere else.

No, this one is ours. I always keep the original copyright people in place, you can see that throughout the rest of the code ;)

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > accounts-model-item.cpp, lines 139-142
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12730#file12730line139>
> >
> >     Urgh - do not do that. Declare AccountsModelItem as a metatype const_cast to AccountsModelItem and do QVariant::fromValue() from there. Even though I'd try to avoid that const_cast if possible, but it seems rather unfeasible.

That's from upstream :) But I'll fix it.

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > accounts-model.h, lines 122-124
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12731#file12731line122>
> >
> >     You don't really need a p-impl class here as you are not exporting the model, but if you want to keep it that way, declare it as:
> >     
> >     class Private;
> >     Private * const d;
> >     
> >     To keep this consistent with KDE's style.

This is again upstream file. It has some stripped header directives so it can be used in our project without problems. I'd leave this for now, we'll talk more about this on the meeting.

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > accounts-model.cpp, line 77
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12732#file12732line77>
> >
> >     I don't really see the point of that, can you please explain?

Upstream :)

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > contact-delegate.cpp, line 1
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12736#file12736line1>
> >
> >     Copyright missing
> >     
> >     P.S.: Hey, this looks like some of my old code btw :D

Yeah, it is based on the original code ;)

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > contact-model-item.h, lines 39-40
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12737#file12737line39>
> >
> >     Apparently, those do not need to be virtual

Upstream..

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > contact-model-item.cpp, lines 88-90
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12738#file12738line88>
> >
> >     Same as before

Upstream..

> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > main-widget.cpp, line 143
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12746#file12746line143>
> >
> >     KMenu instead of QMenu

Ok

- Martin

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

On March 23, 2011, 9:32 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100838/
> -----------------------------------------------------------
> 
> (Updated March 23, 2011, 9:32 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is the work that begun in my clone repo and has matured enough now to be merged back to 'upstream', where the work should continue now.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 2c62ea10556cdba38d1bca0fe63603df97414336 
>   account-button.h PRE-CREATION 
>   account-button.cpp PRE-CREATION 
>   account-filter-model.h PRE-CREATION 
>   account-filter-model.cpp PRE-CREATION 
>   accounts-model-item.h PRE-CREATION 
>   accounts-model-item.cpp PRE-CREATION 
>   accounts-model.h PRE-CREATION 
>   accounts-model.cpp PRE-CREATION 
>   contact-delegate-overlay.h PRE-CREATION 
>   contact-delegate-overlay.cpp PRE-CREATION 
>   contact-delegate.h PRE-CREATION 
>   contact-delegate.cpp PRE-CREATION 
>   contact-model-item.h PRE-CREATION 
>   contact-model-item.cpp PRE-CREATION 
>   contact-overlays.h PRE-CREATION 
>   contact-overlays.cpp PRE-CREATION 
>   contact-view-hover-button.h PRE-CREATION 
>   contact-view-hover-button.cpp PRE-CREATION 
>   filter-bar.h PRE-CREATION 
>   filter-bar.cpp PRE-CREATION 
>   main-widget.h aed6f7c8336321bc8a6ffb3b9af6b1d493f85790 
>   main-widget.cpp 6146b62eec17b54be63b594200613931673ac5fe 
>   main-widget.ui 5b0d5aaaf3a4e2f49eb030b98b15fcae3a86170c 
>   main.cpp bba0e4175e8853afe603f26ea4707f4974192d0f 
>   ontologies/CMakeLists.txt 3e0bbe8c634908f689dcd360409aeddcc6fc0d23 
>   tree-node.h PRE-CREATION 
>   tree-node.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100838/diff
> 
> 
> Testing
> -------
> 
> We did a lot of thourough testing on #kde-telepathy, also some people emailed their reports which all has been fixed.
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110330/d00e0f1a/attachment-0001.htm 