Bug 144121 - Dynamic playlist crash when playing top track after sort.
Summary: Dynamic playlist crash when playing top track after sort.
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: general (show other bugs)
Version: 1.4-SVN
Platform: Gentoo Packages Linux
: NOR crash (vote)
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-04-12 09:52 UTC by Thomas Lindroth
Modified: 2007-05-09 18:34 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Lindroth 2007-04-12 09:52:15 UTC
Version:           SVN stable revision 646304 (using KDE KDE 3.5.6)
Installed from:    Gentoo Packages
Compiler:          gcc (GCC) 4.1.1 (Gentoo 4.1.1) 
OS:                Linux

Steps to reproduce:
* Load a dynamic playlist
* Play some track
* Sort playlist (on rating preferably)
* Play the top track in the playlist

More info will follow.
Comment 1 richlv 2007-04-12 10:07:50 UTC
confirmed in revision 650696 (sorting by score)

backtrace :

#0  0x00650069 in ?? ()
#1  0xb7d2cc16 in Playlist::activate (this=0x8211778, item=0x844f3ac) at playlist.cpp:1660
#2  0xb7d2c18e in Playlist::doubleClicked (this=0x8211778, item=0x844f3ac) at playlist.cpp:1529
#3  0xb7d3d953 in Playlist::qt_invoke (this=0x8211778, _id=163, _o=0xbfe648f0) at playlist.moc:544
#4  0xb65ed004 in QObject::activate_signal () from /usr/lib/qt/lib/libqt-mt.so.3
#5  0xb693f950 in QListView::doubleClicked () from /usr/lib/qt/lib/libqt-mt.so.3
#6  0xb6f04c0d in KListView::contentsMouseDoubleClickEvent () from /opt/kde/lib/libkdeui.so.4
#7  0xb670c125 in QScrollView::viewportMouseDoubleClickEvent () from /usr/lib/qt/lib/libqt-mt.so.3
#8  0xb670e6f4 in QScrollView::eventFilter () from /usr/lib/qt/lib/libqt-mt.so.3
#9  0xb66db618 in QListView::eventFilter () from /usr/lib/qt/lib/libqt-mt.so.3
#10 0xb7d324de in Playlist::eventFilter (this=0x8211778, o=0x8212008, e=0xbfe652c0) at playlist.cpp:2937
#11 0xb65ea38f in QObject::activate_filters () from /usr/lib/qt/lib/libqt-mt.so.3
#12 0xb65ea464 in QObject::event () from /usr/lib/qt/lib/libqt-mt.so.3
#13 0xb66267df in QWidget::event () from /usr/lib/qt/lib/libqt-mt.so.3
#14 0xb658846f in QApplication::internalNotify () from /usr/lib/qt/lib/libqt-mt.so.3
#15 0xb65886c6 in QApplication::notify () from /usr/lib/qt/lib/libqt-mt.so.3
#16 0xb6bd3ac5 in KApplication::notify () from /opt/kde/lib/libkdecore.so.4
#17 0xb652239b in QETWidget::translateMouseEvent () from /usr/lib/qt/lib/libqt-mt.so.3
#18 0xb6520821 in QApplication::x11ProcessEvent () from /usr/lib/qt/lib/libqt-mt.so.3
#19 0xb6533f25 in QEventLoop::processEvents () from /usr/lib/qt/lib/libqt-mt.so.3
#20 0xb659eb91 in QEventLoop::enterLoop () from /usr/lib/qt/lib/libqt-mt.so.3
#21 0xb659eae6 in QEventLoop::exec () from /usr/lib/qt/lib/libqt-mt.so.3
#22 0xb65875cf in QApplication::exec () from /usr/lib/qt/lib/libqt-mt.so.3
#23 0x0804cf62 in main (argc=1, argv=0xbfe65994) at main.cpp:114
Comment 2 Mark Kretschmann 2007-05-07 21:24:18 UTC
*** Bug 145157 has been marked as a duplicate of this bug. ***
Comment 3 Mark Kretschmann 2007-05-07 21:25:15 UTC
Hmm, is this still valid?
Comment 4 Jeff Mitchell 2007-05-07 23:17:09 UTC
I absolutely can't duplicate this.  I've tried to do it in many ways.  The top track in the playlist plays for me.  Question: did you have the number of played tracks to show set to zero?
Comment 5 Jeff Mitchell 2007-05-08 00:45:43 UTC
I believe this to be a duplicate of bug 145157 which has been fixed.  Please update to at least r662353 and try again -- if you still experience the crash, give very detailed specifics as to how.

