Review Request: Merge request for telepathy-kde-contactlist

Dario Freddi drf at kde.org
Wed Mar 30 14:15:05 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.
>     
>     
>
> 
> Martin Klapetek wrote:
>     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.

Ok, then it's mostly upstream which should be bitch-slapped this time ;D Besides that, nice job on the patch :) Once you fix the remaining issues, it's a "Ship it" from me.


> 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.
> 
> Martin Klapetek wrote:
>     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?

There is a custom KIcon constructor (please see http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKIcon.html#a6057e88b03b32ca70af2da295d626408 ) which does the dirty job itself when it comes to overlays - it is surely not much more optimized than this, but when QIcon will support overlays natively, we'll get the speed bump for free. I advise you to switch to this method, it will also give more clarity to the code.


> 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
> 
> Martin Klapetek wrote:
>     Wel...don't we want to support older compilers too? ;) It was mostly for some MS compilers iirc, but sure, I can change that.

Yeah, it was meant to be used to support MSVC 6 :D However, if you feel like not changing it don't worry, it's just a matter of making the whole code more readable


> 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.
> 
> Martin Klapetek wrote:
>     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.

Sure. Anyway, if this code comes from other project, we should attach a "This file is from blabla project" notice in the copyrights - but no hurry and not a blocker indeed, we'll talk about it in the meeting.


- Dario


-----------------------------------------------------------
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/0ad5932d/attachment-0001.htm

More information about the KDE-Telepathy mailing list