> 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. >
>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. - Matěj ----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
