Fwd: extragear/multimedia/amarok/src/collection

[Maximilian Kossick]

Hey

did any of the users who had problems with deleted files test a recent
SVN revision? I'm pretty sure this commit fixes the issue, and
although it may not represent the consensus on the mailing list, but
it should be backported nevertheless if it solves the problem. (and
does so without feature regressions)

Cheers, Max


---------- Forwarded message ----------
From: Maximilian Kossick <maximilian.kossick at googlemail.com>
Date: Thu, Jun 25, 2009 at 11:00 PM
Subject: extragear/multimedia/amarok/src/collection
To: kde-commits at kde.org


SVN commit 987338 by mkossick:

invert logic for moving files: only delete source file if the
destination declares that to be fine instead of assuming that the
transfer worked, and only keep source files for which we were informed
about a transfer error.
assuming that KIO::FileCopyJob correctly reports an error if a file
could not be moved or copied this should fix the issues with vanishing
files that people have reported.

This fix even works for collections were any error handling was
forgotten (i'm looking at you, mp3tunes and mediadevice)

 M  +12 -13    CollectionLocation.cpp
 M  +16 -6     CollectionLocation.h
 M  +15 -5     sqlcollection/SqlCollectionLocation.cpp
 M  +3 -9      support/FileCollectionLocation.cpp


--- trunk/extragear/multimedia/amarok/src/collection/CollectionLocation.cpp
#987337:987338
@@ -355,8 +355,9 @@
 CollectionLocation::slotFinishCopy()
 {
    if( m_removeSources )
-        removeSourceTracks( m_sourceTracks );
+        removeSourceTracks( m_tracksSuccessfullyTransferred );
    m_sourceTracks.clear();
+    m_tracksSuccessfullyTransferred.clear();
    m_destination->deleteLater();
    m_destination = 0;
    this->deleteLater();
@@ -488,26 +489,24 @@
    return m_source;
 }

+CollectionLocation*
+CollectionLocation::destination() const
+{
+    return m_destination;
+}
+
 void
 CollectionLocation::setSource( CollectionLocation *source )
 {
    m_source = source;
 }
-bool
-CollectionLocation::movedByDestination( const Meta::TrackPtr &track ) const
-{
-    return m_tracksRemovedByDestination.contains( track ) &&
m_tracksRemovedByDestination[ track ];
-}
-bool
-CollectionLocation::consideredByDestination( const Meta::TrackPtr
&track ) const
-{
-    return m_tracksRemovedByDestination.contains( track );
-}
+
 void
-CollectionLocation::setMovedByDestination( const Meta::TrackPtr
&track, bool removeFromDatabase )
+CollectionLocation::transferSuccessful( const Meta::TrackPtr &track )
 {
-    m_tracksRemovedByDestination.insert( track, removeFromDatabase );
+    m_tracksSuccessfullyTransferred.append( track );
 }
+
 bool
 CollectionLocation::isGoingToRemoveSources() const
 {
--- trunk/extragear/multimedia/amarok/src/collection/CollectionLocation.h
#987337:987338
@@ -178,11 +178,11 @@
        bool remove( const Meta::TrackList &tracks );

        /**
-        * Sets or gets which files the source will potentially need to remove
-        */
-        virtual bool movedByDestination( const Meta::TrackPtr &track ) const;
-        virtual bool consideredByDestination( const Meta::TrackPtr
&track ) const;
-        virtual void setMovedByDestination( const Meta::TrackPtr
&track, bool removeFromDatabase );
+          explicitly inform the source collection of successful transfer.
+          The source collection will only remove files (if necessary)
+          for which this method was called.
+          */
+        void transferSuccessful( const Meta::TrackPtr &track );

        /**
        * tells the source location that an error occurred during the
transfer of the file
@@ -216,7 +216,15 @@
         * note: subclasses do not take ownership  of the pointer
         */
        CollectionLocation* source() const;
+
        /**
+          * allows the source location to access the destination
CollectionLocation.
+          * Pointer may be null!
+          * note: subclasses do not take ownership of the pointer
+          */
+        CollectionLocation* destination() const;
+
+        /**
            this method is called on the source location, and should
return a list of urls
            which the destination location can copy using KIO. You must call
            slotGetKIOCopyableUrlsDone( QMap<Meta::TrackPtr, KUrl> )
after retrieving the
@@ -312,7 +320,9 @@

        bool m_removeSources;
        bool m_isRemoveAction;
-        QMap<Meta::TrackPtr, bool> m_tracksRemovedByDestination;
+        //used by the source collection to store the tracks that were
successfully
+        //copied by the destination and can be removed as part of a move
+        Meta::TrackList m_tracksSuccessfullyTransferred;
        QMap<Meta::TrackPtr, QString> m_tracksWithError;
 };

--- trunk/extragear/multimedia/amarok/src/collection/sqlcollection/SqlCollectionLocation.cpp
#987337:987338
@@ -86,13 +86,22 @@
    if( sqlTrack && sqlTrack->inCollection() &&
sqlTrack->collection()->collectionId() == m_collection->collectionId()
)
    {
        bool removed;
-        if( !consideredByDestination( track ) )
+        //SqlCollectionLocation uses KIO::move for moving files internally
+        //therefore we check whether the destination CollectionLocation
+        //represents the same collection, and check the existence of the
+        //file. If it does not exist, we assume that it has been
moved, and remove it from the database.
+        //at worst we get a warning about a file not in the database.
If it still exists, and
+        //this method has been called, do not do anything, as
something is wrong.
+        //If the destination location is another collection, remove
the file as we expect
+        //the destination to tell us if it is really really sure that
it has copied a file.
+        //If we are not copying/moving files destination() will be 0.
+        if( destination() && destination()->collection() == collection() )
        {
-            removed = QFile::remove( sqlTrack->playableUrl().path() );
+            removed = !QFile::exists( sqlTrack->playableUrl().path() );
        }
        else
        {
-            removed = movedByDestination( track );
+            removed = QFile::remove( sqlTrack->playableUrl().path() );
        }
        if( removed )
        {
@@ -171,6 +180,9 @@
        source()->transferError(m_jobs.value( job ),
KIO::buildErrorString( job->error(), job->errorString() ) );
        m_destinations.remove( m_jobs.value( job ) );
    }
+    //we  assume that KIO works correctly...
+    source()->transferSuccessful( m_jobs.value( job ) );
+
    m_jobs.remove( job );
    job->deleteLater();

@@ -328,14 +340,12 @@
        }
        if( src == dest) {
        //no changes, so leave the database alone, and don't erase anything
-            source()->setMovedByDestination( track, false );
            return false;
        }
    //we should only move it directly if we're moving within the same collection
        else if( isGoingToRemoveSources() && source()->collection() ==
collection() )
        {
            job = KIO::file_move( src, dest, -1, flags );
-            source()->setMovedByDestination( track, true );  //remove
old location from tracks table
        }
        else
        {
--- trunk/extragear/multimedia/amarok/src/collection/support/FileCollectionLocation.cpp
#987337:987338
@@ -56,15 +56,9 @@
    DEBUG_BLOCK
    if( !track )
        return false;
-    bool removed;
-    if( !consideredByDestination( track ) )
-    {
-        removed = QFile::remove( track->playableUrl().path() );
-    }
-    else
-    {
-        removed = movedByDestination( track );
-    }
+
+    bool removed = QFile::remove( track->playableUrl().path() );
+
    if( removed )
    {
        QFileInfo file( track->playableUrl().path() );