Bug 111690 - Amarok does not stop after current track if track is not in playlist
Summary: Amarok does not stop after current track if track is not in playlist
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: Playlist (show other bugs)
Version: 1.3-SVN
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-29 06:35 UTC by Laszlo Pandy
Modified: 2006-12-26 04:10 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Debug log and backtrace (22.53 KB, text/plain)
2006-05-04 04:34 UTC, Stéphane Deraco
Details
Patch for the bug #111690 (773 bytes, patch)
2006-05-04 04:35 UTC, Stéphane Deraco
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Pandy 2005-08-29 06:35:31 UTC
Version:           1.3-SVN (using KDE 3.3.2, Mandrake Linux Cooker i586 - Cooker)
Compiler:          gcc version 3.4.3 (Mandrakelinux 10.2 3.4.3-7mdk)
OS:                Linux (i686) release 2.6.11-6mdk

If you select the current track to stop playing, but then clear the playlist and add new items, amarok will start playing the new playlist instead of stopping when its supposed to. This is bad if a song is playing which is not in your playlist and you select stop playing "after current track" from the new stop button, but instead it starts from the beginning of the playlist.
Comment 1 Seb Ruiz 2005-10-23 09:31:19 UTC
illisius, could you take a look at this please?
Comment 2 Gábor Lehel 2005-10-23 10:30:22 UTC
> 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.
Comment 3 Stéphane Deraco 2006-05-04 04:31:56 UTC
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...
Comment 4 Stéphane Deraco 2006-05-04 04:34:12 UTC
Created attachment 15903 [details]
Debug log and backtrace

Backtrace at the bottom
Comment 5 Stéphane Deraco 2006-05-04 04:35:02 UTC
Created attachment 15904 [details]
Patch for the bug #111690

svn diff against rev 537005
Comment 6 richlv 2006-07-05 18:11:51 UTC
crash still reproducable with svn 558575
Comment 7 Alexandre Oliveira 2006-09-09 03:31:05 UTC
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 )
Comment 8 Greg Meyer 2006-11-05 17:09:06 UTC
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
Comment 9 munlinux 2006-11-28 21:34:04 UTC
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.
Comment 10 Alexandre Oliveira 2006-12-26 04:10:28 UTC
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