<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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review has been submitted with commit 0818d6ace981f302bf1f00a063728e4dbfcd0f0c by Matěj Laitl to branch master.</pre>
<br />
<p>- Commit</p>
<br />
<p>On September 22nd, 2012, 3:31 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, Ralf Engels, and Phalgun Guduthur.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated Sept. 22, 2012, 3:31 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;">MetaFile::Track: prevent crash with the new statistics design
Following happened:
1. MetaFile::Track constuctor calls readMetaData()
2. if readMetaData() detects statistics in file tags, it calls
PersistentStatisticsStore->set{Rating,...}()
3. set{Rating,...}() calls notifyObservers, which constructs
KSharedPtr to the track. When the KSharedPtr is destroyed
(probably right after the call), it destroys the Track being
constructed as its refcount drops to zero. Booo!
Prevent this by increasing the refcount temporarily. This is a special
case where PersistentStatisticsStore setter methods are called in
linked track's constructor, other code is safe.
Meta::NepomukTrack: organize header file, implement finishedPlaying()
...and don't actually lie about what methods are Meta::Track and what
are Nepomuk-specific.
Meta::Statistics and subclasses: add docs and asserts wrt thread safety
{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">(fa64799c18702ff777ec013e3b8726a9b8703b45)</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/nepomukcollection/meta/NepomukTrack.h <span style="color: grey">(8b04f26081126809ceaf51d60898955848f28584)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp <span style="color: grey">(40b413fb8302c1f93fa55e8ec462ebb25375ce81)</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>