Bug 433509 - "Play count" inappropriately incremented when track starts playing, not when it finishes
Summary: "Play count" inappropriately incremented when track starts playing, not when ...
Status: RESOLVED FIXED
Alias: None
Product: Elisa
Classification: Applications
Component: general (show other bugs)
Version: 20.12.2
Platform: Debian unstable Linux
: HI wishlist
Target Milestone: ---
Assignee: Matthieu Gallien
URL:
Keywords: usability
Depends on:
Blocks:
 
Reported: 2021-02-23 21:02 UTC by Dave
Modified: 2023-01-31 20:53 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 23.04
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dave 2021-02-23 21:02:57 UTC
The play count for a song increases when the song is played for 1 second and skipped. This exaggerates the impact a song has had in the user experience and preferences. I suggest to omit the play count increment when the song playback doesn't reach certain time threshold. I would say a song has to play for over half its running time, not including time skips, to have its play count increased.

Elisa: 20.12.2
KDE Plasma Version: 5.20.5
KDE Frameworks Version: 5.78.0
Qt Version: 5.15.2
Comment 1 Nate Graham 2021-02-25 04:16:43 UTC
Yeah. I'd even say 90% or more.
Comment 2 David Fischer 2022-01-03 04:06:28 UTC
Play count increment seems to be triggered when ElisaApplication::initializePlayer() which calls &DatabaseInterface::trackHasStartedPlaying
Comment 3 Nate Graham 2022-04-11 00:39:40 UTC
In fact, this seems to simply be a bug: the play count *only* increases when a track is skipped, but not when it goes to completion. I suspect a conditional statement in the code is simply inverted somewhere.
Comment 4 fw.smit01 2022-12-27 19:28:02 UTC
(In reply to Nate Graham from comment #3)
> In fact, this seems to simply be a bug: the play count *only* increases when
> a track is skipped, but not when it goes to completion. I suspect a
> conditional statement in the code is simply inverted somewhere.

In my testing it increments the counter when it starts playing a song, like david says. You don't have to skip it.

For testing I put a few song in the queue. I play the first song and put it on pause. The second song has 6 plays. Then I skip over to the second song. It still has 6 plays. Skipping to the third song doesn't increment the play count. But when I press play when the second song is selected, the play count immediately increases to 7.

The better behaviour would be to only increment the counter when finishing a song. Then you can be pretty sure the user has listened to the entire song. More than 90% played is also possible, but then you have to run some code every time the timestamp changes, so I suggest going for the first one (incrementing when finishing the song).
Comment 5 Nate Graham 2022-12-27 19:38:44 UTC
Indeed, all the action takes place in updateTrackStatistics(), which is only ever called in DatabaseInterface::trackHasStartedPlaying(). Let me see if I can fix it.
Comment 6 fw.smit01 2022-12-27 19:56:27 UTC
(In reply to Nate Graham from comment #5)
> Indeed, all the action takes place in updateTrackStatistics(), which is only
> ever called in DatabaseInterface::trackHasStartedPlaying(). Let me see if I
> can fix it.

Actually, I was trying to fix it. I just need a little bit of guidance. What function should the update be called from instead?
Comment 7 Nate Graham 2022-12-27 20:01:43 UTC
Oh nice, I'll let you take over.

In manageaudioplayer.h, we'll want a new signal finishedPlayingTrack(). That should get emitted in somewhere in ManageAudioPlayer::setPlayerPlaybackState() and possible also in ManageAudioPlayer::setCurrentTrack() if mSkippingCurrentTrack is false.

Then in elisaapplication.cpp, we'll want to connect ManageAudioPlayer::finishedPlayingTrack to a new function in databaseinterface.cpp called DatabaseInterface::trackHasFinishedPlaying(). That new function should be responsible for the logic that has "`PlayCounter` = `PlayCounter` + 1 " in it.

Or something like that. :)
Comment 8 fw.smit01 2022-12-27 22:16:44 UTC
(In reply to Nate Graham from comment #7)
> Oh nice, I'll let you take over.
> 
> In manageaudioplayer.h, we'll want a new signal finishedPlayingTrack(). That
> should get emitted in somewhere in
> ManageAudioPlayer::setPlayerPlaybackState() and possible also in
> ManageAudioPlayer::setCurrentTrack() if mSkippingCurrentTrack is false.
> 
> Then in elisaapplication.cpp, we'll want to connect
> ManageAudioPlayer::finishedPlayingTrack to a new function in
> databaseinterface.cpp called DatabaseInterface::trackHasFinishedPlaying().
> That new function should be responsible for the logic that has
> "`PlayCounter` = `PlayCounter` + 1 " in it.
> 
> Or something like that. :)

Thanks for the guidance, that was very helpful! I have posted a MR: https://invent.kde.org/multimedia/elisa/-/merge_requests/404 (🤖 ⚠️merge request not found)
Comment 9 Nate Graham 2023-01-31 20:53:48 UTC
Git commit cd1c2f1afa96b05766cbea6fb5c9c49ef147f898 by Nate Graham, on behalf of Friso Smit.
Committed on 31/01/2023 at 20:53.
Pushed by ngraham into branch 'master'.

Change track stats only when finishing song
FIXED-IN: 23.04

M  +26   -15   autotests/databaseinterfacetest.cpp
M  +77   -22   src/databaseinterface.cpp
M  +5    -1    src/databaseinterface.h
M  +2    -0    src/elisaapplication.cpp
M  +3    -0    src/manageaudioplayer.cpp
M  +2    -0    src/manageaudioplayer.h

https://invent.kde.org/multimedia/elisa/commit/cd1c2f1afa96b05766cbea6fb5c9c49ef147f898