*** This bug has been marked as a duplicate of 145157 ***
Comment 6 Thomas Lindroth 2007-05-08 08:03:32 UTC
This crash still exists in rev 662428.

This is what I do. Load a dynamic playlist. It works for every dynamic playlist I tried. Play a few tracks so you get a history of grey tracks at the top. Sort the playlist 2 times on some column to make the gray tracks move to the bottom. Play the top track to crash.

This might not be very detailed but it's all I need to do.

There are also some other ways to make it crash but they are not 100% sure to result in a crash. If you sort the playlist just once so the grey tracks are still at the top you might get a crash but sometimes you don't. If you sort the playlist 3 times so the gray tracks move to the bottom and then up again you might be able to crash it when you play any of the gray track. Even if it don't crash weird things will happen. It might forget about the gray tracks and turn them back into black tracks, run fine for a while and then crash.

If you still can't duplicate I'll make some backtraces tonight.
Comment 7 richlv 2007-05-08 17:14:50 UTC
still reproducible in revision 662493

backtrace is different now, though :

(gdb) bt
#0  0x083b0f43 in ?? ()
#1  0xb7cdb421 in ~UrlLoader (this=0x93d4d80) at playlistloader.cpp:183
#2  0xb64ce212 in QApplication::sendPostedEvents () from /usr/lib/qt/lib/libqt-mt.so.3
#3  0xb64ce384 in QApplication::sendPostedEvents () from /usr/lib/qt/lib/libqt-mt.so.3
#4  0xb6478ea6 in QEventLoop::processEvents () from /usr/lib/qt/lib/libqt-mt.so.3
#5  0xb64e3961 in QEventLoop::enterLoop () from /usr/lib/qt/lib/libqt-mt.so.3
#6  0xb64e38b6 in QEventLoop::exec () from /usr/lib/qt/lib/libqt-mt.so.3
#7  0xb64cc39f in QApplication::exec () from /usr/lib/qt/lib/libqt-mt.so.3
#8  0x0804cf62 in main (argc=1, argv=0xbfe999d4) at main.cpp:114
Comment 8 Jeff Mitchell 2007-05-09 01:33:37 UTC
reopening, was told how to replicate
Comment 9 Jeff Mitchell 2007-05-09 01:33:56 UTC
SVN commit 662722 by mitchell:

Please do some testing with this code and tell me if there are some updates.  Note that you can only sort in ascending order now 
-- if possible I'll allow descending but at the moment it's impossible with Qt, might have to seriously hack it to do so.  But I 
don't think it crashes anymore.

CCBUG: 144121


 M  +26 -6     playlist.cpp  
 M  +1 -0      playlist.h  


--- branches/stable/extragear/multimedia/amarok/src/playlist.cpp #662721:662722
@@ -189,6 +189,7 @@
         , m_dynamicDirt( false )
         , m_queueDirt( false )
         , m_undoDirt( false )
+        , m_insertFromADT( false )
         , m_itemToReallyCenter( 0 )
         , m_renameItem( 0 )
         , m_lockStack( 0 )
@@ -1153,6 +1154,7 @@
 Playlist::advanceDynamicTrack()
 {
     int x = currentTrackIndex();
+    bool didDelete = false;
     if( dynamicMode()->cycleTracks() )
     {
         if( x >= dynamicMode()->previousCount() )
@@ -1160,6 +1162,7 @@
             PlaylistItem *first = firstChild();
             removeItem( first );
             delete first;
+            didDelete = true;
         }
     }
 
@@ -1167,12 +1170,14 @@
 
     // Just starting to play from stopped, don't append something needlessely
     // or, we have more than enough items in the queue.
-    bool dontAppend = ( EngineController::instance()->engine()->state() == Engine::Empty ) ||
-                        upcomingTracks > dynamicMode()->upcomingCount();
+    bool dontAppend = !didDelete &&
+                        ( ( EngineController::instance()->engine()->state() == Engine::Empty ) ||
+                        upcomingTracks > dynamicMode()->upcomingCount() );
 
     //keep upcomingTracks requirement, this seems to break StopAfterCurrent
     if( !dontAppend && stopAfterMode() != StopAfterCurrent )
     {
+        m_insertFromADT = true;
         addDynamicModeTracks( 1 );
     }
     m_dynamicDirt = true;
@@ -1395,7 +1400,6 @@
             item->moveItem( last );
         last = item;
     }
-
 }
 
 void Playlist::setStopAfterCurrent( bool on )
