Review Request 113272: GSoC 2013 - Advanced Importers - 1/4: Changes in StatSyncing framework

[Konrad Zemek]

> On Oct. 18, 2013, 10 a.m., Matěj Laitl wrote:
> > src/statsyncing/Config.h, line 39
> > <http://git.reviewboard.kde.org/r/113272/diff/1/?file=201959#file201959line39>
> >
> >     I guess the ability have an extra comma is a g++ extension to C++, right? Only thing I like about it is clearer diff when adding new values - but that's just cosmetic.
> 
> Edward Hades Toroshchin wrote:
>     C++11 and C99 actually allow the trailing comma.

I removed that only after a compiler warning (can't remember which compiler now, and it might have been after supplying -pedantic flag).


> On Oct. 18, 2013, 10 a.m., Matěj Laitl wrote:
> > src/statsyncing/Controller.h, line 40
> > <http://git.reviewboard.kde.org/r/113272/diff/1/?file=201961#file201961line40>
> >
> >     I guess the purpose of QPointer is to be able to detect the deletion of ProviderFactory, right?
> >     
> >      1. I think we tend to prefer QWeakPointer for this use as it is lighter, faster and provides a clearer transition to Qt5.
> >      2. There should be no need to typedef it globally as there is no advantage of passing guarded pointers (QPointer, QWeakPointer) over passing ordinary pointers. Guarded pointers are only useful if you store them as object attributes and in this case I'd prefer to mark these uses explicitly.
> >     
> >     In other words, I think we tend to "globally typedef" only KSharedPtrs, Q(Explicitly)Shared(Data)Pointers and I think this makes sense.
> >     
> >     Or, in other other words :) I think we tend to typedef ... SomethingPtr only for "strong" pointers that guarantee that SomethingPtr cannot go 0 behind your back. It is not the case with QPointer and that's why I think that typedeffing it to ProviderFactoryPtr might be confusing for other devs.

I don't really remember my reasoning here, it might be simply my dislike for raw pointers. I don't check their validity anyway.

WRT transition, I think it's actually the other way around as in Qt5 QWeakPointer looses its ability to track QObjects. QPointer is just as fast as QWeakPointer there, but it's just me looking too far into the future. :)


> On Oct. 18, 2013, 10 a.m., Matěj Laitl wrote:
> > src/statsyncing/Config.h, lines 123-124
> > <http://git.reviewboard.kde.org/r/113272/diff/1/?file=201959#file201959line123>
> >
> >     This is redundant with QAbstractItemModel::rowsAboutToBeRemoved(), but much more convenient. If there are many users of this signal, it is worth it, if there is only one or two, I suggest they are changed to use QAbstractItemModel::rowsAboutToBeRemoved() and then index.index( row, col ).data( ProviderIdRole ) to do the same.

It's used in one place only (ImporterManager class). I changed it like you suggested, but I'm not happy with the effect. When using QAbstractItemModel API to retrieve information, ImporterManager becomes tightly coupled with Config. I feel the resulting code is very awkward and 'smelly'. However, I will change it if you still think it's a good idea.


> On Oct. 18, 2013, 10 a.m., Matěj Laitl wrote:
> > src/statsyncing/Controller.h, lines 68-73
> > <http://git.reviewboard.kde.org/r/113272/diff/1/?file=201961#file201961line68>
> >
> >     Hmm, do you subclass Controller? Or why the need to make these virtual?

I mock Controller in tests.


> On Oct. 18, 2013, 10 a.m., Matěj Laitl wrote:
> > src/statsyncing/Provider.cpp, line 21
> > <http://git.reviewboard.kde.org/r/113272/diff/1/?file=201964#file201964line21>
> >
> >     What is this include needed for?

Nothing, my bad.


- Konrad


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


On Oct. 21, 2013, 4:34 p.m., Konrad Zemek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113272/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2013, 4:34 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> -------
> 
> These are changes in the StatSyncing framework that I made as a part of my project.
> 
> 
> Diffs
> -----
> 
>   src/statsyncing/Config.cpp dd82dbe 
>   src/statsyncing/Controller.h 10c72a8 
>   src/statsyncing/Controller.cpp bf4c5d8 
>   src/statsyncing/Provider.h d9663f9 
>   src/statsyncing/Provider.cpp 775fce3 
>   src/statsyncing/ProviderFactory.h PRE-CREATION 
>   src/statsyncing/ProviderFactory.cpp PRE-CREATION 
>   src/statsyncing/Track.h 2f91704 
>   src/statsyncing/Track.cpp 9655cc1 
> 
> Diff: http://git.reviewboard.kde.org/r/113272/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Konrad Zemek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20131021/ab5bd983/attachment-0001.html>