<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/106494/">http://git.reviewboard.kde.org/r/106494/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">As already noted: Similar to a version that I already proposed.
Improvement for later:
- Think about statistics: Do two tracks that represent the same track (e.g. same tags) have the same statistics object? Do we have a 1:1 connection between statistics and tracks?
- Would it make sense to further separate track and statistics? Should we request listeners to subscribe for statistics changes in addition to track changes?
- in-batch update is not thread save. What happens if we have two batch updates for the same statistics/track. We need to change that to ref/unref mechanism in the future.
But still: code reuse ++
Ship it.
</pre>
<br />
<p>- Ralf</p>
<br />
<p>On September 18th, 2012, 3:52 p.m., Matěj Laitl 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, Bart Cerneels and Ralf Engels.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated Sept. 18, 2012, 3:52 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;">{Tag,Url}StatisticsStore: fix dates not read/saved due to wrong format used in sql
...we should really use some kind of db abstraction.
No need to mention PersistentStatisticsStore explicitly now
Implement Meta::Track::finishedPlaying(), ditch redundant implementations
code sharing++! Also harmonize the logic, it is now:
Raise play count if one of these hold:
* song is shorter than 30s and it has been played fully
* song is longer than 30s and at least 50% was played
* at least 5 minutes of the song was played
Meta::SqlTrack: use {begin,end}Update() for both Statistics and EditCapability
and make it thread-safe by changing m_batchUpdate from bool to int.
Also rename some methods to match other implementations and real
purpose. Add some docs.
Add set{First,Last}Played(), setPlayCount() to Meta::Statistics, ditch StatisticsCapability
..and adapt IpodMeta::Track to the new scheme.
Adapt StatisticsProvider & co. to Meta::Statistics, rename
...and use directly in Track implementations, which saves some code.
Also move all to core-impl/suppor - it is just an implementation
detail.
Also modify PersistentStatisticsStore (legacy StatisticsProvider) to
call notifyObservers() as appropriate, which needs some friend
declarations in Meta.h. Use new Observer functionality to track
linked tracks without holding a reference to them. (to avoid circular
reference counting)
Meta::Statistics: note about thread-safety
Meta::Track: move statistics methods into statistics() interface
* introduce Meta::Statistics, an interface to statistics-related
methods. Memory manager using KSharedPtr.
* remove (set)score(), (set)rating(), first/lastPlayed(), playCount()
from Meta::Track and move it to Meta::Statistics
* Because Meta::Statistics uses KSharedPtrs, existing track
implementations can just inherit Statistics and return this; in
Track::Statistics()
* Existing Track subclasses with dummy implementations of (set)score(),
(set)rating(), first/lastPlayed(), playCount() have it removed.
* Existing Track subclasses with meaningful implementations of
(set)score(), (set)rating(), first/lastPlayed(), playCount() turned
into Meta::Statistics and statistics() method is added for them.
Next steps:
* Make StatisticsProvider an implementation of Meta::Statistics, which
will in turn immensely simplify all tracks that use them.
* Add setPlaycount(), setFirst/LastPlayed() to Meta::Statistics and
ditch StatisticsCapability.
* Implement trackPlayed() in Meta::Track, ditch custom implementations.
Heavily based on Ralf's idea and patch in
https://git.reviewboard.kde.org/r/106276/ it is just attribute vs. the
subclass pattern + some minor details like notifyObservers() in
StatisticsStore, circular refcounting avoidance etc.
I want to deal with other track Capabilities in similar way in future,
the next one being EditCapability: I'd like to introduce Meta::Editable
and Meta::Track::editable() and rename {begin,end}MetaDataUpdate() to
just {begin,end}Update() to share even greater portion of code in Track
implementations.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Everything works as before in my testing and something better: first/last date is correctly read/saved to file tracks, stats are newly preserved for streams.
Individual commits should be available at http://quickgit.kde.org/index.php?p=clones%2Famarok%2Flaitl%2Famarok.git&a=shortlog&h=refs/heads/statistics-refactor in a few moments</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>ChangeLog <span style="color: grey">(8460b80e558c24f71fb9adfbd08a4427e0606e09)</span></li>
<li>playground/src/context/applets/coverbling/CoverBlingApplet.cpp <span style="color: grey">(5ef376a93a23b3a0c6f3323b31d8ddc82947b670)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(8596144087709d9baab19b63c35f0c8169dca427)</span></li>
<li>src/MainWindow.cpp <span style="color: grey">(0530fd725f41d00ce5f5b3aed68ab6e06a5c86b3)</span></li>
<li>src/TrayIcon.cpp <span style="color: grey">(51b2afd528234a3e2f01f2f02f6c496c08249d9b)</span></li>
<li>src/context/applets/currenttrack/CurrentTrack.cpp <span style="color: grey">(aa83a66f39a970b6927d0b63928c79ae55eeec9b)</span></li>
<li>src/context/widgets/RecentlyPlayedListWidget.cpp <span style="color: grey">(3b7bd7207b6a69be468bc241083abe67fb7f3b2d)</span></li>
<li>src/core-impl/collections/audiocd/AudioCdMeta.h <span style="color: grey">(d2ef5dbfab54a965bd66731fc921cc787d40264a)</span></li>
<li>src/core-impl/collections/audiocd/AudioCdMeta.cpp <span style="color: grey">(b24cd1bda95f14d9c5be06c64985f303784be72f)</span></li>
<li>src/core-impl/collections/daap/DaapMeta.h <span style="color: grey">(e0cf9f3ae1952fb0b0ec8632b5645b340c3ac7bd)</span></li>
<li>src/core-impl/collections/daap/DaapMeta.cpp <span style="color: grey">(2b01ebf37ebccb6ae2fcec630638fed262b0eebf)</span></li>
<li>src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp <span style="color: grey">(e344e0f8a8f26450b4340a64188cab72fa1a755c)</span></li>
<li>src/core-impl/collections/db/sql/SqlCollectionLocation.cpp <span style="color: grey">(c55d3986e43bf52f88016e4f353b71c84478c139)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.h <span style="color: grey">(4450dabcd6947341de035c53b2e6998280355ef9)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.cpp <span style="color: grey">(c05e563120ccda2d957ba5c1de507e73e50cbfc4)</span></li>
<li>src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp <span style="color: grey">(8d4b5852ee867e9698fb92e086c43d1cab4e429f)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodMeta.h <span style="color: grey">(5d54d8ce8586c7b7c715abb7bfb748403355fb6d)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodMeta.cpp <span style="color: grey">(13b9d913f744ebef9ac572c782193f0f9a532394)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodMetaEditCapability.h <span style="color: grey">(a19691e3dfc7fbb20befefc19dbd9c2a00c8af47)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodMetaEditCapability.cpp <span style="color: grey">(bc62412923ebf6c90de7b0c180a18c238d2fd0b8)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h <span style="color: grey">(2d0706e03a80d0f182c4dc9613751ef731385bcc)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp <span style="color: grey">(3946b8ffd4e75ba5e5bc259a60a683883abefa31)</span></li>
<li>src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp <span style="color: grey">(98e751f363f983b2857d9702414d150412c5dce7)</span></li>
<li>src/core-impl/collections/playdarcollection/PlaydarMeta.h <span style="color: grey">(93d1c2e103a737682eb05f70fc1db43245d6e313)</span></li>
<li>src/core-impl/collections/playdarcollection/PlaydarMeta.cpp <span style="color: grey">(c9b07f37abc3a0c40dea0a0642e9120249b4efb6)</span></li>
<li>src/core-impl/collections/playdarcollection/support/Query.cpp <span style="color: grey">(98a1eb51479236d85e384dcc989be2660f20d6d1)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionMeta.h <span style="color: grey">(e5d7266840ca244c884e1c397258e9d4d1a961de)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp <span style="color: grey">(091f5f6b484c8a8f66111d8a251c76f08ed2e632)</span></li>
<li>src/core-impl/collections/support/MemoryCustomValue.cpp <span style="color: grey">(f21ac9f8cfde8846a43007f2f355596c1a93fd5a)</span></li>
<li>src/core-impl/collections/support/MemoryMeta.h <span style="color: grey">(675cf425d43dc9d812af0bfabd2c01b2053d2852)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpMeta.h <span style="color: grey">(f4a62db7d71a38054ba61bb0c690e1cb2a12661a)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpMeta.cpp <span style="color: grey">(708bfb8c5682bd4fd003eab70869df1daecddccb)</span></li>
<li>src/core-impl/meta/file/File.h <span style="color: grey">(df96a2a1d74a11f6b559a1d89d27d2535df1107c)</span></li>
<li>src/core-impl/meta/file/File.cpp <span style="color: grey">(3ee6080df7eeb3810438f7405913f42336407521)</span></li>
<li>src/core-impl/meta/file/File_p.h <span style="color: grey">(57131f452a92cc2940d93c8535975a5131d16f7e)</span></li>
<li>src/core-impl/meta/multi/MultiTrack.h <span style="color: grey">(63a8651628eb6335cd0a3e635150bfae95e83235)</span></li>
<li>src/core-impl/meta/proxy/MetaProxy.h <span style="color: grey">(d6dc8f21a5978a362c8005fa4816183bb6ae1090)</span></li>
<li>src/core-impl/meta/proxy/MetaProxy.cpp <span style="color: grey">(0ba84e6d1ef2aad2d1624e1c4898768f8c2f3b22)</span></li>
<li>src/core-impl/meta/stream/Stream.h <span style="color: grey">(8b64d89f79752e0b8d67c78a2438c42e0edc2611)</span></li>
<li>src/core-impl/meta/stream/Stream.cpp <span style="color: grey">(3eb54c74fd77240a3da3cc92b4072c9dae7468f3)</span></li>
<li>src/core-impl/meta/stream/Stream_p.h <span style="color: grey">(390109ab276608768e1a78cb2e4ef1063527b49e)</span></li>
<li>src/core-impl/meta/timecode/TimecodeMeta.h <span style="color: grey">(ceb4d66f43fb21648aa7c0f12c9f33ea4d4e4ad6)</span></li>
<li>src/core-impl/meta/timecode/TimecodeMeta.cpp <span style="color: grey">(f85476d0af2a80b97de795bf557a3eca369c1d6a)</span></li>
<li>src/core-impl/statistics/providers/tag/TagStatisticsProvider.h <span style="color: grey">(df79a00726aee5d05b45847a1db515c291b8ee0f)</span></li>
<li>src/core-impl/statistics/providers/tag/TagStatisticsProvider.cpp <span style="color: grey">(7754b8ae4069775eaa417b87a4bb4e22b1535c1f)</span></li>
<li>src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.h <span style="color: grey">(7b6b3bb891b0b968f8402ffd05c9af1dfe7f8998)</span></li>
<li>src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.cpp <span style="color: grey">(9855954813b95c6036ab7eda9f1cd7c794dfbad4)</span></li>
<li>src/core-impl/support/PersistentStatisticsStore.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/support/PersistentStatisticsStore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/support/TagStatisticsStore.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/support/TagStatisticsStore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/support/UrlStatisticsStore.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/support/UrlStatisticsStore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/CMakeLists.txt <span style="color: grey">(667f2bc2d6e45838efce91c7f47c33a110d77330)</span></li>
<li>src/core/capabilities/Capability.h <span style="color: grey">(3d3354711c4a3d289e704c0ca957d11e25e62092)</span></li>
<li>src/core/capabilities/StatisticsCapability.h <span style="color: grey">(690ef1242a5dc6b0e218b5df3f7464c2182670f7)</span></li>
<li>src/core/capabilities/StatisticsCapability.cpp <span style="color: grey">(34af392070569040814153795d3d5310ce6d2824)</span></li>
<li>src/core/meta/Meta.h <span style="color: grey">(b9045f567fdb386fbc78b5ca35545e2f91cc3f8f)</span></li>
<li>src/core/meta/Meta.cpp <span style="color: grey">(cb0e5c9f337fc781daa3b0038f7e941932a40f3f)</span></li>
<li>src/core/meta/Statistics.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/meta/Statistics.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/meta/support/MetaConstants.cpp <span style="color: grey">(079fd22016b2c340cad6a42578826add1226df6a)</span></li>
<li>src/core/meta/support/MetaUtility.cpp <span style="color: grey">(133076b6163bee5f5d2c92f3c86b6514d1719665)</span></li>
<li>src/core/podcasts/PodcastMeta.h <span style="color: grey">(0cfaea1e708828a35c3a68f14ff0375e5a7f7509)</span></li>
<li>src/core/statistics/StatisticsProvider.h <span style="color: grey">(6f5491209df3dd64bcb0398f7054df2b47d7036f)</span></li>
<li>src/core/statistics/StatisticsProvider.cpp <span style="color: grey">(0b5a788626fb4620740893dda68022c32abb8cd7)</span></li>
<li>src/core/support/Amarok.h <span style="color: grey">(d8c91e789d02b204202c8a99b9459b8d10b3d90d)</span></li>
<li>src/databaseimporter/amarok14/FastForwardWorker.cpp <span style="color: grey">(56e319b24b2544489d9e9491f3e3c037ec7b63b2)</span></li>
<li>src/databaseimporter/itunes/ITunesImporterWorker.cpp <span style="color: grey">(e5ad5e6194547b91b57920a8692e5bbf9a14fea7)</span></li>
<li>src/dialogs/TagDialog.cpp <span style="color: grey">(a448458984c3920e510be9d4693c2e469765079a)</span></li>
<li>src/dialogs/TrackOrganizer.cpp <span style="color: grey">(331f1ef088fd4e6c69eade85ae9ed485faed90b4)</span></li>
<li>src/playlist/PlaylistModel.cpp <span style="color: grey">(700c9096a1810c89106638a27aee2260439def55)</span></li>
<li>src/playlist/navigators/FavoredRandomTrackNavigator.cpp <span style="color: grey">(3d57a8261695a46e4d7545068e9a0b9a89382aec)</span></li>
<li>src/playlist/proxymodels/GroupingProxy.cpp <span style="color: grey">(1e32d9ca44f6d5e8bc955f8b54718fe766027ff8)</span></li>
<li>src/playlist/proxymodels/ProxyBase.cpp <span style="color: grey">(06306adee1f41100a8bff489845e4b3eea9b271d)</span></li>
<li>src/playlist/proxymodels/SortAlgorithms.cpp <span style="color: grey">(d6a395dab2845e41c461037c8d00ea22446e7934)</span></li>
<li>src/playlist/view/listview/PrettyItemDelegate.cpp <span style="color: grey">(d3c2c55de12c0078cf9bed0b2974c91c96e93149)</span></li>
<li>src/playlistgenerator/constraints/TagMatch.cpp <span style="color: grey">(01c09e55d83c6512a9da6626a6ca218bb3753941)</span></li>
<li>src/scriptengine/MetaTypeExporter.cpp <span style="color: grey">(b944c8e72471a6d66956df938b2c94e248cb9f37)</span></li>
<li>src/services/ServiceMetaBase.h <span style="color: grey">(793eb9ebbd35495984734ed04bd159c2ff1b7bb5)</span></li>
<li>src/services/ServiceMetaBase.cpp <span style="color: grey">(039146b29d648557c6b1960e84b23872b4da823e)</span></li>
<li>src/services/lastfm/meta/LastFmMeta.h <span style="color: grey">(6d93cad50fba629a4a278e3380b56859dde5616a)</span></li>
<li>src/services/lastfm/meta/LastFmMeta.cpp <span style="color: grey">(0e7659bd570d0dff18e6555b772228937bb9269b)</span></li>
<li>src/services/lastfm/meta/LastFmMeta_p.h <span style="color: grey">(33f3b73b18fef3227811011fa3900ed5fbe50fcf)</span></li>
<li>src/services/magnatune/MagnatuneMeta.cpp <span style="color: grey">(523617be763834927d685fb070778e39c20b114f)</span></li>
<li>src/widgets/Osd.cpp <span style="color: grey">(423be4a009c82f71f23a364c16a170a24db2f8ac)</span></li>
<li>tests/core-impl/collections/db/sql/TestSqlScanManager.cpp <span style="color: grey">(2dbed81808989b96361b99cf0650699b53fc28fa)</span></li>
<li>tests/core-impl/collections/db/sql/TestSqlTrack.cpp <span style="color: grey">(cb789f71350b8acf7aa6272846d36e7b6c505308)</span></li>
<li>tests/core-impl/meta/file/TestMetaFileTrack.h <span style="color: grey">(a7759e0703301870d22791eb9a4c5b60368fc637)</span></li>
<li>tests/core-impl/meta/file/TestMetaFileTrack.cpp <span style="color: grey">(7549a585e25e2e552d633df2fd9174aeaf7813b1)</span></li>
<li>tests/core/meta/TestMetaTrack.cpp <span style="color: grey">(0adb7633538533202c50487b1eaff8e4ead150e9)</span></li>
<li>tests/mocks/MetaMock.h <span style="color: grey">(6a706a4472b127fa38f596cb2c780e3ef2278dba)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106494/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>