Commit d3982dc6 authored by Matěj Laitl's avatar Matěj Laitl
Browse files

Fix Saved Playlists and Podcasts actions

After 2 failed attempts to solve this non-intrusively, I realized the
obvious: the actions belong to the view, not the model! (Qt has no
Model/View/Controller, it has Model/View+Controller)

After this it was "easy", just a fair bit of work to move relevant
actions to PlaylistBrowserModel and ensure they work correctly. Some
playlist (and podcast) actions weren't really appropriate for
PlaylistBrowserModel. Methods to get actions from these needed to be
changed, because the old way simply didn't allow reliable action
execution.

When I was at it, I cleaned up a fair bit of things in podcasts (and
playlist providers) - I haven't seen such code bloat in a while -
100-line-long unused function isn't anything surprising there. I'm
pointing at you, Bart. :-)

This regression exists since Ralf's cleanups, although Ralf certainly
didn't introduce it - he just broke the delicate equilibrium of hacks
that kept it working. (and I still think it was working just by chance)

This doesn't go into ChangeLog as it was introduced post-2.7. The code
is not perfect yet (we have lost "Create Empty Playlist" if you clink
into empty space), but the bug this fixes is much more critical.

BUG: 318782
FIXED-IN: 2.8
CCMAIL: amarok-devel@kde.org
parent 73742929
......@@ -41,29 +41,6 @@ lessThanPlaylistTitles( const Playlists::PlaylistPtr &lhs, const Playlists::Play
PlaylistBrowserModel::PlaylistBrowserModel( int playlistCategory )
: m_playlistCategory( playlistCategory )
{
m_createEmptyPlaylistAction = new QAction( KIcon( "media-track-add-amarok" ),
i18n( "Create empty playlist" ),
this );
connect( m_createEmptyPlaylistAction, SIGNAL(triggered()), SLOT(slotCreateEmptyPlaylist()) );
//common, unconditional actions
m_appendAction = new QAction( KIcon( "media-track-add-amarok" ), i18n( "&Add to Playlist" ),
this );
// object name must match one in PlaylistBrowserNS::PlaylistBrowserView
m_appendAction->setObjectName( "appendAction" );
// key shortcut is only for display purposes here, actual one is determined by View in Model/View classes
m_appendAction->setShortcut( Qt::Key_Enter );
m_appendAction->setProperty( "popupdropper_svg_id", "append" );
connect( m_appendAction, SIGNAL(triggered()), this, SLOT(slotAppend()) );
m_loadAction = new QAction( KIcon( "folder-open" ),
i18nc( "Replace the currently loaded tracks with these",
"&Replace Playlist" ),
this );
m_loadAction->setObjectName( "loadAction" );
m_loadAction->setProperty( "popupdropper_svg_id", "load" );
connect( m_loadAction, SIGNAL(triggered()), this, SLOT(slotLoad()) );
connect( The::playlistManager(), SIGNAL(updated(int)), SLOT(slotUpdate(int)) );
connect( The::playlistManager(), SIGNAL(playlistAdded(Playlists::PlaylistPtr,int)),
......@@ -93,6 +70,7 @@ PlaylistBrowserModel::data( const QModelIndex &index, int role ) const
QList<Playlists::PlaylistProvider *> providers =
The::playlistManager()->getProvidersForPlaylist( playlist );
Playlists::PlaylistProvider *provider = providers.count() == 1 ? providers.first() : 0;
Meta::TrackPtr track;
switch( index.column() )
{
......@@ -100,7 +78,7 @@ PlaylistBrowserModel::data( const QModelIndex &index, int role ) const
{
if( IS_TRACK(index) )
{
Meta::TrackPtr track = playlist->tracks()[index.row()];
track = playlist->tracks()[index.row()];
name = track->prettyName();
icon = KIcon( "amarok_track" );
}
......@@ -190,17 +168,17 @@ PlaylistBrowserModel::data( const QModelIndex &index, int role ) const
if( IS_TRACK(index) )
return QVariant();
else
return i18ncp( "number of playlists from one source",
"One playlist", "%1 playlists",
playlistCount );
case PrettyTreeRoles::DecoratorRole:
return QVariant::fromValue( actionsFor( index ) );
case PrettyTreeRoles::DecoratorRoleCount:
return actionsFor( index ).count();
return i18ncp( "number of playlists from one source", "One playlist",
"%1 playlists", playlistCount );
case PlaylistBrowserModel::ProviderRole:
return provider ? QVariant::fromValue<Playlists::PlaylistProvider *>( provider ) : QVariant();
default: return QVariant();
return provider ? QVariant::fromValue( provider ) : QVariant();
case PlaylistBrowserModel::PlaylistRole:
return playlist ? QVariant::fromValue( playlist ) : QVariant();
case PlaylistBrowserModel::TrackRole:
return track ? QVariant::fromValue( track ) : QVariant();
default:
return QVariant();
}
}
......@@ -617,34 +595,6 @@ PlaylistBrowserModel::loadPlaylists()
return playlists;
}
void
PlaylistBrowserModel::slotLoad()
{
QAction *action = qobject_cast<QAction *>( QObject::sender() );
if( action == 0 )
return;
QModelIndexList indexes = action->data().value<QModelIndexList>();
Meta::TrackList tracks = tracksFromIndexes( indexes );
if( !tracks.isEmpty() )
The::playlistController()->insertOptioned( tracks, Playlist::LoadAndPlay );
}
void
PlaylistBrowserModel::slotAppend()
{
QAction *action = qobject_cast<QAction *>( QObject::sender() );
if( action == 0 )
return;
QModelIndexList indexes = action->data().value<QModelIndexList>();
Meta::TrackList tracks = tracksFromIndexes( indexes );
if( !tracks.isEmpty() )
The::playlistController()->insertOptioned( tracks, Playlist::AppendAndPlay );
}
void
PlaylistBrowserModel::slotPlaylistAdded( Playlists::PlaylistPtr playlist, int category )
{
......@@ -709,13 +659,6 @@ PlaylistBrowserModel::slotPlaylistUpdated( Playlists::PlaylistPtr playlist, int
endInsertRows();
}
void
PlaylistBrowserModel::slotCreateEmptyPlaylist()
{
The::playlistManager()->save( Meta::TrackList(),
Amarok::generatePlaylistName( Meta::TrackList() ) );
}
Meta::TrackList
PlaylistBrowserModel::tracksFromIndexes( const QModelIndexList &list ) const
{
......@@ -779,40 +722,6 @@ PlaylistBrowserModel::providerForIndex( const QModelIndex &idx ) const
return m_playlists.at( playlistRow )->provider();
}
QActionList
PlaylistBrowserModel::actionsFor( const QModelIndex &idx ) const
{
if( !idx.isValid() )
{
QActionList emptyActions;
emptyActions << m_createEmptyPlaylistAction;
return emptyActions;
}
//whether we use the list from m_appendAction of m_loadAction does not matter they are the same
QModelIndexList actionList = m_appendAction->data().value<QModelIndexList>();
actionList << idx;
QVariant value = QVariant::fromValue( actionList );
m_appendAction->setData( value );
m_loadAction->setData( value );
QActionList actions;
actions << m_appendAction << m_loadAction;
if( !IS_TRACK(idx) )
{
Playlists::PlaylistPtr playlist = m_playlists.value( idx.internalId() );
actions << playlist->actions();
}
else
{
Playlists::PlaylistPtr playlist = m_playlists.value( idx.parent().internalId() );
actions << playlist->trackActions( idx.row() );
}
return actions;
}
Playlists::PlaylistProvider *
PlaylistBrowserModel::getProviderByName( const QString &name )
{
......
......@@ -39,8 +39,7 @@ namespace PlaylistBrowserNS {
/**
@author Bart Cerneels <bart.cerneels@kde.org>
*/
class PlaylistBrowserModel : public QAbstractItemModel,
public Playlists::PlaylistObserver
class PlaylistBrowserModel : public QAbstractItemModel, public Playlists::PlaylistObserver
{
Q_OBJECT
public:
......@@ -53,8 +52,11 @@ class PlaylistBrowserModel : public QAbstractItemModel,
enum
{
ProviderRole = Qt::UserRole + 21, // pointer to associated PlaylistProvider
CustomRoleOffset = Qt::UserRole + 22 //first role that can be used by sublasses for their own data
ProviderRole = Qt::UserRole + 21, // pointer to associated PlaylistProvider
PlaylistRole = Qt::UserRole + 22, // PlaylistPtr for associated playlist or null
TrackRole = Qt::UserRole + 23, // TrackPtr for associated track or null
EpisodeIsNewRole = Qt::UserRole + 24, // for podcast episodes, supports setting, type: bool
CustomRoleOffset = Qt::UserRole + 25 //first role that can be used by sublasses for their own data
};
PlaylistBrowserModel( int PlaylistCategory );
......@@ -97,7 +99,6 @@ class PlaylistBrowserModel : public QAbstractItemModel,
protected:
virtual Playlists::PlaylistList loadPlaylists();
virtual QActionList actionsFor( const QModelIndex &idx ) const;
Meta::TrackList tracksFromIndexes( const QModelIndexList &list ) const;
Meta::TrackPtr trackFromIndex( const QModelIndex &index ) const;
......@@ -110,23 +111,14 @@ class PlaylistBrowserModel : public QAbstractItemModel,
Playlists::PlaylistProvider *getProviderByName( const QString &name );
private slots:
void slotLoad();
void slotAppend();
void slotPlaylistAdded( Playlists::PlaylistPtr playlist, int category );
void slotPlaylistRemoved( Playlists::PlaylistPtr playlist, int category );
void slotPlaylistUpdated( Playlists::PlaylistPtr playlist, int category );
void slotCreateEmptyPlaylist();
private:
int m_playlistCategory;
QAction *m_appendAction;
QAction *m_loadAction;
QAction *m_createEmptyPlaylistAction;
};
}
//we store these in a QVariant for the load and append actions
Q_DECLARE_METATYPE( QModelIndexList )
#endif //AMAROK_PLAYLISTBROWSERMODEL_H
......@@ -18,12 +18,12 @@
#ifndef PLAYLISTBROWSERVIEW_H
#define PLAYLISTBROWSERVIEW_H
#include "core/playlists/Playlist.h"
#include "widgets/PrettyTreeView.h"
#include <QMutex>
class PopupDropper;
class KAction;
class QKeyEvent;
class QMouseEvent;
class QContextMenuEvent;
......@@ -35,54 +35,69 @@ class PlaylistBrowserView : public Amarok::PrettyTreeView
Q_OBJECT
public:
explicit PlaylistBrowserView( QAbstractItemModel *model, QWidget *parent = 0 );
~PlaylistBrowserView();
virtual void setModel( QAbstractItemModel *model );
void setNewFolderAction( KAction *action );
signals:
void currentItemChanged( const QModelIndex &current );
protected:
//TODO:re-implement QWidget::dragEnterEvent() to show drop-not-allowed indicator
// TODO: re-implement QWidget::dragEnterEvent() to show drop-not-allowed indicator
virtual void keyPressEvent( QKeyEvent *event );
virtual void mouseDoubleClickEvent( QMouseEvent *event );
virtual void mouseReleaseEvent( QMouseEvent *event );
virtual void startDrag( Qt::DropActions supportedActions );
virtual void contextMenuEvent( QContextMenuEvent* event );
virtual void contextMenuEvent( QContextMenuEvent *event );
protected slots:
/** reimplemented to emit a signal */
void currentChanged( const QModelIndex &current, const QModelIndex &previous );
private slots:
// these are connected to m_*Actions:
void slotCreateEmptyPlaylist();
void slotAppend();
void slotLoad();
void slotSetNew( bool newState );
void slotRename();
void slotDelete();
void slotRemoveTracks();
void slotExport();
private:
/**
* Execute all actions that are named appendAction
*/
void appendAndPlay( const QModelIndex &index );
void appendAndPlay( const QModelIndexList &list );
void insertToPlayQueue( int options );
/**
* Execute all actions that are named deleteAction
*/
void deletePlaylistsTracks( const QModelIndexList &list );
/**
* Executes all actions that are named @param name
* Gets action for a list of indices and sets internal action targets to these.
*
* After you have processed/triggered the actions, you should call
* resetActionTargets() to prevent stale targets laying around.
*/
void performActionNamed( const QString &name, const QModelIndexList &list );
QAction *decoratorActionAt( const QModelIndex &idx, const QPoint position );
QList<QAction *> actionsFor( QModelIndexList indexes );
QList<QAction *> actionsFor( const QModelIndexList &indexes );
void resetActionTargets();
PopupDropper* m_pd;
KAction *m_addFolderAction;
QAction *m_createEmptyPlaylistAction;
QAction *m_appendAction;
QAction *m_loadAction;
QAction *m_setNewAction; // for podcasts
QAction *m_renamePlaylistAction;
QAction *m_deletePlaylistAction;
QAction *m_removeTracksAction;
QAction *m_exportAction;
QAction *m_separatorAction;
bool m_ongoingDrag;
Playlists::PlaylistProvider *m_writableActionProvider;
Playlists::PlaylistList m_actionPlaylists;
Playlists::PlaylistList m_writableActionPlaylists;
QMultiHash<Playlists::PlaylistPtr, int> m_actionTracks; // maps playlists to track positions
QMultiHash<Playlists::PlaylistPtr, int> m_writableActionTracks;
};
} // namespace PlaylistBrowserNS
......
......@@ -66,17 +66,8 @@ PlaylistBrowserNS::PodcastModel::destroy()
PlaylistBrowserNS::PodcastModel::PodcastModel()
: PlaylistBrowserModel( PlaylistManager::PodcastChannel )
, m_setNewAction( 0 )
{
s_instance = this;
m_setNewAction = new QAction( KIcon( "rating" ),
i18nc( "toggle the \"new\" status of this podcast episode",
"&New" ),
this
);
m_setNewAction->setProperty( "popupdropper_svg_id", "new" );
m_setNewAction->setCheckable( true );
connect( m_setNewAction, SIGNAL(triggered(bool)), SLOT(slotSetNew(bool)) );
}
bool
......@@ -267,6 +258,8 @@ PlaylistBrowserNS::PodcastModel::episodeData( const PodcastEpisodePtr &episode,
if( idx.column() == PlaylistBrowserModel::PlaylistItemColumn )
return icon( episode );
break;
case EpisodeIsNewRole:
return episode->isNew();
}
return PlaylistBrowserModel::data( idx, role );
......@@ -275,10 +268,18 @@ PlaylistBrowserNS::PodcastModel::episodeData( const PodcastEpisodePtr &episode,
bool
PlaylistBrowserNS::PodcastModel::setData( const QModelIndex &idx, const QVariant &value, int role )
{
DEBUG_BLOCK
PodcastEpisodePtr episode = episodeForIndex( idx );
if( !episode || !value.canConvert<bool>() || role != EpisodeIsNewRole )
{
return PlaylistBrowserModel::setData( idx, value, role );
}
//TODO: implement setNew.
return PlaylistBrowserModel::setData( idx, value, role );
bool checked = value.toBool();
episode->setNew( checked );
if( checked )
emit episodeMarkedAsNew( episode );
emit dataChanged( idx, idx );
return true;
}
int
......@@ -353,62 +354,6 @@ PlaylistBrowserNS::PodcastModel::refreshPodcasts()
}
}
QActionList
PlaylistBrowserNS::PodcastModel::actionsFor( const QModelIndex &idx ) const
{
if( !idx.isValid() )
{
//TODO: add podcast action
return QActionList();
}
QActionList actions = PlaylistBrowserModel::actionsFor( idx );
/* by default a list of podcast episodes can only be changed to isNew = false or
isKeep = false, except when all selected episodes are the same state */
m_setNewAction->setChecked( false );
Podcasts::PodcastEpisodeList episodes = m_setNewAction->data().value<Podcasts::PodcastEpisodeList>();
if( IS_TRACK(idx) )
episodes << episodeForIndex( idx );
else
episodes << channelForIndex( idx )->episodes();
foreach( const Podcasts::PodcastEpisodePtr episode, episodes )
{
if( episode->isNew() )
m_setNewAction->setChecked( true );
}
m_setNewAction->setData( QVariant::fromValue( episodes ) );
actions << m_setNewAction;
return actions;
}
void
PlaylistBrowserNS::PodcastModel::slotSetNew( bool newState )
{
Q_UNUSED( newState );
QAction *action = qobject_cast<QAction *>( QObject::sender() );
if( action == 0 )
return;
Podcasts::PodcastEpisodeList episodes = action->data().value<Podcasts::PodcastEpisodeList>();
foreach( Podcasts::PodcastEpisodePtr episode, episodes )
{
if( !episode.isNull() )
{
episode->setNew( action->isChecked() );
if( action->isChecked() )
emit episodeMarkedAsNew( episode );
}
}
}
Podcasts::PodcastChannelPtr
PlaylistBrowserNS::PodcastModel::channelForIndex( const QModelIndex &idx ) const
{
......
......@@ -50,6 +50,7 @@ enum
class PodcastModel : public PlaylistBrowserModel
{
Q_OBJECT
public:
static PodcastModel *instance();
static void destroy();
......@@ -77,14 +78,8 @@ class PodcastModel : public PlaylistBrowserModel
void addPodcast();
void refreshPodcasts();
private slots:
void slotSetNew( bool newState );
protected:
virtual QActionList actionsFor( const QModelIndex &idx ) const;
private:
static PodcastModel* s_instance;
static PodcastModel *s_instance;
PodcastModel();
QVariant channelData( const Podcasts::PodcastChannelPtr &channel,
......@@ -97,8 +92,6 @@ class PodcastModel : public PlaylistBrowserModel
Q_DISABLE_COPY( PodcastModel )
QAction *m_setNewAction;
/**
* A convenience function to convert a PodcastEpisodeList into a TrackList.
*/
......
......@@ -101,45 +101,28 @@ IpodPlaylistProvider::providerActions()
}
QActionList
IpodPlaylistProvider::playlistActions( Playlists::PlaylistPtr playlist )
IpodPlaylistProvider::playlistActions( const Playlists::PlaylistList &playlists )
{
QList<QAction *> actions;
if( !m_playlists.contains( playlist ) ) // make following static cast safe
return actions;
KSharedPtr<IpodPlaylist> ipodPlaylist = KSharedPtr<IpodPlaylist>::staticCast( playlist );
switch( ipodPlaylist->type() )
QActionList actions;
foreach( const Playlists::PlaylistPtr &playlist, playlists )
{
case IpodPlaylist::Normal:
actions << Playlists::UserPlaylistProvider::playlistActions( playlist );
break;
case IpodPlaylist::Stale:
case IpodPlaylist::Orphaned:
if( !m_playlists.contains( playlist ) ) // make following static cast safe
continue;
IpodPlaylist::Type type = KSharedPtr<IpodPlaylist>::staticCast( playlist )->type();
if( type == IpodPlaylist::Stale || type == IpodPlaylist::Orphaned )
{
actions << m_coll->m_consolidateAction;
break;
}
}
return actions;
}
QActionList
IpodPlaylistProvider::trackActions( Playlists::PlaylistPtr playlist, int trackIndex )
IpodPlaylistProvider::trackActions( const QMultiHash<Playlists::PlaylistPtr, int> &playlistTracks )
{
QList<QAction *> actions;
if( !m_playlists.contains( playlist ) ) // make following static cast safe
return actions;
KSharedPtr<IpodPlaylist> ipodPlaylist = KSharedPtr<IpodPlaylist>::staticCast( playlist );
switch( ipodPlaylist->type() )
{
case IpodPlaylist::Normal:
actions << Playlists::UserPlaylistProvider::trackActions( playlist, trackIndex );
break;
case IpodPlaylist::Stale:
case IpodPlaylist::Orphaned:
actions << m_coll->m_consolidateAction;
break;
}
return actions;
return playlistActions( playlistTracks.uniqueKeys() );
}
bool
......@@ -149,7 +132,7 @@ IpodPlaylistProvider::isWritable()
}
void
IpodPlaylistProvider::rename( Playlists::PlaylistPtr playlist, const QString &newName )
IpodPlaylistProvider::renamePlaylist( Playlists::PlaylistPtr playlist, const QString &newName )
{
if( !m_playlists.contains( playlist ) ) // make following static cast safe
return;
......@@ -163,7 +146,7 @@ IpodPlaylistProvider::rename( Playlists::PlaylistPtr playlist, const QString &ne
}
bool
IpodPlaylistProvider::deletePlaylists( Playlists::PlaylistList playlistlist )
IpodPlaylistProvider::deletePlaylists( const Playlists::PlaylistList &playlistlist )
{
if( !isWritable() )
return false;
......
......@@ -49,13 +49,12 @@ class IpodPlaylistProvider : public Playlists::UserPlaylistProvider, private Pla
const QString& name = QString() );
virtual QActionList providerActions();
virtual QActionList playlistActions( Playlists::PlaylistPtr playlist );
virtual QActionList trackActions( Playlists::PlaylistPtr playlist,
int trackIndex );
virtual QActionList playlistActions( const Playlists::PlaylistList &playlists );
virtual QActionList trackActions( const QMultiHash<Playlists::PlaylistPtr, int> &playlistTracks );
virtual bool isWritable();
virtual void rename( Playlists::PlaylistPtr playlist, const QString &newName );
virtual bool deletePlaylists( Playlists::PlaylistList playlistlist );
virtual void renamePlaylist( Playlists::PlaylistPtr playlist, const QString &newName );
virtual bool deletePlaylists( const Playlists::PlaylistList &playlistlist );
// PlaylistObserver methods:
virtual void metadataChanged( Playlists::PlaylistPtr playlist );
......
......@@ -33,7 +33,6 @@
#include <KInputDialog>
#include <KUrl>
#include <QAction>
#include <QMap>
static const int USERPLAYLIST_DB_VERSION = 2;
......@@ -43,7 +42,6 @@ namespace Playlists {
MediaDeviceUserPlaylistProvider::MediaDeviceUserPlaylistProvider( Collections::MediaDeviceCollection *collection )
: Playlists::UserPlaylistProvider()
, m_renameAction( 0 )
, m_collection( collection )
{
DEBUG_BLOCK
......@@ -79,65 +77,7 @@ MediaDeviceUserPlaylistProvider::playlists()
return playlists;
}
#if 0
void
SqlPlaylists::UserPlaylistProvider::slotDelete()
{
DEBUG_BLOCK
//TODO FIXME Confirmation of delete
foreach( Playlists::PlaylistPtr playlist, The::userPlaylistModel()->selectedPlaylists() )
{
Meta::SqlPlaylistPtr sqlPlaylist =
Meta::SqlPlaylistPtr::dynamicCast( playlist );