Review Request: Track statistics refactor (intended to be merged ASAP)
Matěj Laitl
matej at laitl.cz
Tue Sep 18 17:08:44 UTC 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106494/
-----------------------------------------------------------
(Updated Sept. 18, 2012, 5:08 p.m.)
Review request for Amarok, Bart Cerneels and Ralf Engels.
Changes
-------
Add "Meta::Statistics and subclasses: add docs and asserts wrt thread safety" commit.
Description (updated)
-------
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.
Diffs (updated)
-----
tests/mocks/MetaMock.h 6a706a4472b127fa38f596cb2c780e3ef2278dba
tests/core/meta/TestMetaTrack.cpp 0adb7633538533202c50487b1eaff8e4ead150e9
tests/core-impl/meta/file/TestMetaFileTrack.cpp 7549a585e25e2e552d633df2fd9174aeaf7813b1
tests/core-impl/collections/db/sql/TestSqlTrack.cpp cb789f71350b8acf7aa6272846d36e7b6c505308
tests/core-impl/meta/file/TestMetaFileTrack.h a7759e0703301870d22791eb9a4c5b60368fc637
src/widgets/Osd.cpp 423be4a009c82f71f23a364c16a170a24db2f8ac
tests/core-impl/collections/db/sql/TestSqlScanManager.cpp 2dbed81808989b96361b99cf0650699b53fc28fa
src/services/lastfm/meta/LastFmMeta_p.h 33f3b73b18fef3227811011fa3900ed5fbe50fcf
src/services/magnatune/MagnatuneMeta.cpp 523617be763834927d685fb070778e39c20b114f
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/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/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/databaseimporter/amarok14/FastForwardWorker.cpp 56e319b24b2544489d9e9491f3e3c037ec7b63b2
src/core/support/Amarok.h d8c91e789d02b204202c8a99b9459b8d10b3d90d
src/core/statistics/StatisticsProvider.cpp 0b5a788626fb4620740893dda68022c32abb8cd7
src/core/statistics/StatisticsProvider.h 6f5491209df3dd64bcb0398f7054df2b47d7036f
src/core/podcasts/PodcastMeta.h 0cfaea1e708828a35c3a68f14ff0375e5a7f7509
src/core/meta/support/MetaConstants.cpp 079fd22016b2c340cad6a42578826add1226df6a
src/core/meta/support/MetaUtility.cpp 133076b6163bee5f5d2c92f3c86b6514d1719665
src/core/meta/Statistics.cpp PRE-CREATION
src/core/meta/Statistics.h PRE-CREATION
src/core/meta/Meta.cpp cb0e5c9f337fc781daa3b0038f7e941932a40f3f
src/core/meta/Meta.h b9045f567fdb386fbc78b5ca35545e2f91cc3f8f
src/core/capabilities/StatisticsCapability.h 690ef1242a5dc6b0e218b5df3f7464c2182670f7
src/core/capabilities/StatisticsCapability.cpp 34af392070569040814153795d3d5310ce6d2824
src/core/CMakeLists.txt 667f2bc2d6e45838efce91c7f47c33a110d77330
src/core/capabilities/Capability.h 3d3354711c4a3d289e704c0ca957d11e25e62092
src/core-impl/support/UrlStatisticsStore.cpp PRE-CREATION
src/core-impl/support/UrlStatisticsStore.h PRE-CREATION
src/core-impl/support/TagStatisticsStore.cpp PRE-CREATION
src/core-impl/support/TagStatisticsStore.h PRE-CREATION
src/core-impl/support/PersistentStatisticsStore.cpp PRE-CREATION
src/core-impl/support/PersistentStatisticsStore.h PRE-CREATION
src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.h 7b6b3bb891b0b968f8402ffd05c9af1dfe7f8998
src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.cpp 9855954813b95c6036ab7eda9f1cd7c794dfbad4
src/core-impl/statistics/providers/tag/TagStatisticsProvider.cpp 7754b8ae4069775eaa417b87a4bb4e22b1535c1f
src/core-impl/meta/timecode/TimecodeMeta.cpp f85476d0af2a80b97de795bf557a3eca369c1d6a
src/core-impl/statistics/providers/tag/TagStatisticsProvider.h df79a00726aee5d05b45847a1db515c291b8ee0f
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/stream/Stream.h 8b64d89f79752e0b8d67c78a2438c42e0edc2611
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/file/File_p.h 57131f452a92cc2940d93c8535975a5131d16f7e
src/core-impl/meta/file/File.h df96a2a1d74a11f6b559a1d89d27d2535df1107c
src/core-impl/meta/file/File.cpp 3ee6080df7eeb3810438f7405913f42336407521
src/core-impl/collections/upnpcollection/UpnpMeta.h f4a62db7d71a38054ba61bb0c690e1cb2a12661a
src/core-impl/collections/upnpcollection/UpnpMeta.cpp 708bfb8c5682bd4fd003eab70869df1daecddccb
src/core-impl/collections/support/MemoryCustomValue.cpp f21ac9f8cfde8846a43007f2f355596c1a93fd5a
src/core-impl/collections/support/MemoryMeta.h 675cf425d43dc9d812af0bfabd2c01b2053d2852
src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp 091f5f6b484c8a8f66111d8a251c76f08ed2e632
src/core-impl/collections/playdarcollection/support/Query.cpp 98a1eb51479236d85e384dcc989be2660f20d6d1
src/core-impl/collections/proxycollection/ProxyCollectionMeta.h e5d7266840ca244c884e1c397258e9d4d1a961de
src/core-impl/collections/playdarcollection/PlaydarMeta.cpp c9b07f37abc3a0c40dea0a0642e9120249b4efb6
src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp 98e751f363f983b2857d9702414d150412c5dce7
src/core-impl/collections/playdarcollection/PlaydarMeta.h 93d1c2e103a737682eb05f70fc1db43245d6e313
src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h 2d0706e03a80d0f182c4dc9613751ef731385bcc
src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 3946b8ffd4e75ba5e5bc259a60a683883abefa31
src/core-impl/collections/ipodcollection/IpodMetaEditCapability.h a19691e3dfc7fbb20befefc19dbd9c2a00c8af47
src/core-impl/collections/ipodcollection/IpodMetaEditCapability.cpp bc62412923ebf6c90de7b0c180a18c238d2fd0b8
src/core-impl/collections/ipodcollection/IpodMeta.cpp 13b9d913f744ebef9ac572c782193f0f9a532394
src/core-impl/collections/ipodcollection/IpodMeta.h 5d54d8ce8586c7b7c715abb7bfb748403355fb6d
src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp 8d4b5852ee867e9698fb92e086c43d1cab4e429f
src/core-impl/collections/db/sql/SqlMeta.cpp c05e563120ccda2d957ba5c1de507e73e50cbfc4
src/core-impl/collections/db/sql/SqlMeta.h 4450dabcd6947341de035c53b2e6998280355ef9
src/core-impl/collections/db/sql/SqlCollectionLocation.cpp c55d3986e43bf52f88016e4f353b71c84478c139
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/TrayIcon.cpp 51b2afd528234a3e2f01f2f02f6c496c08249d9b
src/context/applets/currenttrack/CurrentTrack.cpp aa83a66f39a970b6927d0b63928c79ae55eeec9b
ChangeLog 8460b80e558c24f71fb9adfbd08a4427e0606e09
playground/src/context/applets/coverbling/CoverBlingApplet.cpp 5ef376a93a23b3a0c6f3323b31d8ddc82947b670
src/CMakeLists.txt 8596144087709d9baab19b63c35f0c8169dca427
src/MainWindow.cpp 0530fd725f41d00ce5f5b3aed68ab6e06a5c86b3
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/8999610d/attachment-0001.html>
More information about the Amarok-devel
mailing list