@@ -2174,7 +2178,16 @@
 {
     saveUndoState();
 
-    KListView::setSorting( col, b );
+    //HACK dynamic mode can't deal with disabled items at the bottom...
+    //so only allow ascending sort in dynamic mode
+
+    if( dynamicMode() )
+    {
+        if( b )
+            KListView::setSorting( col, b );
+    }
+    else
+        KListView::setSorting( col, b );
 }
 
 void
@@ -2997,8 +3010,15 @@
                 if( prev && dynamicMode() )
                     prev->setDynamicEnabled( false );
 
-                activate( after );
-                if ( dynamicMode() && dynamicMode()->cycleTracks() )
+                if( m_insertFromADT )
+                {
+                    if( EngineController::engine()->state() == Engine::Playing )
+                        activate( after );
+                    m_insertFromADT = false;
+                }
+                else
+                    activate( after );
+                if( dynamicMode() && dynamicMode()->cycleTracks() )
                     adjustDynamicPrevious( dynamicMode()->previousCount() );
             }
         }
--- branches/stable/extragear/multimedia/amarok/src/playlist.h #662721:662722
@@ -400,6 +400,7 @@
         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
         bool          m_undoDirt;           //Make sure we don't repopulate the playlist when dynamic mode and undo()
+        bool          m_insertFromADT;      //Don't automatically start playing if a user hits Next in dynamic mode when not already playing
 
         QListViewItem *m_itemToReallyCenter;
         QListViewItem *m_renameItem;
Comment 10 Thomas Lindroth 2007-05-09 07:29:14 UTC
I did some testing and it's still possible to crash it. 
1. load a dynamic playlist
2. play a track in the playlist
3. sort on something (grey tracks still at the top)
4. play the top track
5. IF not crash THEM GOTO 2

My proposed fix is to disable all sorting when a dynamic playlist is loaded.
Comment 11 Jeff Mitchell 2007-05-09 16:59:29 UTC
There are reasons we'd like to allow sorting in dynamic mode but do to some various issues, including Qt ones, it's very difficult to get working right now.  So I think I'll agree with you and disable it.
Comment 12 Jeff Mitchell 2007-05-09 17:00:46 UTC
SVN commit 662910 by mitchell:

Dynamic mode fixes:
Make sure that double-clicking on a track when the engine is stopped moves it to the correct place when it starts playing
Disable sorting altogether.  Maybe revisit it when I have the time or inclination, but it's really troublesome, especially 
descending sort.
CCBUG: 144121


 M  +6 -10     playlist.cpp  
 M  +2 -1      playlistloader.cpp  


--- branches/stable/extragear/multimedia/amarok/src/playlist.cpp #662909:662910
@@ -1612,7 +1612,7 @@
         return;
     }
 
-    if( dynamicMode() && !m_dynamicDirt && !Amarok::repeatTrack() )
+    if( dynamicMode() && !Amarok::repeatTrack() )
     {
         if( m_currentTrack && item->isDynamicEnabled() )
         {
@@ -1645,7 +1645,7 @@
             }
 
         }
-        if( m_currentTrack && m_currentTrack != item )
+        if( !m_dynamicDirt && m_currentTrack && m_currentTrack != item )
         {
             m_currentTrack->setDynamicEnabled( false );
             advanceDynamicTrack();
@@ -2178,15 +2178,11 @@
 {
     saveUndoState();
 
-    //HACK dynamic mode can't deal with disabled items at the bottom...
-    //so only allow ascending sort in dynamic mode
+    //HACK There are reasons to allow sorting in dynamic mode, but
+    //it breaks other things that I don't have the time or patience
+    //to figure out...at least right now
 
-    if( dynamicMode() )
-    {
-        if( b )
-            KListView::setSorting( col, b );
-    }
-    else
+    if( !dynamicMode() )
         KListView::setSorting( col, b );
 }
 
--- branches/stable/extragear/multimedia/amarok/src/playlistloader.cpp #662909:662910
@@ -180,7 +180,8 @@
     if( Playlist::instance() )
     {
         Playlist::instance()->unlock();
-        delete m_markerListViewItem;
+        if( m_markerListViewItem )
+           delete m_markerListViewItem;
     }
 
     delete m_xmlSource;
Comment 13 Jeff Mitchell 2007-05-09 18:34:53 UTC
That should have generally fixed it.  Apparently clicking around really rapidly can cause an issue with a similar backtrace...but I'm pretty sure it's a different issue :-)