Bug 111690

illisius, could you take a look at this please?

> illisius, could you take a look at this please?
Certainly.
This is because we just keep a pointer to the track to stop after -- which, after it has been removed from the playlist, it naturally can't point to any longer. I'll see if I can come up with a solution to this that doesn't suck.

I have reproduced this bug using the svn version (rev 537005). Moreover, I found another bug linked to this one that crash amaroK.
How to reproduce :
- select the current track to "Stop after current track"
- select all the tracks in the playlist and remove them
- add other files in the playlist (did not try with same files...)
- wait for the current track to finish (or seek near the end)
- according to the former bug, amaroK should not play other tracks, but it does play the first track of the new playlist
- select "Stop after current track" --> amaroK crash

I will enclose the full debug trace of amaroK, as the backtrace.

I have taken an (humble) eye on the code, and as Gábor wrote, it's a problem with the pointer to the track to stop after.

I may have a solution, but I don't know if it is a good design! IMHO, if the user remove the track to stop after from the playlist, then reset the "stop after" status. It is really "stop after *this* track", not "stop once the end of a track has been reached".

In that case, I may have a patch that solve the bug...

Created attachment 15903 [details]
Debug log and backtrace

Backtrace at the bottom

Created attachment 15904 [details]
Patch for the bug #111690

svn diff against rev 537005

crash still reproducable with svn 558575

SVN commit 582322 by aoliveira:

Don't keep a m_stopAfterTrack pointing to a deleted item, as it would lead to crashes.
Thanks for the patch, Stephane Deraco <steph.dev@gmail.com>
CCBUG: 111690



 M  +7 -1      playlist.cpp  


--- trunk/extragear/multimedia/amarok/src/playlist.cpp #582321:582322
@@ -2222,6 +2222,10 @@
     m_prevTracks.clear();
     m_prevAlbums.clear();
 
+    if (m_stopAfterTrack) {
+        m_stopAfterTrack = 0;
+        setStopAfterMode( DoNotStop );
+    }
     const PLItemList prev = m_nextTracks;
     m_nextTracks.clear();
     emit queueChanged( PLItemList(), prev );
@@ -4377,8 +4381,10 @@
         }
     }
 
-    if( m_stopAfterTrack == item )
+    if( m_stopAfterTrack == item ) {
         m_stopAfterTrack = 0; //to be safe
+        setStopAfterMode( DoNotStop );
+    }
 
     //keep m_nextTracks queue synchronized
     if( m_nextTracks.removeRef( item ) && !multi )

The following errors spew out in the console during the time the gui is frozen while using helix.  This is only an excerpt, there is probably about 1000 lines.

X Error: BadIDChoice (invalid resource ID chosen for this connection) 14
  Major opcode:  1
  Minor opcode:  0
  Resource id:  0x3200e22
X Error: BadIDChoice (invalid resource ID chosen for this connection) 14
  Major opcode:  1
  Minor opcode:  0
  Resource id:  0x3200e23
X Error: BadIDChoice (invalid resource ID chosen for this connection) 14
  Major opcode:  1
  Minor opcode:  0
  Resource id:  0x3200e24
X Error: BadIDChoice (invalid resource ID chosen for this connection) 14
  Major opcode:  1
  Minor opcode:  0
  Resource id:  0x3200e25
X Error: BadIDChoice (invalid resource ID chosen for this connection) 14
  Major opcode:  53
  Minor opcode:  0
  Resource id:  0x3200e26

Adding my support for this request / bug. It would be nice if AmaroK remembered that the user had selected "stop after current song", even if he then removed the song from the playlist.

I sometimes clear and "pre-fill" the playlist with what suits my mood, even if I don't intend to continue listening right-away. I also practically always use "stop after current" rather than plain "stop", as I dislike cutting off a song in the middle.

SVN commit 616542 by aoliveira:

Let Stop After Current work even when current track is removed from playlist.
It's a bit confusing I think. Maybe we need some indication that it's going to stop (maybe a different icon on the section of the down 
arrow/repeat modes?)
RFC
BUG: 111690


 M  +45 -27    playlist.cpp  
 M  +3 -1      playlist.h  


--- trunk/extragear/multimedia/amarok/src/playlist.cpp #616541:616542
@@ -179,6 +179,7 @@
         , m_undoCounter( 0 )
         , m_dynamicMode( 0 )
         , m_stopAfterTrack( 0 )
+        , m_stopAfterMode( DoNotStop )
         , m_showHelp( true )
         , m_dynamicDirt( false )
         , m_queueDirt( false )
