Review Request: Use multiple inheritance to get an implementation for statistics handling
Bart Cerneels
bart.cerneels at kde.org
Mon Sep 10 06:55:21 UTC 2012
> On Sept. 9, 2012, 10:22 a.m., Matěj Laitl wrote:
> > 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.
>
> Ralf Engels wrote:
> The best implementation depends mainly on how you see the statistics in relation to the tracks.
>
> 1.
> If you see statistics as something that only some collections have, then Capabilities are fine for it.
> (However I still don't like the Capabilities because they are running against the inheritance concept and add complexity, since we suddenly have tracks with vastly different capabilities)
>
> 2.
> If you see statistics as something that every track has, but is implemented in each collection, then it would look differently.
> One implementation per collection. No shared statistics tables. But also no Statistsics providers, since we don't need them if we have different implentations for the collections.
>
> 3.
> If statistics are a thing that every collection should have and we want to provide a default implementation, then something like my proposal is what you need.
> - A default implementation that covers most cases.
> - Sharing of statistics over the collections.
> - More usage of the StatisticsTagProvider
>
> Now, which of the three cases do we have in Amarok?
> The "rating()" function is build into the Meta::Track, as well as "finishedPlaying()". So it seem to me that we don't want to have the first case.
> A lot of collections use the PermanentUrlStatisticsProvider. And the collections that don't use it should, since it adds functionality without additional cost (in code complexity).
> So we don't have the second case, since the implementations are not different but mostly the same.
> For me it seems that we have the third case. Statistics were cleanly ment to be build into the Tracks. You even get the "value changed" indication for the track if statistics are changed, so conceptually the statistics seem to be part of the track as e.g. the title.
>
> A clear indication that we have case three is the fact that cleaning up the code removed around 500 lines (while I added a lot of comments at the same time).
>
> Now, let's look into the stuff again:
> 1. I was not bold enough to flatly provide a default implementation for statistics in Meta::Track.
> That is even difficult since Meta::Track is in "core" while sql handling is in "core-impl". So it would make sense to provice an abstract default implementation. Also having an abstract statistics provider class makes it very clear what functions are needed to be implemented for a full statistics provider.
>
> 2. setAllStatistics is a performance improvement.
> The most common statistic settings operation is "finishedPlaying" which sets three values.
> So you have three sql commits or another way to group the operations together. You can do that with a "write later" or "block update" function. But have a look at the simplistic implementation of SqlMeta and you see that a "setAllStatistics" is easier.
>
> 3. "overuse of statistics provider".
> That is a clear "form follows function". If we have the function that I outlined above in (3) and no way to provide a default implementation in "core" then we need some type of mix-ins.
> I have tried to implement it in two different ways but the StatisticsProvider is the best step for now.
> Other refactoring tries had much bigger code changes with much less cleanup effect.
>
>
> As further steps I propose:
> 1. create a statistics table to be used by everybody.
> 2. try to move more collections to the tag statistics provider which would ensure that a track played from the file collection remains it's rating once it's imported into the sql collection (among other things)
> 3. Change the name of the StatisticsProvider. Google the provider pattern for an enlightning read. This is not a provider and we shouldn't call it such.
>
>
> Matěj Laitl wrote:
> >1.
> > If you see statistics as something that only some collections have, then Capabilities are fine for it.
> > (However I still don't like the Capabilities because they are running against the inheritance concept and add complexity, since we suddenly have tracks with vastly different capabilities)
> >
> > 2.
> > If you see statistics as something that every track has, but is implemented in each collection, then it would look differently.
> > One implementation per collection. No shared statistics tables. But also no Statistsics providers, since we don't need them if we have different implentations for the collections.
> >
> > 3.
> > If statistics are a thing that every collection should have and we want to provide a default implementation, then something like my proposal is what you need.
> > - A default implementation that covers most cases.
> > - Sharing of statistics over the collections.
> > - More usage of the StatisticsTagProvider
> >
> > Now, which of the three cases do we have in Amarok?
>
> IMO, we *want* to have the option 2.
>
> > The "rating()" function is build into the Meta::Track, as well as "finishedPlaying()". So it seem to me that we don't want to have the first case.
>
> Right.
>
> > A lot of collections use the PermanentUrlStatisticsProvider. And the collections that don't use it should, since it adds functionality without additional cost (in code complexity).
> > So we don't have the second case, since the implementations are not different but mostly the same.
>
> This is IMO our design failure. PermanentUrlStatisticsProvider has little sense IMO. If you want Amarok to track the stats of a track -> put it in the Local Collection. Else you have to put the statistics into the track. Period. PermanentUrlStatisticsProvider adds little functionality (urls can easily change, not accessible outside of Amarok) and duplicates our existing Local Collection efforts. Just ditch it.
>
> > For me it seems that we have the third case. Statistics were cleanly ment to be build into the Tracks. You even get the "value changed" indication for the track if statistics are
> > changed, so conceptually the statistics seem to be part of the track as e.g. the title.
>
> Right. You don't have a SQL table of titles of all tracks you hit by occasion, right? It is *built into the track* using *collection-specific method.* Local colleciton has in in SQL, iPod Collection has it in iTunes DB, UMS (and "file") collection can have it in ID3 tags.
>
> > A clear indication that we have case three is the fact that cleaning up the code removed around 500 lines (while I added a lot of comments at the same time).
>
> No, it is a result of removing StatisticsCapability. StatisticsCapability was a little hack to allow importing of stats, the methods should have been in a different place.
>
> > Now, let's look into the stuff again:
> > 1. I was not bold enough to flatly provide a default implementation for statistics in Meta::Track.
> > That is even difficult since Meta::Track is in "core" while sql handling is in "core-impl". So it would make sense to provice an abstract default implementation. Also having an abstract
> > statistics provider class makes it very clear what functions are needed to be implemented for a full statistics provider.
>
> It wouldn't be bold, it would be silly. We for sure don't want our core to depend on sql. Moreover, I don't want any code (apart from SqlCollection) to depend on sql.
>
> > 2. setAllStatistics is a performance improvement.
> > The most common statistic settings operation is "finishedPlaying" which sets three values.
> > So you have three sql commits or another way to group the operations together. You can do that with a "write later" or "block update" function. But have a look at the simplistic
> > implementation of SqlMeta and you see that a "setAllStatistics" is easier.
>
> My proposal (partially implemented in my gsoc branch, rest is coming) is to move all the setRating(), setScore() and add setFirst/LastPlayed(), setPlayCount() to EditCapability (or rather the interface that will replace it). Other option is to add extra WriteStatsInterface for the 5 methods alone and decide wheter the getters should be in Meta::Track or in StatsInterface.
>
> > 3. "overuse of statistics provider".
> > That is a clear "form follows function". If we have the function that I outlined above in (3) and no way to provide a default implementation in "core" then we need some type of mix-ins.
> > I have tried to implement it in two different ways but the StatisticsProvider is the best step for now.
> > Other refactoring tries had much bigger code changes with much less cleanup effect.
>
> This is clear consequence of you wanting the option 3. Please consider my arguments above that it would be a *really bad* idea.
>
> > As further steps I propose:
> > 1. create a statistics table to be used by everybody.
>
> I strongly oppose. Some tracks should be incapable of having statistics. Some tracks need to save it differently (iPod collection, UMS colleciton with iD3 tags, MTP collection, Nepomuk collection).
>
> > 2. try to move more collections to the tag statistics provider which would ensure that a track played from the file collection remains it's rating once it's imported into the sql collection (among other things)
>
> Already implemented (by me) in SqlCollectionLocation, test it!
>
> > 3. Change the name of the StatisticsProvider. Google the provider pattern for an enlightning read. This is not a provider and we shouldn't call it such.
>
> Alternative: ditch StatisticsProvider.
>
> Ralf Engels wrote:
> Let's first discuss about the functionality.
> (and not answer your specific points because that is useless unless we agree on the requirements)
>
> I would like all collections to have full statistics support.
> Where the underlying mechanisms don't support it I want to have a default statistics support storing the rating locally.
> The collections that currently use the statistics provider are:
>
> AudioCd
> DaapMeta
> UpnpMeta
> File
> TimecodeMeta (whatever that is)
> ServiceMeta (and by implication all collections that inherit but don't provide a different implementation)
> PodCastMeta has a problem, as it is in core and thus can't use any of the providers.
>
> Don't talk about implementation right now, just about the requirement.
>
> What is the requirement? Should all collections support ratings even if the underlying mechanism doesn't?
> I personally would like to have ratings for files on write protected network file systems, audio cds and podcasts.
>
> Matěj Laitl wrote:
> Hmm, thinking about it I must agree that having ratings for some of these would be nice. Please give me more time to think about it and to perhaps incorporate it to my Meta::Track & Capabilities refactor suggestion.
>> A lot of collections use the PermanentUrlStatisticsProvider. And the collections that don't use it should, since it adds functionality without additional cost (in code complexity).
>> So we don't have the second case, since the implementations are not different but mostly the same.
> This is IMO our design failure. PermanentUrlStatisticsProvider has little sense IMO. If you want Amarok to track the stats of a track -> put it in the Local Collection. Else you have to put the statistics into the track. Period. PermanentUrlStatisticsProvider adds little functionality (urls can easily change, not accessible outside of Amarok) and duplicates our existing Local Collection efforts. Just ditch it.
I agree with this. I question the use case of wanting to keep/know the rating of a non local-collection track, or tracks on mediaplayer that support it, etc. It's probably a case of developers scratching an imaginary itch and real users probably don't even see the problem being solved.
We can remove the complexity at the requirement level and improve the functionality we do really need.
- Bart
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106276/#review18709
-----------------------------------------------------------
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/20120910/249acaab/attachment-0001.html>
More information about the Amarok-devel
mailing list