extragear/multimedia/amarok/src
Maximilian Kossick
maximilian.kossick at googlemail.com
Tue Jun 17 16:50:43 CEST 2008
why do we have to link Meta:.Observer to magnatune album? something is
really broken if that's necessary. Magnatunealbum isn't even in
libamarok, but in the magnatune service instead
On Tue, Jun 17, 2008 at 4:13 PM, Mark Kretschmann <kretschmann at kde.org> wrote:
> SVN commit 821449 by markey:
>
> Refactor Meta::Observer so that we automatically unsubscribe all Meta subscriptions in the dtor. This fixes many issues, e.g. you are now actually able to listen to more than 5 tracks in a row without crashes ;) Also, it makes the code simpler, since we don't have to do manual bookkeeping.
>
> The important API change is that you now subscribe/unsubscribe to Meta objects with subscribeTo(MetaPtr) / unsubscribeTo(MetaPtr); these two methods are built into the Observer base.
>
> This was quite a bit of work, but I think it was worth it :)
>
> CCMAIL: amarok-devel at kde.org
>
> implement subscribeTo/unsubscribeTo
>
>
> M +6 -6 Osd.cpp
> M +4 -21 context/engines/current/CurrentEngine.cpp
> M +2 -2 covermanager/CoverManager.cpp
> M +157 -1 meta/Meta.cpp
> M +38 -12 meta/Meta.h
> M +1 -1 meta/proxy/MetaProxy_p.h
> M +4 -4 playlist/PlaylistModel.cpp
> M +3 -4 servicebrowser/lastfm/meta/MultiPlayableCapabilityImpl_p.h
> M +6 -3 servicebrowser/magnatunestore/MagnatunePurchaseDialog.cpp
> M +6 -7 widgets/TrackTooltip.cpp
>
>
> --- trunk/extragear/multimedia/amarok/src/Osd.cpp #821448:821449
> @@ -600,11 +600,11 @@
> DEBUG_BLOCK
>
> if( m_track && m_track->artist() )
> - m_track->artist()->unsubscribe( this );
> + unsubscribeTo( m_track->artist() );
> if( m_track && m_track->album() )
> - m_track->album()->unsubscribe( this );
> + unsubscribeTo( m_track->album() );
> if( m_track )
> - m_track->unsubscribe( this );
> + unsubscribeTo( m_track );
>
> m_track = track;
>
> @@ -639,11 +639,11 @@
> image = track->album()->image( 100 ).toImage();
>
> if( m_track ) {
> - m_track->subscribe( this );
> + subscribeTo( m_track );
> if( m_track->artist() )
> - m_track->artist()->subscribe( this );
> + subscribeTo( m_track->artist() );
> if( m_track->album() )
> - m_track->album()->subscribe( this );
> + subscribeTo( m_track->album() );
> }
>
> OSDWidget::show( text, image );
> --- trunk/extragear/multimedia/amarok/src/context/engines/current/CurrentEngine.cpp #821448:821449
> @@ -43,14 +43,6 @@
> CurrentEngine::~CurrentEngine()
> {
> DEBUG_BLOCK
> - if( m_currentTrack )
> - {
> - m_currentTrack->unsubscribe( this );
> - if( m_currentTrack->album() )
> - {
> - m_currentTrack->album()->unsubscribe( this );
> - }
> - }
> }
>
> QStringList CurrentEngine::sources() const
> @@ -83,11 +75,9 @@
> if( m_currentTrack )
> {
> debug() << "2";
> - m_currentTrack->unsubscribe( this );
> + unsubscribeTo( m_currentTrack );
> if( m_currentTrack->album() )
> - {
> - m_currentTrack->album()->unsubscribe( this );
> - }
> + unsubscribeTo( m_currentTrack->album() );
> }
> update();
> }
> @@ -110,24 +100,17 @@
> void CurrentEngine::update()
> {
> DEBUG_BLOCK
> -
> - if( m_currentTrack ) {
> - debug() << "Unsubscribing m_currentTrack.";
> - m_currentTrack->unsubscribe( this );
> - if( m_currentTrack->album() )
> - m_currentTrack->album()->unsubscribe( this );
> - }
>
> m_currentTrack = The::engineController()->currentTrack();
> if( !m_currentTrack )
> return;
> - m_currentTrack->subscribe( this );
> + subscribeTo( m_currentTrack );
>
> QVariantMap trackInfo = Meta::Field::mapFromTrack( m_currentTrack.data() );
>
> int width = coverWidth();
> if( m_currentTrack->album() )
> - m_currentTrack->album()->subscribe( this );
> + subscribeTo( m_currentTrack->album() );
> removeAllData( "current" );
> if( m_currentTrack->album() ) {
>
> --- trunk/extragear/multimedia/amarok/src/covermanager/CoverManager.cpp #821448:821449
> @@ -922,7 +922,7 @@
> m_artist = i18n( "No Artist" );
> setText( album->prettyName() );
> setIcon( album->image( 100 ) );
> - album->subscribe( qobject_cast<CoverManager*>(parent->parent()->parent()) );
> + qobject_cast<CoverManager*>(parent->parent()->parent())->subscribeTo( album ); // Man this looks sick
> // setDragEnabled( true );
> // setDropEnabled( true );
> calcRect();
> @@ -930,7 +930,7 @@
>
> CoverViewItem::~CoverViewItem()
> {
> - m_albumPtr->unsubscribe( qobject_cast<CoverManager*>( m_parent->parent()->parent()) );
> + qobject_cast<CoverManager*>(m_parent->parent()->parent())->subscribeTo( m_albumPtr );
> }
> bool CoverViewItem::hasCover() const
> {
> --- trunk/extragear/multimedia/amarok/src/meta/Meta.cpp #821448:821449
> @@ -1,6 +1,7 @@
> /* This file is part of the KDE project
> Copyright (C) 2007 Maximilian Kossick <maximilian.kossick at googlemail.com>
> Copyright (C) 2007 Ian Monroe <ian at monroe.nu>
> + Copyright (C) 2008 Mark Kretschmann <kretschmann at kde.org>
>
> This program is free software; you can redistribute it and/or
> modify it under the terms of the GNU General Public License
> @@ -24,6 +25,7 @@
> #include "Collection.h"
> #include "Debug.h"
> #include "QueryMaker.h"
> +#include "../servicebrowser/magnatunestore/MagnatuneMeta.h"
>
> #include <QDir>
> #include <QImage>
> @@ -34,10 +36,158 @@
>
> Meta::Observer::~Observer()
> {
> - //nothing to do
> + // Unsubscribe all stray Meta subscriptions:
> +
> + foreach( TrackPtr ptr, m_trackSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> + foreach( ArtistPtr ptr, m_artistSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> + foreach( AlbumPtr ptr, m_albumSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> + foreach( GenrePtr ptr, m_genreSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> + foreach( ComposerPtr ptr, m_composerSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> + foreach( YearPtr ptr, m_yearSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> + foreach( MagnatuneAlbum* ptr, m_magnatuneAlbumSubscriptions )
> + if( ptr )
> + ptr->unsubscribe( this );
> }
>
> void
> +Meta::Observer::subscribeTo( TrackPtr ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_trackSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( TrackPtr ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_trackSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::subscribeTo( ArtistPtr ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_artistSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( ArtistPtr ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_artistSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::subscribeTo( AlbumPtr ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_albumSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( AlbumPtr ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_albumSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::subscribeTo( ComposerPtr ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_composerSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( ComposerPtr ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_composerSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::subscribeTo( GenrePtr ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_genreSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( GenrePtr ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_genreSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::subscribeTo( YearPtr ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_yearSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( YearPtr ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_yearSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::subscribeTo( MagnatuneAlbum* ptr )
> +{
> + if( ptr ) {
> + ptr->subscribe( this );
> + m_magnatuneAlbumSubscriptions.append( ptr );
> + }
> +}
> +
> +void
> +Meta::Observer::unsubscribeTo( MagnatuneAlbum* ptr )
> +{
> + if( ptr ) {
> + ptr->unsubscribe( this );
> + m_magnatuneAlbumSubscriptions.remove( ptr );
> + }
> +}
> +
> +void
> Meta::Observer::metadataChanged( Track *track )
> {
> Q_UNUSED( track );
> @@ -144,6 +294,8 @@
> void
> Meta::Track::notifyObservers() const
> {
> + DEBUG_BLOCK
> +
> foreach( Observer *observer, m_observers )
> {
> if( m_observers.contains( observer ) ) // guard against observers removing themselves in destructors
> @@ -174,6 +326,8 @@
> void
> Meta::Artist::notifyObservers() const
> {
> + DEBUG_BLOCK
> +
> foreach( Observer *observer, m_observers )
> {
> if( m_observers.contains( observer ) ) // guard against observers removing themselves in destructors
> @@ -198,6 +352,8 @@
> void
> Meta::Album::notifyObservers() const
> {
> + DEBUG_BLOCK
> +
> foreach( Observer *observer, m_observers )
> {
> if( m_observers.contains( observer ) ) // guard against observers removing themselves in destructors
> --- trunk/extragear/multimedia/amarok/src/meta/Meta.h #821448:821449
> @@ -1,5 +1,6 @@
> /* This file is part of the KDE project
> Copyright (C) 2007 Maximilian Kossick <maximilian.kossick at googlemail.com>
> + Copyright (C) 2008 Mark Kretschmann <kretschmann at kde.org>
>
> This program is free software; you can redistribute it and/or
> modify it under the terms of the GNU General Public License
> @@ -45,27 +46,42 @@
> class Genre;
> class Composer;
> class Year;
> + class MagnatuneAlbum;
>
> -
> typedef KSharedPtr<MetaBase> DataPtr;
> - typedef QList<DataPtr > DataList;
> -
> + typedef QList<DataPtr> DataList;
> typedef KSharedPtr<Track> TrackPtr;
> - typedef QList<TrackPtr > TrackList;
> + typedef QList<TrackPtr> TrackList;
> typedef KSharedPtr<Artist> ArtistPtr;
> - typedef QList<ArtistPtr > ArtistList;
> + typedef QList<ArtistPtr> ArtistList;
> typedef KSharedPtr<Album> AlbumPtr;
> - typedef QList<AlbumPtr > AlbumList;
> + typedef QList<AlbumPtr> AlbumList;
> typedef KSharedPtr<Composer> ComposerPtr;
> typedef QList<ComposerPtr> ComposerList;
> typedef KSharedPtr<Genre> GenrePtr;
> - typedef QList<GenrePtr > GenreList;
> + typedef QList<GenrePtr> GenreList;
> typedef KSharedPtr<Year> YearPtr;
> - typedef QList<YearPtr > YearList;
> + typedef QList<YearPtr> YearList;
> + typedef QList<MagnatuneAlbum*> MagnatuneAlbumList;
>
> class AMAROK_EXPORT Observer
> {
> public:
> + void subscribeTo( TrackPtr );
> + void unsubscribeTo( TrackPtr );
> + void subscribeTo( ArtistPtr );
> + void unsubscribeTo( ArtistPtr );
> + void subscribeTo( AlbumPtr );
> + void unsubscribeTo( AlbumPtr );
> + void subscribeTo( ComposerPtr );
> + void unsubscribeTo( ComposerPtr );
> + void subscribeTo( GenrePtr );
> + void unsubscribeTo( GenrePtr );
> + void subscribeTo( YearPtr );
> + void unsubscribeTo( YearPtr );
> + void subscribeTo( MagnatuneAlbum* );
> + void unsubscribeTo( MagnatuneAlbum* );
> +
> /** This method is called when the metadata of a track has changed.
> The called class may not cache the pointer */
> virtual void metadataChanged( Track *track );
> @@ -75,10 +91,21 @@
> virtual void metadataChanged( Composer *composer );
> virtual void metadataChanged( Year *year );
> virtual ~Observer();
> +
> + private:
> + TrackList m_trackSubscriptions;
> + ArtistList m_artistSubscriptions;
> + AlbumList m_albumSubscriptions;
> + ComposerList m_composerSubscriptions;
> + GenreList m_genreSubscriptions;
> + YearList m_yearSubscriptions;
> + MagnatuneAlbumList m_magnatuneAlbumSubscriptions;
> };
>
> class AMAROK_EXPORT MetaBase : public QSharedData
> {
> + friend class Observer;
> +
> public:
> MetaBase() {}
> virtual ~MetaBase() {}
> @@ -94,9 +121,6 @@
>
> virtual void addMatchTo( QueryMaker *qm ) = 0;
>
> - virtual void subscribe( Observer *observer );
> - virtual void unsubscribe( Observer *observer );
> -
> virtual bool hasCapabilityInterface( Meta::Capability::Type type ) const;
>
> virtual Meta::Capability* asCapabilityInterface( Meta::Capability::Type type );
> @@ -125,9 +149,11 @@
> }
>
> protected:
> + virtual void subscribe( Observer *observer );
> + virtual void unsubscribe( Observer *observer );
> +
> virtual void notifyObservers() const = 0;
>
> - protected:
> QSet<Meta::Observer*> m_observers;
>
> private: // no copy allowed, since it's not safe with observer list
> --- trunk/extragear/multimedia/amarok/src/meta/proxy/MetaProxy_p.h #821448:821449
> @@ -93,7 +93,7 @@
> Meta::TrackPtr track = newTrackProvider->trackForUrl( url );
> if( track )
> {
> - track->subscribe( this );
> + subscribeTo( track );
> realTrack = track;
> notifyObservers();
> disconnect( CollectionManager::instance(), SIGNAL( trackProviderAdded( TrackProvider* ) ), this, SLOT( slotNewTrackProvider( TrackProvider* ) ) );
> --- trunk/extragear/multimedia/amarok/src/playlist/PlaylistModel.cpp #821448:821449
> @@ -878,9 +878,9 @@
> {
> if( track )
> {
> - track->subscribe( this );
> + subscribeTo( track );
> if( track->album() )
> - track->album()->subscribe( this );
> + subscribeTo( track->album() );
>
> m_items.insert( row + i, new Item( track ) );
> i++;
> @@ -920,9 +920,9 @@
> for( int i = position; i < position + rows; i++ )
> {
> Item* item = m_items.takeAt( position ); //take at position, row times
> - item->track()->unsubscribe( this );
> + unsubscribeTo( item->track() );
> if( item->track()->album() )
> - item->track()->album()->unsubscribe( this );
> + unsubscribeTo( item->track()->album() );
> ret.push_back( item->track() );
> delete item;
> }
> --- trunk/extragear/multimedia/amarok/src/servicebrowser/lastfm/meta/MultiPlayableCapabilityImpl_p.h #821448:821449
> @@ -30,13 +30,12 @@
> , m_url( track->playableUrl() )
> , m_track( track )
> {
> - m_track->subscribe( this );
> + //FIXME
> + //subscribeTo( m_track );
> }
>
> virtual ~MultiPlayableCapabilityImpl()
> - {
> - m_track->unsubscribe( this );
> - }
> + {}
>
> virtual void fetchFirst() { m_track->playCurrent(); }
> virtual void fetchNext() { m_track->playNext(); }
> --- trunk/extragear/multimedia/amarok/src/servicebrowser/magnatunestore/MagnatunePurchaseDialog.cpp #821448:821449
> @@ -38,6 +38,8 @@
> : QDialog( parent, fl )
> {
> Q_UNUSED( modal );
> + DEBUG_BLOCK
> +
> setObjectName( name );
> setupUi( this );
>
> @@ -48,7 +50,9 @@
> }
>
> MagnatunePurchaseDialog::~MagnatunePurchaseDialog()
> -{}
> +{
> + DEBUG_BLOCK
> +}
>
>
> void MagnatunePurchaseDialog::setAlbum( MagnatuneAlbum * album )
> @@ -62,10 +66,9 @@
>
> m_albumCode = album->albumCode();
>
> - album->subscribe( this );
> + subscribeTo( album );
>
> coverPixmapLabel->setPixmap( album->image( 200, false ) );
> -
> }
>
> void MagnatunePurchaseDialog::purchase( )
> --- trunk/extragear/multimedia/amarok/src/widgets/TrackTooltip.cpp #821448:821449
> @@ -93,10 +93,10 @@
> return;
>
> if( m_track->artist() )
> - m_track->artist()->unsubscribe( this );
> + unsubscribeTo( m_track->artist() );
> if( m_track->album() )
> - m_track->album()->unsubscribe( this );
> - m_track->unsubscribe( this );
> + unsubscribeTo( m_track->album() );
> + unsubscribeTo( m_track );
>
> if( force || m_track != track )
> {
> @@ -185,12 +185,11 @@
>
> updateWidgets();
>
> - m_track->subscribe( this );
> + subscribeTo( m_track );
> if( m_track->artist() )
> - m_track->artist()->subscribe( this );
> + subscribeTo( m_track->artist() );
> if( m_track->album() )
> - m_track->album()->subscribe( this );
> -
> + subscribeTo( m_track->album() );
> }
> }
>
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>
More information about the Amarok-devel
mailing list