[Amarok] 57e87dc Fix Dynamic Collections.

[Maximilian Kossick]

Please check if UrlUpdateJob in MountPointManager.cpp has to be
reenabled. It might not be necessary, as statistics and labels now use
a FK on the urls tables, but please make sure that the deviceid/rpath
pair in the urls table gets updated.

On Mon, Aug 9, 2010 at 12:56 AM, Jeff Mitchell <mitchell at kde.org> wrote:
> commit 57e87dc0139577120a19c0f1c39f5b7fb02b002e
> Author: Jeff Mitchell <mitchell at kde.org>
> Date:   Sun Aug 8 18:50:39 2010 -0400
>
>    Fix Dynamic Collections.
>
>    There were three problems I could find:
>
>    1) The more minor was that devices that were already mounted when Amarok
>    was first started (or the database freshly wiped) wouldn't be found
>    because the database would be created after the mount point detection
>    code, so the device entries couldn't be inserted.
>
>    2) When a device was added, the Solid predicate that was being searched
>    could not always match. Dunno why...bug in Solid, or us, but regardless,
>    instead find all storage volumes and search the returned devices
>    manually.
>
>    3) The MountPointManager code wasn't properly watching for mount changes.
>    MediaDeviceCache was, though, so now MPM gets its signals from MDC
>    instead of from Solid directly.
>
>    This fixes all of the problems with Dynamic Collection that I could
>    find. I'd appreciate if people could test this out and see if they can
>    still reproduce problems. N.B.: you *must* rescan your files after you
>    compile this code, because that's the only way to get the files associated
>    with the proper device IDs (unless you know how to tweak your DB
>    manually). So if you don't rescan your files you'll probably still have
>    problems until you do.
>
>    CCMAIL: amarok-devel at kde.org
>    CCBUG: 171213
>
> diff --git a/ChangeLog b/ChangeLog
> index 7a856ec..714a7c4 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -42,6 +42,9 @@ VERSION 2.3.2-Beta 1
>       Patch by Richard Longland <rlongland at hotmail.com>.
>
>   BUGFIXES:
> +    * Fix regression in Dynamic Collections. For files that were scanned when
> +      Dynamic Collections wasn't working, you will need to rescan them to get
> +      them associated with the proper device.
>     * Fix crash on exit with newer KDE versions. Patch by Martin Blumenstingl and Felix
>       Geyer. (BR 245513)
>     * Fixed playlist bottom toolbar getting to tall when using "Text only"
> diff --git a/src/MediaDeviceCache.h b/src/MediaDeviceCache.h
> index c34ea7c..11f0c01 100644
> --- a/src/MediaDeviceCache.h
> +++ b/src/MediaDeviceCache.h
> @@ -55,7 +55,7 @@ class AMAROK_EXPORT MediaDeviceCache : public QObject
>     signals:
>         void deviceAdded( const QString &udi );
>         void deviceRemoved( const QString &udi );
> -       void accessibilityChanged( bool accessible, const QString &udi );
> +        void accessibilityChanged( bool accessible, const QString &udi );
>
>     public slots:
>         void slotAddSolidDevice( const QString &udi );
> diff --git a/src/core-impl/collections/sqlcollection/MountPointManager.cpp b/src/core-impl/collections/sqlcollection/MountPointManager.cpp
> index 013376b..a28e976 100644
> --- a/src/core-impl/collections/sqlcollection/MountPointManager.cpp
> +++ b/src/core-impl/collections/sqlcollection/MountPointManager.cpp
> @@ -18,6 +18,8 @@
>
>  #include "MountPointManager.h"
>
> +#include "MediaDeviceCache.h"
> +
>  #include "core/support/Amarok.h"
>  #include "core/support/Debug.h"
>  #include "statusbar/StatusBar.h"
> @@ -52,8 +54,8 @@ MountPointManager::MountPointManager( QObject *parent, SqlStorage *storage )
>         return;
>     }
>
> -    connect( Solid::DeviceNotifier::instance(), SIGNAL( deviceAdded( QString ) ), SLOT( deviceAdded( QString ) ) );
> -    connect( Solid::DeviceNotifier::instance(), SIGNAL( deviceRemoved( QString ) ), SLOT( deviceRemoved( QString ) ) );
> +    connect( MediaDeviceCache::instance(), SIGNAL( deviceAdded( QString ) ), SLOT( deviceAdded( QString ) ) );
> +    connect( MediaDeviceCache::instance(), SIGNAL( deviceRemoved( QString ) ), SLOT( deviceRemoved( QString ) ) );
>
>     init();
>
> @@ -445,14 +447,22 @@ void
>  MountPointManager::deviceAdded( const QString &udi )
>  {
>     DEBUG_BLOCK
> -    Solid::Predicate predicate = Solid::Predicate( Solid::DeviceInterface::StorageVolume, "udi", udi );
> +    Solid::Predicate predicate = Solid::Predicate( Solid::DeviceInterface::StorageVolume );
>     QList<Solid::Device> devices = Solid::Device::listFromQuery( predicate );
> -    //there'll be maximum one device because we are using the udi in the predicate
> -    if( !devices.isEmpty() )
> +    //Looking for a specific udi in predicate seems flaky/buggy; the foreach loop barely
> +    //takes any time, so just be safe
> +    bool found = false;
> +    debug() << "looking for udi " << udi;
> +    foreach( Solid::Device device, devices )
>     {
> -        Solid::Device device = devices[0];
> -        createHandlerFromDevice( device, udi );
> +        if( device.udi() == udi )
> +        {
> +            createHandlerFromDevice( device, udi );
> +            found = true;
> +        }
>     }
> +    if( !found )
> +        debug() << "Did not find device from Solid for udi " << udi;
>  }
>
>  void
> @@ -479,6 +489,7 @@ MountPointManager::deviceRemoved( const QString &udi )
>
>  void MountPointManager::createHandlerFromDevice( const Solid::Device& device, const QString &udi )
>  {
> +    DEBUG_BLOCK
>     if ( device.isValid() )
>     {
>         debug() << "Device added and mounted, checking handlers";
> @@ -507,8 +518,12 @@ void MountPointManager::createHandlerFromDevice( const Solid::Device& device, co
>                 emit deviceAdded( key );
>                 break;  //we found the added medium and don't have to check the other device handlers
>             }
> +            else
> +                debug() << "Factory can't handle device " << udi;
>         }
>     }
> +    else
> +        debug() << "Device not valid!";
>  }
>
>
> diff --git a/src/core-impl/collections/sqlcollection/SqlCollection.cpp b/src/core-impl/collections/sqlcollection/SqlCollection.cpp
> index 0176fd3..f7a9ad8 100644
> --- a/src/core-impl/collections/sqlcollection/SqlCollection.cpp
> +++ b/src/core-impl/collections/sqlcollection/SqlCollection.cpp
> @@ -73,17 +73,26 @@ SqlCollection::~SqlCollection()
>  }
>
>  void
> -SqlCollection::init()
> +SqlCollection::setUpdater( DatabaseUpdater *updater )
>  {
> -    QTimer::singleShot( 0, this, SLOT( initXesam() ) );
> -    if( !m_updater )
> +    DEBUG_BLOCK
> +    if( !updater )
>     {
>         debug() << "Could not load updater!";
>         return;
>     }
> -
> +    m_updater = updater;
>     if( m_updater->needsUpdate() )
> +    {
> +        debug() << "Needs update!";
>         m_updater->update();
> +    }
> +}
> +
> +void
> +SqlCollection::init()
> +{
> +    QTimer::singleShot( 0, this, SLOT( initXesam() ) );
>
>     QStringList result = m_sqlStorage->query( "SELECT count(*) FROM tracks" );
>     // If database version is updated, the collection needs to be rescanned.
> diff --git a/src/core-impl/collections/sqlcollection/SqlCollection.h b/src/core-impl/collections/sqlcollection/SqlCollection.h
> index da7e340..89fcdc0 100644
> --- a/src/core-impl/collections/sqlcollection/SqlCollection.h
> +++ b/src/core-impl/collections/sqlcollection/SqlCollection.h
> @@ -105,7 +105,7 @@ class AMAROK_SQLCOLLECTION_EXPORT SqlCollection : public Collections::Collection
>
>         void setSqlStorage( SqlStorage *storage ) { m_sqlStorage = storage; }
>         void setRegistry( SqlRegistry *registry ) { m_registry = registry; }
> -        void setUpdater( DatabaseUpdater *updater ) { m_updater = updater; }
> +        void setUpdater( DatabaseUpdater *updater );
>         void setCapabilityDelegate( Capabilities::CollectionCapabilityDelegate *delegate ) { m_capabilityDelegate = delegate; }
>         void setCollectionLocationFactory( SqlCollectionLocationFactory *factory ) { m_collectionLocationFactory = factory; }
>         void setQueryMakerFactory( SqlQueryMakerFactory *factory ) { m_queryMakerFactory = factory; }
> diff --git a/src/core-impl/collections/sqlcollection/SqlCollectionFactory.cpp b/src/core-impl/collections/sqlcollection/SqlCollectionFactory.cpp
> index c852111..a1966ac 100644
> --- a/src/core-impl/collections/sqlcollection/SqlCollectionFactory.cpp
> +++ b/src/core-impl/collections/sqlcollection/SqlCollectionFactory.cpp
> @@ -160,13 +160,17 @@ SqlCollectionFactory::createSqlCollection( const QString &id, const QString &pre
>     SqlCollection *coll = new SqlCollection( id, prettyName );
>     coll->setCapabilityDelegate( new Capabilities::CollectionCapabilityDelegateImpl() );
>
> -    MountPointManager *mpm = new MountPointManager( coll, storage );
> -
> -    coll->setMountPointManager( new SqlMountPointManagerImpl( mpm ) );
>     DatabaseUpdater *updater = new DatabaseUpdater();
>     updater->setStorage( storage );
>     updater->setCollection( coll );
> +
> +    //setUpdater runs the update function; this must be run *before* MountPointManager is initialized or its handlers may try to insert
> +    //into the database before it's created/updated!
>     coll->setUpdater( updater );
> +
> +    MountPointManager *mpm = new MountPointManager( coll, storage );
> +
> +    coll->setMountPointManager( new SqlMountPointManagerImpl( mpm ) );
>     ScanManager *scanMgr = new ScanManager( coll );
>     scanMgr->setCollection( coll );
>     scanMgr->setStorage( storage );
> diff --git a/src/core-impl/collections/sqlcollection/device/massstorage/MassStorageDeviceHandler.cpp b/src/core-impl/collections/sqlcollection/device/massstorage/MassStorageDeviceHandler.cpp
> index f0cc20a..eb4286f 100644
> --- a/src/core-impl/collections/sqlcollection/device/massstorage/MassStorageDeviceHandler.cpp
> +++ b/src/core-impl/collections/sqlcollection/device/massstorage/MassStorageDeviceHandler.cpp
> @@ -39,6 +39,7 @@ MassStorageDeviceHandler::MassStorageDeviceHandler( int deviceId, const QString
>     , m_mountPoint( mountPoint )
>     , m_udi( udi )
>  {
> +    DEBUG_BLOCK
>  }
>
>  MassStorageDeviceHandler::~MassStorageDeviceHandler()
> @@ -104,7 +105,19 @@ bool MassStorageDeviceHandlerFactory::canCreateFromConfig( ) const
>
>  bool MassStorageDeviceHandlerFactory::canHandle( const Solid::Device &device ) const
>  {
> +    DEBUG_BLOCK
>     const Solid::StorageVolume *volume = device.as<Solid::StorageVolume>();
> +    if( !volume )
> +    {
> +        debug() << "found no volume";
> +        return false;
> +    }
> +    if( volume->uuid().isEmpty() )
> +        debug() << "has empty uuid";
> +    if( volume->isIgnored() )
> +        debug() << "volume is ignored";
> +    if( excludedFilesystem( volume->fsType() ) )
> +        debug() << "excluded filesystem of type " << volume->fsType();
>     return volume && !volume->uuid().isEmpty()
>            && !volume->isIgnored() && !excludedFilesystem( volume->fsType() );
>  }
> @@ -126,7 +139,10 @@ DeviceHandler * MassStorageDeviceHandlerFactory::createHandler( const Solid::Dev
>  {
>     DEBUG_BLOCK
>     if( !s )
> +    {
> +        debug() << "!s, returning 0";
>         return 0;
> +    }
>     const Solid::StorageVolume *volume = device.as<Solid::StorageVolume>();
>     const Solid::StorageAccess *volumeAccess = device.as<Solid::StorageAccess>();
>     if( !volume || !volumeAccess )
> @@ -135,7 +151,10 @@ DeviceHandler * MassStorageDeviceHandlerFactory::createHandler( const Solid::Dev
>         return 0;
>     }
>     if( volumeAccess->filePath().isEmpty() )
> +    {
> +        debug() << "not mounted, can't do anything";
>         return 0; // It's not mounted, we can't do anything.
> +    }
>     QStringList ids = s->query( QString( "SELECT id, label, lastmountpoint "
>                                          "FROM devices WHERE type = 'uuid' "
>                                          "AND uuid = '%1';" ).arg( volume->uuid() ) );
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>