@@ -939,7 +940,7 @@
 {
     PlaylistItem *item = currentTrack();
 
-    if( !m_visCount || ( m_currentTrack && m_stopAfterTrack == m_currentTrack  ) )
+    if( !m_visCount || stopAfterMode() == StopAfterCurrent )
     {
         if( dynamicMode() && m_visCount )
         {
@@ -948,7 +949,7 @@
             m_dynamicDirt = false;
         }
 
-        m_stopAfterTrack = 0;
+        setStopAfterMode(DoNotStop);
         EngineController::instance()->stop();
 
         if( !AmarokConfig::randomMode() ) {
@@ -1179,7 +1180,7 @@
                         upcomingTracks > dynamicMode()->upcomingCount();
 
     //keep upcomingTracks requirement, this seems to break StopAfterCurrent
-    if( !dontAppend && m_stopAfterTrack != m_currentTrack )
+    if( !dontAppend && stopAfterMode() != StopAfterCurrent )
     {
         addDynamicModeTracks( 1 );
     }
@@ -1410,10 +1411,12 @@
 {
     PlaylistItem *prev_stopafter = m_stopAfterTrack;
 
-    if( on )
-        m_stopAfterTrack = m_currentTrack;
-    else
-        m_stopAfterTrack = 0;
+    if( on ) {
+        setStopAfterItem( m_currentTrack );
+    }
+    else {
+        setStopAfterMode( DoNotStop );
+    }
 
     if( m_stopAfterTrack )
         m_stopAfterTrack->update();
@@ -1421,6 +1424,20 @@
         prev_stopafter->update();
 }
 
+void Playlist::setStopAfterItem( PlaylistItem *item ) {
+    if( !item ) {
+        setStopAfterMode( DoNotStop );
+        return;
+    }
+    else if( item == m_currentTrack )
+        setStopAfterMode( StopAfterCurrent );
+    else if( item == m_nextTracks.getLast() )
+        setStopAfterMode( StopAfterQueue );
+    else
+        setStopAfterMode( StopAfterQueue );
+    m_stopAfterTrack = item;
+}
+
 void Playlist::toggleStopAfterCurrentItem()
 {
     PlaylistItem *item = currentItem();
@@ -1430,11 +1447,13 @@
         return;
 
     PlaylistItem *prev_stopafter = m_stopAfterTrack;
-    if( m_stopAfterTrack == item )
+    if( m_stopAfterTrack == item ) {
             m_stopAfterTrack = 0;
+            setStopAfterMode( DoNotStop );
+    }
     else
     {
-        m_stopAfterTrack = item;
+        setStopAfterItem( item );
         item->setSelected( false );
         item->update();
     }
@@ -1451,12 +1470,12 @@
 
     PlaylistItem *prev_stopafter = m_stopAfterTrack;
     if( m_stopAfterTrack == item ) {
-        m_stopAfterTrack = 0;
+        setStopAfterMode( DoNotStop );
         Amarok::OSD::instance()->OSDWidget::show( i18n("Stop Playing After Track: Off") );
     }
     else
     {
-        m_stopAfterTrack = item;
+        setStopAfterItem( item );
         item->setSelected( false );
         item->update();
         Amarok::OSD::instance()->OSDWidget::show( i18n("Stop Playing After Track: On") );
@@ -1469,7 +1488,7 @@
 void Playlist::setStopAfterMode( int mode )
 {
     PlaylistItem *prevStopAfter = m_stopAfterTrack;
-
+    m_stopAfterMode = mode;
     switch( mode )
     {
         case DoNotStop:
@@ -1489,18 +1508,14 @@
         m_stopAfterTrack->update();
 }
 
-int Playlist::stopAfterMode() const
+int Playlist::stopAfterMode()
 {
-    if( !m_stopAfterTrack )
-        return DoNotStop;
-    else if( m_stopAfterTrack == m_currentTrack )
-        return StopAfterCurrent;
-    else if( m_stopAfterTrack == m_nextTracks.getLast() )
-        return StopAfterQueue;
-    else if( m_stopAfterTrack )
-        return StopAfterOther;
-    else
-        return DoNotStop;
+    if ( m_stopAfterMode != DoNotStop
+         && m_stopAfterTrack && m_stopAfterTrack == m_currentTrack ) {
+         m_stopAfterMode = StopAfterCurrent;
+    }
+
+    return m_stopAfterMode;
 }
 
 void Playlist::generateInfo()
@@ -2051,8 +2066,8 @@
 
             PlaylistItem::setPixmapChanged();
 
-            if( m_stopAfterTrack == m_currentTrack )
-                m_stopAfterTrack = 0; //we just stopped
+            if( stopAfterMode() == StopAfterCurrent )
+                setStopAfterMode( DoNotStop );
 
             //reset glow state
             slotGlowTimer();
@@ -2099,7 +2114,9 @@
 
     if (m_stopAfterTrack) {
         m_stopAfterTrack = 0;
-        setStopAfterMode( DoNotStop );
+        if ( stopAfterMode() != StopAfterCurrent ) {
+            setStopAfterMode( DoNotStop );
+        }
     }
     const PLItemList prev = m_nextTracks;
     m_nextTracks.clear();
@@ -4314,7 +4331,8 @@
 
     if( m_stopAfterTrack == item ) {
         m_stopAfterTrack = 0; //to be safe
-        setStopAfterMode( DoNotStop );
+        if (stopAfterMode() != StopAfterCurrent)
+            setStopAfterMode( DoNotStop );
     }
 
     //keep m_nextTracks queue synchronized
--- trunk/extragear/multimedia/amarok/src/playlist.h #616541:616542
@@ -138,7 +138,7 @@
         //call this every time you modifyDynamicMode(), otherwise you'll get memory leaks and/or crashes
         void finishedModifying( DynamicMode *mode );
 
-        int  stopAfterMode() const;
+        int  stopAfterMode();
 
         void addCustomMenuItem ( const QString &submenu, const QString &itemTitle );
         void customMenuClicked ( int id );
@@ -230,6 +230,7 @@
         void setFilter( const QString &filter );
         void setFilterSlot( const QString &filter );                       //uses a delay where applicable
         void setStopAfterCurrent( bool on );
+        void setStopAfterItem( PlaylistItem *item );
         void toggleStopAfterCurrentItem();
         void toggleStopAfterCurrentTrack();
         void setStopAfterMode( int mode );
@@ -388,6 +389,7 @@
         DynamicMode  *m_dynamicMode;
         KURL::List    m_queueList;
         PlaylistItem *m_stopAfterTrack;
+        int           m_stopAfterMode;
         bool          m_showHelp;
         bool          m_dynamicDirt;        //So we don't call advanceDynamicTrack() on activate()
         bool          m_queueDirt;          //When queuing disabled items, we need to place the marker on the newly inserted item