Review Request: Track statistics refactor (intended to be merged ASAP)

Matěj Laitl matej at laitl.cz
Tue Sep 18 15:52:49 UTC 2012 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106494/
-----------------------------------------------------------

Review request for Amarok, Bart Cerneels and Ralf Engels.


Description
-------

{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.


Diffs
-----

  ChangeLog 8460b80e558c24f71fb9adfbd08a4427e0606e09 
  playground/src/context/applets/coverbling/CoverBlingApplet.cpp 5ef376a93a23b3a0c6f3323b31d8ddc82947b670 
  src/CMakeLists.txt 8596144087709d9baab19b63c35f0c8169dca427 
  src/MainWindow.cpp 0530fd725f41d00ce5f5b3aed68ab6e06a5c86b3 
  src/TrayIcon.cpp 51b2afd528234a3e2f01f2f02f6c496c08249d9b 
  src/context/applets/currenttrack/CurrentTrack.cpp aa83a66f39a970b6927d0b63928c79ae55eeec9b 
  src/context/widgets/RecentlyPlayedListWidget.cpp 3b7bd7207b6a69be468bc241083abe67fb7f3b2d 
  src/core-impl/collections/audiocd/AudioCdMeta.h d2ef5dbfab54a965bd66731fc921cc787d40264a 
  src/core-impl/collections/audiocd/AudioCdMeta.cpp b24cd1bda95f14d9c5be06c64985f303784be72f 
  src/core-impl/collections/daap/DaapMeta.h e0cf9f3ae1952fb0b0ec8632b5645b340c3ac7bd 
  src/core-impl/collections/daap/DaapMeta.cpp 2b01ebf37ebccb6ae2fcec630638fed262b0eebf 
  src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp e344e0f8a8f26450b4340a64188cab72fa1a755c 
  src/core-impl/collections/db/sql/SqlCollectionLocation.cpp c55d3986e43bf52f88016e4f353b71c84478c139 
  src/core-impl/collections/db/sql/SqlMeta.h 4450dabcd6947341de035c53b2e6998280355ef9 
  src/core-impl/collections/db/sql/SqlMeta.cpp c05e563120ccda2d957ba5c1de507e73e50cbfc4 
  src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp 8d4b5852ee867e9698fb92e086c43d1cab4e429f 
  src/core-impl/collections/ipodcollection/IpodMeta.h 5d54d8ce8586c7b7c715abb7bfb748403355fb6d 
  src/core-impl/collections/ipodcollection/IpodMeta.cpp 13b9d913f744ebef9ac572c782193f0f9a532394 
  src/core-impl/collections/ipodcollection/IpodMetaEditCapability.h a19691e3dfc7fbb20befefc19dbd9c2a00c8af47 
  src/core-impl/collections/ipodcollection/IpodMetaEditCapability.cpp bc62412923ebf6c90de7b0c180a18c238d2fd0b8 
  src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h 2d0706e03a80d0f182c4dc9613751ef731385bcc 
  src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 3946b8ffd4e75ba5e5bc259a60a683883abefa31 
  src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp 98e751f363f983b2857d9702414d150412c5dce7 
  src/core-impl/collections/playdarcollection/PlaydarMeta.h 93d1c2e103a737682eb05f70fc1db43245d6e313 
  src/core-impl/collections/playdarcollection/PlaydarMeta.cpp c9b07f37abc3a0c40dea0a0642e9120249b4efb6 
  src/core-impl/collections/playdarcollection/support/Query.cpp 98a1eb51479236d85e384dcc989be2660f20d6d1 
  src/core-impl/collections/proxycollection/ProxyCollectionMeta.h e5d7266840ca244c884e1c397258e9d4d1a961de 
  src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp 091f5f6b484c8a8f66111d8a251c76f08ed2e632 
  src/core-impl/collections/support/MemoryCustomValue.cpp f21ac9f8cfde8846a43007f2f355596c1a93fd5a 
  src/core-impl/collections/support/MemoryMeta.h 675cf425d43dc9d812af0bfabd2c01b2053d2852 
  src/core-impl/collections/upnpcollection/UpnpMeta.h f4a62db7d71a38054ba61bb0c690e1cb2a12661a 
  src/core-impl/collections/upnpcollection/UpnpMeta.cpp 708bfb8c5682bd4fd003eab70869df1daecddccb 
  src/core-impl/meta/file/File.h df96a2a1d74a11f6b559a1d89d27d2535df1107c 
  src/core-impl/meta/file/File.cpp 3ee6080df7eeb3810438f7405913f42336407521 
  src/core-impl/meta/file/File_p.h 57131f452a92cc2940d93c8535975a5131d16f7e 
  src/core-impl/meta/multi/MultiTrack.h 63a8651628eb6335cd0a3e635150bfae95e83235 
  src/core-impl/meta/proxy/MetaProxy.h d6dc8f21a5978a362c8005fa4816183bb6ae1090 
  src/core-impl/meta/proxy/MetaProxy.cpp 0ba84e6d1ef2aad2d1624e1c4898768f8c2f3b22 
  src/core-impl/meta/stream/Stream.h 8b64d89f79752e0b8d67c78a2438c42e0edc2611 
  src/core-impl/meta/stream/Stream.cpp 3eb54c74fd77240a3da3cc92b4072c9dae7468f3 
  src/core-impl/meta/stream/Stream_p.h 390109ab276608768e1a78cb2e4ef1063527b49e 
  src/core-impl/meta/timecode/TimecodeMeta.h ceb4d66f43fb21648aa7c0f12c9f33ea4d4e4ad6 
  src/core-impl/meta/timecode/TimecodeMeta.cpp f85476d0af2a80b97de795bf557a3eca369c1d6a 
  src/core-impl/statistics/providers/tag/TagStatisticsProvider.h df79a00726aee5d05b45847a1db515c291b8ee0f 
  src/core-impl/statistics/providers/tag/TagStatisticsProvider.cpp 7754b8ae4069775eaa417b87a4bb4e22b1535c1f 
  src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.h 7b6b3bb891b0b968f8402ffd05c9af1dfe7f8998 
  src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.cpp 9855954813b95c6036ab7eda9f1cd7c794dfbad4 
  src/core-impl/support/PersistentStatisticsStore.h PRE-CREATION 
  src/core-impl/support/PersistentStatisticsStore.cpp PRE-CREATION 
  src/core-impl/support/TagStatisticsStore.h PRE-CREATION 
  src/core-impl/support/TagStatisticsStore.cpp PRE-CREATION 
  src/core-impl/support/UrlStatisticsStore.h PRE-CREATION 
  src/core-impl/support/UrlStatisticsStore.cpp PRE-CREATION 
  src/core/CMakeLists.txt 667f2bc2d6e45838efce91c7f47c33a110d77330 
  src/core/capabilities/Capability.h 3d3354711c4a3d289e704c0ca957d11e25e62092 
  src/core/capabilities/StatisticsCapability.h 690ef1242a5dc6b0e218b5df3f7464c2182670f7 
  src/core/capabilities/StatisticsCapability.cpp 34af392070569040814153795d3d5310ce6d2824 
  src/core/meta/Meta.h b9045f567fdb386fbc78b5ca35545e2f91cc3f8f 
  src/core/meta/Meta.cpp cb0e5c9f337fc781daa3b0038f7e941932a40f3f 
  src/core/meta/Statistics.h PRE-CREATION 
  src/core/meta/Statistics.cpp PRE-CREATION 
  src/core/meta/support/MetaConstants.cpp 079fd22016b2c340cad6a42578826add1226df6a 
  src/core/meta/support/MetaUtility.cpp 133076b6163bee5f5d2c92f3c86b6514d1719665 
  src/core/podcasts/PodcastMeta.h 0cfaea1e708828a35c3a68f14ff0375e5a7f7509 
  src/core/statistics/StatisticsProvider.h 6f5491209df3dd64bcb0398f7054df2b47d7036f 
  src/core/statistics/StatisticsProvider.cpp 0b5a788626fb4620740893dda68022c32abb8cd7 
  src/core/support/Amarok.h d8c91e789d02b204202c8a99b9459b8d10b3d90d 
  src/databaseimporter/amarok14/FastForwardWorker.cpp 56e319b24b2544489d9e9491f3e3c037ec7b63b2 
  src/databaseimporter/itunes/ITunesImporterWorker.cpp e5ad5e6194547b91b57920a8692e5bbf9a14fea7 
  src/dialogs/TagDialog.cpp a448458984c3920e510be9d4693c2e469765079a 
  src/dialogs/TrackOrganizer.cpp 331f1ef088fd4e6c69eade85ae9ed485faed90b4 
  src/playlist/PlaylistModel.cpp 700c9096a1810c89106638a27aee2260439def55 
  src/playlist/navigators/FavoredRandomTrackNavigator.cpp 3d57a8261695a46e4d7545068e9a0b9a89382aec 
  src/playlist/proxymodels/GroupingProxy.cpp 1e32d9ca44f6d5e8bc955f8b54718fe766027ff8 
  src/playlist/proxymodels/ProxyBase.cpp 06306adee1f41100a8bff489845e4b3eea9b271d 
  src/playlist/proxymodels/SortAlgorithms.cpp d6a395dab2845e41c461037c8d00ea22446e7934 
  src/playlist/view/listview/PrettyItemDelegate.cpp d3c2c55de12c0078cf9bed0b2974c91c96e93149 
  src/playlistgenerator/constraints/TagMatch.cpp 01c09e55d83c6512a9da6626a6ca218bb3753941 
  src/scriptengine/MetaTypeExporter.cpp b944c8e72471a6d66956df938b2c94e248cb9f37 
  src/services/ServiceMetaBase.h 793eb9ebbd35495984734ed04bd159c2ff1b7bb5 
  src/services/ServiceMetaBase.cpp 039146b29d648557c6b1960e84b23872b4da823e 
  src/services/lastfm/meta/LastFmMeta.h 6d93cad50fba629a4a278e3380b56859dde5616a 
  src/services/lastfm/meta/LastFmMeta.cpp 0e7659bd570d0dff18e6555b772228937bb9269b 
  src/services/lastfm/meta/LastFmMeta_p.h 33f3b73b18fef3227811011fa3900ed5fbe50fcf 
  src/services/magnatune/MagnatuneMeta.cpp 523617be763834927d685fb070778e39c20b114f 
  src/widgets/Osd.cpp 423be4a009c82f71f23a364c16a170a24db2f8ac 
  tests/core-impl/collections/db/sql/TestSqlScanManager.cpp 2dbed81808989b96361b99cf0650699b53fc28fa 
  tests/core-impl/collections/db/sql/TestSqlTrack.cpp cb789f71350b8acf7aa6272846d36e7b6c505308 
  tests/core-impl/meta/file/TestMetaFileTrack.h a7759e0703301870d22791eb9a4c5b60368fc637 
  tests/core-impl/meta/file/TestMetaFileTrack.cpp 7549a585e25e2e552d633df2fd9174aeaf7813b1 
  tests/core/meta/TestMetaTrack.cpp 0adb7633538533202c50487b1eaff8e4ead150e9 
  tests/mocks/MetaMock.h 6a706a4472b127fa38f596cb2c780e3ef2278dba 

Diff: http://git.reviewboard.kde.org/r/106494/diff/


Testing
-------

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


Thanks,

Matěj Laitl

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120918/e22fc1e1/attachment-0001.html>

More information about the Amarok-devel mailing list