Bug 438651

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