> 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. > > Bart Cerneels wrote: > >> 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. > >
Ralf makes a very good argument though. AudioCD, DAAP and UPnP don't support native statistics (UPnP in theory does, but I've never seen a working implementation). File can use ID3 tags, we already have the support code. Not sure what to do with non-writable. ServiceMeta will be gone soon. We will make the ServiceCollections (Ampache & MP3Tunes) real collections, the stores will use a simple TrackProvider and the rest (last.fm, opmldirectory, gpodder) is not using it currently. Dedicated podcast players on devices always have statistics (% played or in play sections even). UMS and similar bare file implementations can use ID3 tags (from MetaFile::Track). So it still comes down to: do our users care that some collections don't support statistics? I think there are decent enough implementations for both yes and no answers, with yes obviously being more complex. We just have to select which one. - 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
