Review Request: complete refactor of telepathy-nepomuk-service

Fri Jan 28 16:01:44 CET 2011

> On Jan. 24, 2011, 6 p.m., Dario Freddi wrote:
> > nepomuk-storage.h, lines 40-69
> > <http://git.reviewboard.kde.org/r/100439/diff/1/?file=7460#file7460line40>
> >
> >     These two classes seem like good candidates for being implemented with Implicit data sharing to me - what do you reckon?

Yeah, true, although perhaps a bit overkill. On the other hand, it's not exactly going to hurt performance, is it? so I might as well.

> On Jan. 24, 2011, 6 p.m., Dario Freddi wrote:
> > nepomuk-storage.cpp, line 408
> > <http://git.reviewboard.kde.org/r/100439/diff/1/?file=7461#file7461line408>
> >
> >     I'd remove those asserts (as in this one and all similar occurrences) in the final version - after all, you are checking for  existance in the next line.

Ah, you misunderstand the purpose of them. The logic goes as follows: ASSERTS are useful because they ensure that developers notice when things that should not be happening - it crashes. However, on a user's system the asserts won't be triggered, meaning the bug condition, if it occurs, will go right past. However, a debug warning is useful because it will still show up making debugging potentially a bit easier. More importantly, some of those error cases should actually have more than just a kWarning - in some places notifying the user might be sensible. However, I don't currently know when or how to do that, so have left it for now.

The fundamental purpose of the asserts is to make certain that developers notice the issues when testing (which with just a kwarning they might not).

> On Jan. 24, 2011, 6 p.m., Dario Freddi wrote:
> > nepomuk-storage.cpp, lines 610-617
> > <http://git.reviewboard.kde.org/r/100439/diff/1/?file=7461#file7461line610>
> >
> >     I am a bit concerned about this as well. First of all, I wonder if qHash() is needed at all in this case.
> >     Moreover, if it is true that some accounts have no ID, if we had 2 accounts without ID having the same contact ID we would end up with a duplicated key.
> >     I have no idea on how to get this one right though.

yeah... I'll just leave this version for now, but before finalising these changes we need to figure something out.

> On Jan. 24, 2011, 6 p.m., Dario Freddi wrote:
> > service.cpp, line 51
> > <http://git.reviewboard.kde.org/r/100439/diff/1/?file=7465#file7465line51>
> >
> >     Triggering shutdown in the destructor is a good idea only if the shutdown is a sync process. Otherwise, given that we're the first in the chain and all of the created objects are our children, we might end up having some weird crashes on exit caused by having a function called on a dead object.
> >     
> >     Especially, I see that most of the classes are emitting signal on shutdown, so I think it's quite realistic to expect some crashes on exit.
> >

I was fairly sure I'd dealt with all the shutdown/destruction issues before posting this patch, but I'll check again.

- George

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

On Jan. 24, 2011, 4:31 p.m., George Goldberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100439/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2011, 4:31 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is a initial review-request for my major refactoring of telepathy-nepomuk-service:
> 
> The motivation is that it will make the code cleaner, more maintainable and also make it possible to easily add support for syncing channel participants to Nepomuk who are not in the buddy-list but are present in channels.
> 
> *THIS IS NOT FINISHED* - the unit tests are incomplete, and avatar, group and capabilities support is not fully ported from the old version of t-n-s. I'm putting this up here for preliminary review comments however, because it's blocking a lot of development at the moment, so I'd like to get it moving again.
> 
> This patch also ports t-n-s to tpqt4 0.5+ and to the latest version of shared-desktop-ontologies where most of our ontology changes have been merged upstream. It also requires a as-yet-not-merged version of telepathy-testlib in order to compile the unit tests successfully.
> 
> Initial feedback welcome.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 5f4441d36279ca8921b9b7a28661e2a279812250 
>   abstract-storage.h PRE-CREATION 
>   abstract-storage.cpp PRE-CREATION 
>   account.h PRE-CREATION 
>   account.cpp PRE-CREATION 
>   cmake/modules/FindTelepathyQt4.cmake 4bed08aebfb1005c75cf45ab95422c408e909137 
>   contact.h PRE-CREATION 
>   contact.cpp PRE-CREATION 
>   controller.h PRE-CREATION 
>   controller.cpp PRE-CREATION 
>   nepomuk-storage.h PRE-CREATION 
>   nepomuk-storage.cpp PRE-CREATION 
>   nepomuktelepathyservice.h ccc433d8ca29d4670602b4a154e3bdf7e2a708c0 
>   nepomuktelepathyservice.cpp 7f1badbbc007201da68936e5e8f05bd315c16e43 
>   service.h PRE-CREATION 
>   service.cpp PRE-CREATION 
>   telepathy.trig fb1cead4e7852f86f445fbe0294157c737c7fed5 
>   telepathyaccount.h d0283eabb6ef4b708e83039f24c5accc52927037 
>   telepathyaccount.cpp e683bbe32374a59b6003d1b324f6eb0f4c2f5cd0 
>   telepathyaccountmonitor.h 4720fca7933004f59acc0137b11dd04dfbf8e98a 
>   telepathyaccountmonitor.cpp 5e1a0413c0f5477b7deef62180200045732f179e 
>   telepathycontact.h d49651257ef145faedb0d4229e35af0f2113bd8b 
>   telepathycontact.cpp dcba982be550b05cf9a5d85abf1ab8ddfcaa52e0 
>   test-backdoors.h PRE-CREATION 
>   test-backdoors.cpp PRE-CREATION 
>   tests/CMakeLists.txt 1a643487e5d85e808f7b96a199891a18fab4f360 
>   tests/account-test.h d9a1349d2f97bb84ef9df5a672eeb933ddfb2a6d 
>   tests/account-test.cpp 712932bed689bceedbe3f5fa60fc757759fdf895 
>   tests/controller-test.h PRE-CREATION 
>   tests/controller-test.cpp PRE-CREATION 
>   tests/storage-test.h PRE-CREATION 
>   tests/storage-test.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100439/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> George
> 
>

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