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

Fri Oct 18 10:00:12 UTC 2013

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

This generally looks very good to me, there's just a couple of questions. I must note that I've read just this chapter 1/4 yet so I might not have the right context in all cases. Review of other parts is on my TODO list, but perhaps it is more useful to go though this central one first.

src/statsyncing/Config.h
<http://git.reviewboard.kde.org/r/113272/#comment30565>

    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.

src/statsyncing/Config.h
<http://git.reviewboard.kde.org/r/113272/#comment30567>

    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.

src/statsyncing/Config.cpp
<http://git.reviewboard.kde.org/r/113272/#comment30566>

    cosmetic, but his actually changes Amarok-style to mixed style (no space after "enabled")

src/statsyncing/Controller.h
<http://git.reviewboard.kde.org/r/113272/#comment30585>

    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.

src/statsyncing/Controller.h
<http://git.reviewboard.kde.org/r/113272/#comment30589>

    Hmm, do you subclass Controller? Or why the need to make these virtual?

src/statsyncing/Controller.h
<http://git.reviewboard.kde.org/r/113272/#comment30587>

    I find name of this method confusing. The word "registered" seems to refer to registerProvider(), but it is not the case. I suggest hasProviderFactories()

src/statsyncing/Controller.h
<http://git.reviewboard.kde.org/r/113272/#comment30588>

    I suggest "providerIsConfigurable" as the current may be misread as a property of controller itself named provider-configurable (whatever it would mean).

src/statsyncing/Controller.h
<http://git.reviewboard.kde.org/r/113272/#comment30568>

    These methods could use more documentation:
     * can they return 0? (if you pass invalid id or an if of a non-configurable provider, if there are no providers to create)
     * who is responsible for memory management of the returned pointer?

src/statsyncing/Controller.h
<http://git.reviewboard.kde.org/r/113272/#comment30591>

    Hmm, I'm confused. Is this a *provider* id or a *factory* id? If it is the latter, I suggest renaming it to "type".

    If it is the former, I suggest decoupling these.

src/statsyncing/Controller.cpp
<http://git.reviewboard.kde.org/r/113272/#comment30594>

    Kudos for deduplicating existing code!

src/statsyncing/Provider.h
<http://git.reviewboard.kde.org/r/113272/#comment30595>

    Oh, I'd be much happier if this class could go into its own file. (I ack that putting the factory into the same file is currently broadly used in Amarok code-base, but I'd like to change that)

src/statsyncing/Provider.h
<http://git.reviewboard.kde.org/r/113272/#comment30596>

    I suggest renaming this to type() or providerType() to avoid mismatch with provider->id().

src/statsyncing/Provider.h
<http://git.reviewboard.kde.org/r/113272/#comment30597>

    Duplicate typedef, the other one is in Controller.h. Not an error, but is it really needed?

    (+ the same notes about QPointer, typedeffing..)

src/statsyncing/Provider.cpp
<http://git.reviewboard.kde.org/r/113272/#comment30598>

    What is this include needed for?

src/statsyncing/Track.h
<http://git.reviewboard.kde.org/r/113272/#comment30599>

    cosmetic: excessive () in @param lastPlayed()

src/statsyncing/Track.h
<http://git.reviewboard.kde.org/r/113272/#comment30600>

    cosmetic: excessive ()

src/statsyncing/Track.cpp
<http://git.reviewboard.kde.org/r/113272/#comment30601>

    cosmetic: Amarok code style

- Matěj Laitl

On Oct. 16, 2013, 7:11 p.m., Konrad Zemek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113272/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2013, 7:11 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.h 140e647ecbcd6edd11cbe637a3e7b482bd24ec70 
>   src/statsyncing/Config.cpp dd82dbe7729bf1b8cc2f5153c8af39aade0a9b81 
>   src/statsyncing/Controller.h 10c72a809d1cb7065363a978df6aa2561621de2a 
>   src/statsyncing/Controller.cpp bf4c5d8324bce796cf9b0057d21721b20af700b5 
>   src/statsyncing/Provider.h d9663f93073de3035eb609967adea491f8192ab9 
>   src/statsyncing/Provider.cpp 775fce388248b8e7aca1af442714d26549d014ce 
>   src/statsyncing/Track.h 2f917045371d8caab3ec48d37050dfa5cec15f18 
>   src/statsyncing/Track.cpp 9655cc16078724f5aa4f1dde532c1b65f2e574c1 
>   src/statsyncing/TrackTuple.h 157d6044c1c7c79f471b9059f6a812887fa730ac 
>   src/statsyncing/TrackTuple.cpp 944279414ab82ff811b33e1de0cd0f73f623d23b 
>   src/statsyncing/jobs/SynchronizeTracksJob.cpp b789aa32314b345270c07b34823e82b2a74ed6dd 
> 
> 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/20131018/fbe7c703/attachment-0001.html>