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

Phalgun Guduthur phalgun.guduthur at gmail.com
Sun Sep 23 02:35:41 UTC 2012 

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

Ship it!


I went through the Nepomuk collection code that was changed. It looks good. 

After I implement Resource Watcher to watch the Nepomuk resources, I had to implement a variant of these functions myself. So its good that I already have them now. 

- Phalgun Guduthur


On Sept. 22, 2012, 3:31 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106494/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2012, 3:31 p.m.)
> 
> 
> Review request for Amarok, Bart Cerneels, Ralf Engels, and Phalgun Guduthur.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   ChangeLog fa64799c18702ff777ec013e3b8726a9b8703b45 
>   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/nepomukcollection/meta/NepomukTrack.h 8b04f26081126809ceaf51d60898955848f28584 
>   src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp 40b413fb8302c1f93fa55e8ec462ebb25375ce81 
>   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/20120923/24801130/attachment-0001.html>

More information about the Amarok-devel mailing list