Review Request: Use multiple inheritance to get an implementation for statistics handling

Sun Sep 9 10:22:41 UTC 2012

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

There are some welcome changes in this patch:
 * ability to implement statistics by inheritance
 * StatisticsCapability removal

However, there are some things I dislike:
 * Meta::Track inheriting AbstractStatistics
 * setAllStatistics() method (gosh!)
 * Overusing the StatisticsProvider idea. I think that statistics are implemention detail of each collection. I even dislike the statistics_permanent and statistics_tag tables and I'd like to see them ditched. If someone wants to have statistics for tracks on filesystem, put them in tags. Otherwise put the tracks into Local Collection. I'd like if the SqlCollection would be the only thing that would actually require the SQL database.

What I'd like to do instead:
 * StatisticsCapability is already ditched in my gsoc branch, with setPlaycount() and setFirst/LastPlayed() moved to EditCapability
 * move also setRating(), setScore() into EditCapability
 * refactor the whole capability concept (for tracks to start with) along with changing Meta::Track to be passed-by-value class with explicit data sharing. This would then mean that tracks could implement "Capabilities" (to be named better, perhaps Interfaces) by simply implementing the methods and then they could just "return this;" in relevant methods. Let me present you the suggestion in a couple of days.

- Matěj Laitl

On Aug. 30, 2012, 12:03 p.m., Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106276/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2012, 12:03 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> This is try number three to get a consistent implementation of statistics handling.
> This time with multiple inheritance.
>     
> This change will provide a complete implementation of statistics for a couple of collections.
> That means that collections will get missing functional implementations e.g. setPlaycount.
>     
> For the future we will need a single sql table for all statistics and coverage for the ugly cases which are:
> - PodcastMeta which currently is not covered.
> - SqlMeta which uses it's own table and has problems with restoring statistics.
> - A couple of collections that create tracks where the url is changed later on.
> 
> Oh, and it's 500 lines less code.
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/audiocd/AudioCdMeta.h 73ac547 
>   src/core-impl/collections/audiocd/AudioCdMeta.cpp d767125 
>   src/core-impl/collections/daap/DaapMeta.h 5278b57 
>   src/core-impl/collections/daap/DaapMeta.cpp a2429d7 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp e344e0f 
>   src/core-impl/collections/db/sql/SqlMeta.h 4450dab 
>   src/core-impl/collections/db/sql/SqlMeta.cpp c05e563 
>   src/core-impl/collections/db/sql/SqlRegistry.h f64b30c 
>   src/core-impl/collections/db/sql/SqlRegistry.cpp 1444f94 
>   src/core-impl/collections/db/sql/SqlRegistry_p.h a7d4e26 
>   src/core-impl/collections/db/sql/SqlRegistry_p.cpp 68cc871 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h 2d0706e 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 3946b8f 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.h 93d1c2e 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.cpp c9b07f3 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.h e5d7266 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp 091f5f6 
>   src/core-impl/collections/support/MemoryMeta.h 675cf42 
>   src/core-impl/collections/upnpcollection/UpnpMeta.h feabc1e 
>   src/core-impl/collections/upnpcollection/UpnpMeta.cpp 5ffee54 
>   src/core-impl/meta/file/File.h df96a2a 
>   src/core-impl/meta/file/File.cpp 46f672b 
>   src/core-impl/meta/file/File_p.h f756074 
>   src/core-impl/meta/multi/MultiTrack.h 63a8651 
>   src/core-impl/meta/proxy/MetaProxy.h 0579d08 
>   src/core-impl/meta/proxy/MetaProxy.cpp 51299f8 
>   src/core-impl/meta/stream/Stream.h 8b64d89 
>   src/core-impl/meta/stream/Stream.cpp 3eb54c7 
>   src/core-impl/meta/stream/Stream_p.h 390109a 
>   src/core-impl/meta/timecode/TimecodeMeta.h ceb4d66 
>   src/core-impl/meta/timecode/TimecodeMeta.cpp f85476d 
>   src/core-impl/statistics/providers/tag/TagStatisticsProvider.h df79a00 
>   src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.h 7b6b3bb 
>   src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.cpp 9855954 
>   src/core/CMakeLists.txt 29a215e 
>   src/core/capabilities/Capability.h 8ec6d16 
>   src/core/capabilities/StatisticsCapability.h 690ef12 
>   src/core/capabilities/StatisticsCapability.cpp 34af392 
>   src/core/meta/Meta.h bf2184b 
>   src/core/meta/Meta.cpp df45908 
>   src/core/podcasts/PodcastMeta.h 0cfaea1 
>   src/core/statistics/StatisticsProvider.h 6f54912 
>   src/core/statistics/StatisticsProvider.cpp 0b5a788 
>   src/databaseimporter/amarok14/FastForwardWorker.cpp 56e319b 
>   src/databaseimporter/itunes/ITunesImporterWorker.cpp e5ad5e6 
>   src/services/ServiceMetaBase.h 793eb9e 
>   src/services/ServiceMetaBase.cpp 039146b 
>   src/services/magnatune/MagnatuneMeta.cpp 523617b 
>   tests/core-impl/collections/db/sql/TestSqlScanManager.cpp c449216 
>   tests/core-impl/meta/file/TestMetaFileTrack.cpp 7549a58 
>   tests/core/meta/TestMetaTrack.cpp 0adb763 
>   tests/mocks/MetaMock.h b478c87 
>   tests/mocks/MockTrack.h 57bc344 
> 
> Diff: http://git.reviewboard.kde.org/r/106276/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ralf Engels
> 
>

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