From amarok-devel Mon Aug 09 14:40:56 2010 From: Maximilian Kossick Date: Mon, 09 Aug 2010 14:40:56 +0000 To: amarok-devel Subject: Re: [Amarok] 57e87dc Fix Dynamic Collections. Message-Id: X-MARC-Message: https://marc.info/?l=amarok-devel&m=128152383718438 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 wrote: > commit 57e87dc0139577120a19c0f1c39f5b7fb02b002e > Author: Jeff Mitchell > 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@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 . > >   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 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(); > +    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(); >     const Solid::StorageAccess *volumeAccess = device.as(); >     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@kde.org > https://mail.kde.org/mailman/listinfo/amarok-devel > _______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel