<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106276/">http://git.reviewboard.kde.org/r/106276/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 9th, 2012, 10:22 a.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
<br />
<p>- Ralf</p>
<br />
<p>On August 30th, 2012, 12:03 p.m., Ralf Engels wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Ralf Engels.</div>
<p style="color: grey;"><i>Updated Aug. 30, 2012, 12:03 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core-impl/collections/audiocd/AudioCdMeta.h <span style="color: grey">(73ac547)</span></li>
<li>src/core-impl/collections/audiocd/AudioCdMeta.cpp <span style="color: grey">(d767125)</span></li>
<li>src/core-impl/collections/daap/DaapMeta.h <span style="color: grey">(5278b57)</span></li>
<li>src/core-impl/collections/daap/DaapMeta.cpp <span style="color: grey">(a2429d7)</span></li>
<li>src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp <span style="color: grey">(e344e0f)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.h <span style="color: grey">(4450dab)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.cpp <span style="color: grey">(c05e563)</span></li>
<li>src/core-impl/collections/db/sql/SqlRegistry.h <span style="color: grey">(f64b30c)</span></li>
<li>src/core-impl/collections/db/sql/SqlRegistry.cpp <span style="color: grey">(1444f94)</span></li>
<li>src/core-impl/collections/db/sql/SqlRegistry_p.h <span style="color: grey">(a7d4e26)</span></li>
<li>src/core-impl/collections/db/sql/SqlRegistry_p.cpp <span style="color: grey">(68cc871)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h <span style="color: grey">(2d0706e)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp <span style="color: grey">(3946b8f)</span></li>
<li>src/core-impl/collections/playdarcollection/PlaydarMeta.h <span style="color: grey">(93d1c2e)</span></li>
<li>src/core-impl/collections/playdarcollection/PlaydarMeta.cpp <span style="color: grey">(c9b07f3)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionMeta.h <span style="color: grey">(e5d7266)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp <span style="color: grey">(091f5f6)</span></li>
<li>src/core-impl/collections/support/MemoryMeta.h <span style="color: grey">(675cf42)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpMeta.h <span style="color: grey">(feabc1e)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpMeta.cpp <span style="color: grey">(5ffee54)</span></li>
<li>src/core-impl/meta/file/File.h <span style="color: grey">(df96a2a)</span></li>
<li>src/core-impl/meta/file/File.cpp <span style="color: grey">(46f672b)</span></li>
<li>src/core-impl/meta/file/File_p.h <span style="color: grey">(f756074)</span></li>
<li>src/core-impl/meta/multi/MultiTrack.h <span style="color: grey">(63a8651)</span></li>
<li>src/core-impl/meta/proxy/MetaProxy.h <span style="color: grey">(0579d08)</span></li>
<li>src/core-impl/meta/proxy/MetaProxy.cpp <span style="color: grey">(51299f8)</span></li>
<li>src/core-impl/meta/stream/Stream.h <span style="color: grey">(8b64d89)</span></li>
<li>src/core-impl/meta/stream/Stream.cpp <span style="color: grey">(3eb54c7)</span></li>
<li>src/core-impl/meta/stream/Stream_p.h <span style="color: grey">(390109a)</span></li>
<li>src/core-impl/meta/timecode/TimecodeMeta.h <span style="color: grey">(ceb4d66)</span></li>
<li>src/core-impl/meta/timecode/TimecodeMeta.cpp <span style="color: grey">(f85476d)</span></li>
<li>src/core-impl/statistics/providers/tag/TagStatisticsProvider.h <span style="color: grey">(df79a00)</span></li>
<li>src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.h <span style="color: grey">(7b6b3bb)</span></li>
<li>src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.cpp <span style="color: grey">(9855954)</span></li>
<li>src/core/CMakeLists.txt <span style="color: grey">(29a215e)</span></li>
<li>src/core/capabilities/Capability.h <span style="color: grey">(8ec6d16)</span></li>
<li>src/core/capabilities/StatisticsCapability.h <span style="color: grey">(690ef12)</span></li>
<li>src/core/capabilities/StatisticsCapability.cpp <span style="color: grey">(34af392)</span></li>
<li>src/core/meta/Meta.h <span style="color: grey">(bf2184b)</span></li>
<li>src/core/meta/Meta.cpp <span style="color: grey">(df45908)</span></li>
<li>src/core/podcasts/PodcastMeta.h <span style="color: grey">(0cfaea1)</span></li>
<li>src/core/statistics/StatisticsProvider.h <span style="color: grey">(6f54912)</span></li>
<li>src/core/statistics/StatisticsProvider.cpp <span style="color: grey">(0b5a788)</span></li>
<li>src/databaseimporter/amarok14/FastForwardWorker.cpp <span style="color: grey">(56e319b)</span></li>
<li>src/databaseimporter/itunes/ITunesImporterWorker.cpp <span style="color: grey">(e5ad5e6)</span></li>
<li>src/services/ServiceMetaBase.h <span style="color: grey">(793eb9e)</span></li>
<li>src/services/ServiceMetaBase.cpp <span style="color: grey">(039146b)</span></li>
<li>src/services/magnatune/MagnatuneMeta.cpp <span style="color: grey">(523617b)</span></li>
<li>tests/core-impl/collections/db/sql/TestSqlScanManager.cpp <span style="color: grey">(c449216)</span></li>
<li>tests/core-impl/meta/file/TestMetaFileTrack.cpp <span style="color: grey">(7549a58)</span></li>
<li>tests/core/meta/TestMetaTrack.cpp <span style="color: grey">(0adb763)</span></li>
<li>tests/mocks/MetaMock.h <span style="color: grey">(b478c87)</span></li>
<li>tests/mocks/MockTrack.h <span style="color: grey">(57bc344)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106276/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>