SUMMARY When Amarok is playing a playlist, if the file of the next track does not exist, then Amarok starts playing the next track that exists. That's OK. However, if the user presses the Previous Track button (or a keyboard shortcut) and that previous file no longer exists... Amarok stalls. STEPS TO REPRODUCE 1. Copy three existing music files to /tmp, add them to the current Amarok playlist (in "standard" mode, not "random" nor "repeat" mode). 2. Start playing the track that is located before the new files, delete the new files, check that Amarok skips the nonexistent files. That's OK. 3. Press the Previous Track button and see that Amarok stalls. SOFTWARE/OS VERSIONS Operating System: Kubuntu 20.04 KDE Plasma Version: 5.18.5 KDE Frameworks Version: 5.68.0 Qt Version: 5.12.8 Kernel Version: 5.4.0-74-generic Amarok Version: 2.9.71 (Build Date: Jun 10 2021, from Pedro de Carvalho Gomes's PPA)
INFORMATION THAT MAY HELP We can see a `if( track->isPlayable() )` inside a loop in: quint64 Playlist::StandardTrackNavigator::chooseNextTrack( bool repeatPlaylist ) { if( !m_queue.isEmpty() ) return m_queue.first(); if( m_onlyQueue ) return 0; Meta::TrackPtr track; bool playableTrackFound = false; int nextRow; //search for a playable track in order from right after the currently active track till the end for( nextRow = m_model->activeRow() + 1 ; nextRow < m_model->qaim()->rowCount() ; nextRow++ ) // 'activeRow()' may be -1. { track = m_model->trackAt(nextRow); if( track->isPlayable() ) { playableTrackFound = true; break; } } [...] and maybe something similar can be used in void Playlist::Actions::requestPrevTrack() { m_nextTrackCandidate = m_navigator->requestLastTrack(); play( m_nextTrackCandidate ); } or quint64 Playlist::StandardTrackNavigator::requestLastTrack() { return likelyLastTrack(); } [...] quint64 Playlist::StandardTrackNavigator::likelyLastTrack() { int lastRow = m_model->activeRow() - 1; [...] or quint64 Playlist::NonlinearTrackNavigator::requestLastTrack() { doItemListsMaintenance(); quint64 lastItem = m_historyItems.isEmpty() ? 0 : m_historyItems.takeLast(); [...]
Hi Toni, thanks for the report. I will take a look as soon as I find some time. But it may be faster if you send the patch and test yourself, given that you have already invested the time to look at the code. If you're interested, I can help you with any build/development doubt you have
> Hi Toni, thanks for the report. I will take a look as soon as I find some time. > But it may be faster if you send the patch and test yourself, given that you > have already invested the time to look at the code. If you're interested, I can > help you with any build/development doubt you have Ok, Pedro. In <https://invent.kde.org/multimedia/amarok/-/merge_requests/29> I've proposed a first step in order to solve the problems, with a question. Thanks!
I take a look at this in the next few days. Thanks for the effort
I have revised and merged the code for StandardTrackNavigator. As I wrote at Gitlab, your logic was correct, and my suggestion was merely to refactor the two for-loops into a single do-while. But that I'll do myself, at the two choose*Track methods It's still pending for me to look at how to address the issue with Non-linear navigators
I fixed the problem for Non-linear playlists as well. Answering your question at the older Merger Request, m_model->trackAt(nextItem) returns a track by its row. That is, its position at the playlist. But here we should use m_model->trackById(nextItem) which returns a track by its ID, which is the info stored at m_historyItems I have fixed NonlinearTrackNavigator::requestLastTrack(), and now it scans till it finds a playable track in history. I'm considering to do the same for likelyLastTrack(), which is used to preview the "prev" track to be played. I just have to think about some use case, such as the file was temporarily removed (network filesystem, for example), than reappeared
> I fixed the problem for Non-linear playlists as well. That's excellent! :-) After it's commited, I can also perform some tests and tell about it. > I just have to think about some use case, such as the file was temporarily > removed (network filesystem, for example), then reappeared Yes, sometimes files and folders are renamed, deleted, moved; sometimes some a "mount" has not be done, etc. and if it's really good that Amarok is resilient to all of that. 👍
In the tests that I performed, using the Standard mode (it utilizes StandardTrackNavigator), everything worked as it was expected. Great! Using the Random Mode, if some track files do not exist: - The Previous Track button also works! - However, pressing the Next Track button sometimes fails. INFORMATION THAT MAY HELP Pedro Gomes lately made this improvement: Playlist::NonlinearTrackNavigator::requestLastTrack() { doItemListsMaintenance(); - quint64 lastItem = m_historyItems.isEmpty() ? 0 : m_historyItems.takeLast(); + quint64 lastItem = 0; + while (!m_historyItems.isEmpty()) + { + quint64 possibleLastItem = m_historyItems.takeLast(); + if (m_model->trackForId(possibleLastItem)->isPlayable()) { + lastItem = possibleLastItem; + break; + } + } setCurrentItem( nextItem ); return m_currentItem; } Perhaps a similar change is needed in the following function? Playlist::NonlinearTrackNavigator::requestNextTrack() { doItemListsMaintenance(); ItemList *donor = nextItemChooseDonorList(); quint64 nextItem = donor ? donor->takeFirst() : 0; setCurrentItem( nextItem ); return m_currentItem; } Thanks for Amarok!
Hey Toni, indeed something similar should be implemented for the next track. That is, a loop until a playable track is found. Can you take care of this? The code from requestLastTrack() can be used as inspiration
Git commit 270da2f699f60414c890bbcb476a168b3cc573ba by Tuomas Nurmi. Committed on 07/04/2024 at 07:42. Pushed by nurmi into branch 'master'. Guard against giving out only broken tracks in RandomTrackNavigators Related to fixes done to NonLinearNavigator.cpp in a previous commit. Provide equal or even redundant functionality, but both should probably remain as the changes in NonLinearNavigator also affect e.g. RandomAlbumNavigators, to which this kind of quite direct safeguardings are not maybe the way to go. M +1 -1 ChangeLog M +26 -16 src/playlist/navigators/FavoredRandomTrackNavigator.cpp M +12 -3 src/playlist/navigators/RandomTrackNavigator.cpp https://invent.kde.org/multimedia/amarok/-/commit/270da2f699f60414c890bbcb476a168b3cc573ba