extragear/multimedia/amarok/src/playlist
Soren Harward
stharward at gmail.com
Wed Oct 8 21:00:30 CEST 2008
SVN commit 869326 by stharward:
Fixed various "crash on remove tracks" bugs in PlaylistModel
Damn, that one took a while to figure out what was going on. The
problem was that my clever insertRows/removeRows strategy for the
PlaylistModel was screwing up the internal state of the GroupingProxy.
The ListView holds PersistentModelIndexes to various parts of the
GroupingProxy, and when the GroupingProxy and the PlaylistModel got
unsychronized, Amarok would crash pretty soon thereafter.
So I reworked the insertRows and removeRows approach, and cleaned up a
few problems in other places.
CCMAIL: amarok-devel at kde.org
M +4 -2 GroupingProxy.cpp
M +2 -0 PlaylistController.cpp
M +35 -38 PlaylistModel.cpp
M +0 -15 view/listview/PrettyItemDelegate.cpp
M +4 -1 view/listview/PrettyListView.cpp
--- trunk/extragear/multimedia/amarok/src/playlist/GroupingProxy.cpp #869325:869326
@@ -236,21 +236,23 @@
void
Playlist::GroupingProxy::modelRowsInserted( const QModelIndex& idx, int start, int end )
{
+ beginInsertRows( idx, start, end );
for ( int i = start; i <= end; i++ )
{
m_rowGroupMode.insert( i, None );
}
- emit rowsInserted( mapToSource( idx ), start, end );
+ endInsertRows();
}
void
Playlist::GroupingProxy::modelRowsRemoved( const QModelIndex& idx, int start, int end )
{
+ beginRemoveRows( idx, start, end );
for ( int i = start; i <= end; i++ )
{
m_rowGroupMode.removeAt( start );
}
- emit rowsRemoved( mapToSource( idx ), start, end );
+ endRemoveRows();
}
void
--- trunk/extragear/multimedia/amarok/src/playlist/PlaylistController.cpp #869325:869326
@@ -331,6 +331,8 @@
if ( from.size() <= 0 )
return;
+ to = qBound(0, to, m_model->rowCount() - 1);
+
int min = to;
int max = to;
foreach( int i, from )
--- trunk/extragear/multimedia/amarok/src/playlist/PlaylistModel.cpp #869325:869326
@@ -519,9 +519,9 @@
}
// actually do the insertion
- beginInsertRows( QModelIndex(), min, min + cmds.size() - 1 );
foreach( InsertCmd ic, cmds )
{
+ beginInsertRows( QModelIndex(), ic.second, ic.second );
Meta::TrackPtr track = ic.first;
m_totalLength += track->length();
subscribeTo( track );
@@ -532,8 +532,8 @@
m_items.insert( ic.second, newitem );
m_itemIds.insert( newitem->id(), newitem );
newIds.append( newitem->id() );
+ endInsertRows();
}
- endInsertRows();
emit dataChanged( createIndex( min, 0 ), createIndex( max, columnCount() - 1 ) );
emit insertedIds( newIds );
@@ -570,7 +570,26 @@
}
}
- beginRemoveRows( QModelIndex(), min, min + cmds.size() - 1 );
+ /* This next bit is probably more complicated that you expected it to be.
+ * The reason for the complexity comes from the following:
+ *
+ * 1. Qt's Model/View architecture can handle removal of only consecutive rows
+ * 2. The "remove rows" command from the Controller must handle
+ * non-consecutive rows, and the removal command probably isn't sorted
+ *
+ * So each item has to be removed individually, and you can't just iterate
+ * over the commands, calling "m_items.removeAt(index)" as you go, because
+ * the indices of m_items will change with every removeAt(). Thus the
+ * following strategy of copying m_item, and removing the items from the
+ * copy, and then replacing m_items with the newly modified list.
+ *
+ * As a safety measure, the items themselves are not deleted until after m_items
+ * has been replaced. If you delete as you go, then m_items will be holding
+ * dangling pointers, and the program will probably crash if the model is
+ * accessed in this state. -- stharward */
+
+ QList<Item*> newlist(m_items); // copy the current item list
+ QList<Item*> delitems;
foreach( RemoveCmd rc, cmds )
{
Meta::TrackPtr track = rc.first;
@@ -579,41 +598,25 @@
if ( track->album() )
unsubscribeFrom( track->album() );
- Item* delitem = m_items.at( rc.second );
- delIds.append( delitem->id() );
- m_itemIds.remove( delitem->id() );
- delete delitem;
- m_items[rc.second] = 0;
+ Item* item = m_items.at(rc.second);
+ int idx = newlist.indexOf(item);
+ if (idx != -1) {
+ beginRemoveRows(QModelIndex(), idx, idx);
+ delitems.append(newlist.takeAt(idx));
+ endRemoveRows();
+ } else {
+ error() << "tried to delete a non-existent item:" << rc.first->prettyName() << rc.second;
+ }
}
+ m_items = newlist;
+ qDeleteAll(delitems);
+ delitems.clear();
- QMutableListIterator<Item*> i( m_items );
- while ( i.hasNext() )
- {
- i.next();
- if ( i.value() == 0 )
- i.remove();
- }
- endRemoveRows();
if ( m_items.size() > 0 )
{
max = ( max < m_items.size() ) ? max : m_items.size() - 1;
emit dataChanged( createIndex( min, 0 ), createIndex( max, columnCount() ) );
}
- else
- {
-
- /* As of version 4.4.2, Qt's ItemViews are really unhappy when all rows
- * are removed from the model, even if you call beginRemoveRows and
- * endRemoveRows properly (like above). The Views and SelectionModel
- * try to access invalid indexes and the whole program crashes pretty
- * soon thereafter. Resetting the model after all rows are removed
- * works around this problem. I'm filing a bug about this with
- * TrollTech, so hopefully the workaround won't be needed in the
- * future. -- stharward */
-
- debug() << "empty model; calling reset";
- reset();
- }
emit removedIds( delIds );
//update the active row
@@ -647,12 +650,6 @@
max = qMax( max, rc.first );
max = qMax( max, rc.second );
debug() << "moving" << rc.first << "to" << rc.second;
- //FIXME
- if ( rc.second < 0 )
- {
- debug() << "FIXME: Target out of range. Aborting.";
- return;
- }
}
int newActiveRow = m_activeRow;
@@ -676,7 +673,7 @@
}
}
m_activeRow = newActiveRow;
- emit dataChanged( createIndex( min, 0 ), createIndex( max, columnCount() ) );
+ emit dataChanged( createIndex( min, 0 ), createIndex( max, columnCount() - 1 ) );
//update the active row
}
--- trunk/extragear/multimedia/amarok/src/playlist/view/listview/PrettyItemDelegate.cpp #869325:869326
@@ -76,15 +76,6 @@
QSize
Playlist::PrettyItemDelegate::sizeHint( const QStyleOptionViewItem&, const QModelIndex& index ) const
{
-
- /* Qt's ItemViews are horrible about calling sizeHint() and paint() for
- * invalid indexes when the model has been reset. Ideally, this safety
- * check shouldn't be necessary, but without it this function will crash
- * when handed an invalid index. -- stharward */
-
- if ( !Model::instance()->rowExists( index.row() ) )
- return QSize();
-
int height = 0;
int groupMode = index.data( GroupRole ).toInt();
@@ -111,12 +102,6 @@
void
Playlist::PrettyItemDelegate::paint( QPainter* painter, const QStyleOptionViewItem& option, const QModelIndex& index ) const
{
-
- /* see note in sizeHint() about this safety check */
-
- if ( !Model::instance()->rowExists( index.row() ) )
- return;
-
painter->save();
QApplication::style()->drawPrimitive( QStyle::PE_PanelItemViewItem, &option, painter );
painter->translate( option.rect.topLeft() );
--- trunk/extragear/multimedia/amarok/src/playlist/view/listview/PrettyListView.cpp #869325:869326
@@ -125,11 +125,13 @@
DEBUG_BLOCK
if ( dynamic_cast<PrettyListView*>( event->source() ) == this )
{
+ QAbstractItemModel* plModel = model();
int targetRow = indexAt( event->pos() ).row();
+ targetRow = ( targetRow < 0 ) ? plModel->rowCount() - 1 : targetRow; // target of < 0 means we dropped on the end of the playlist
QList<int> sr = selectedRows();
Controller::instance()->moveRows( sr, targetRow );
+ // FIXME: this doesn't work when stuff is dragged to end of the playlist
QItemSelection selItems;
- QAbstractItemModel* plModel = model();
foreach( int row, sr )
{
Q_UNUSED( row )
@@ -156,6 +158,7 @@
else if ( event->key() == Qt::Key_Return )
{
trackActivated( currentIndex() );
+ event->accept();
}
else if ( event->matches( QKeySequence::SelectAll ) )
{
More information about the Amarok-devel
mailing list