> 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.
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. - Ralf ----------------------------------------------------------- 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 [email protected] https://mail.kde.org/mailman/listinfo/amarok-devel
