Bug 203820 - [PATCH] JJ: Missing message when a file can't be copied to a media player
Summary: [PATCH] JJ: Missing message when a file can't be copied to a media player
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Collections/Media Devices (show other bugs)
Version: 2.3-GIT
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2009-08-14 13:13 UTC by Pascal d'Hermilly
Modified: 2010-09-27 22:57 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.4
Sentry Crash Report:


Attachments
CopyFailed handler (4.22 KB, patch)
2010-07-30 16:35 UTC, Sergey Ivanov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pascal d'Hermilly 2009-08-14 13:13:16 UTC
Version:           2.1 (using KDE 4.3.0)
OS:                Linux
Installed from:    Ubuntu Packages

I tried copying some albums from my local collection to an iPod. Just drag and drop.
First album went fine, but when I dragged the second amarok flickered and nothing happened. It simply refused to transfer the album.
Later I found out that it is because the album was Ogg files, and iPod probably doesn't support Ogg files. However there was no message telling me why Amarok was failing.
Comment 1 Myriam Schweingruber 2009-08-14 19:37:13 UTC
Pascal, you really should upgrade to 2.1.1, available in the jaunty-backports repository (not the backports PPA, that's a different one), there have been quite some bugfixes.
Comment 2 Pascal d'Hermilly 2009-08-15 00:09:40 UTC
Yes well I'm actually using 2.1.1 in Karmic now. I just don't remember which specific version I was using last week (other than 2.1.x) when I had this episode. And I don't personally own an iPod.

Should this bug have been fixed in 2.1.1?
Comment 3 Myriam Schweingruber 2009-08-15 00:24:34 UTC
No, else I would have closed the bug, but for testing purpose it's always better to have the latest version (and you get some bugfixes with it, which is always positive, no?).
Comment 4 Pascal d'Hermilly 2009-08-19 11:35:43 UTC
I confirm in 2.1.1
Comment 5 Myriam Schweingruber 2009-11-16 17:30:43 UTC
Any news on this? Don't know if this can be solved in a JJ, eventually?
Comment 6 Myriam Schweingruber 2009-12-08 11:58:43 UTC
Still no message in 2.2-git. Marked as junior_job
Comment 7 Sergey Ivanov 2010-07-30 13:58:17 UTC
May i see debug log when it happened?
Comment 8 Myriam Schweingruber 2010-07-30 15:20:21 UTC
Can somebody with an iPod please reproduce this and provide the debug output? Just run amarok -d --nofork twice to activate the debugging.
Comment 9 Sergey Ivanov 2010-07-30 16:35:23 UTC
Created attachment 49683 [details]
CopyFailed handler

This patch !should! to fix it. But i don't have iPod or any devices work via MTP, so I can't test it well. It would be great if you try it and write back here or on my mail.
Comment 10 Myriam Schweingruber 2010-08-04 22:58:51 UTC
Thank you for the patch, I will ask a developer to look at it.
Comment 11 Mark Kretschmann 2010-09-27 12:40:43 UTC
commit f50796d7ab38f76fb942ff22698210fdeb3b7401
Author: Mark Kretschmann <kretschmann@kde.org>
Date:   Mon Sep 27 12:38:10 2010 +0200

    Print message when a file can't be copied to a media player.
    
    Patch by Sergey Ivanov <123kash@gmail.com>. Thanks!
    
    BUG: 203820

diff --git a/src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp b/src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp
index 1168948..3373b2c 100644
--- a/src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp
+++ b/src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp
@@ -1401,6 +1401,8 @@ IpodHandler::slotCopyingDone( KIO::Job* job, KUrl from, KUrl to, time_t mtime, b
 
     if( !job->error() )
         slotFinalizeTrackCopy( track );
+    else
+        slotCopyTrackFailed( track );
 }
 
 void
diff --git a/src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp b/src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp
index 1c16380..0c18a26 100644
--- a/src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp
+++ b/src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp
@@ -318,7 +318,7 @@ MediaDeviceHandler::copyTrackListToDevice(const Meta::TrackList tracklist)
     m_isCopying = true;
 
     bool isDupe = false;
-    bool hasDupe = false;
+    bool hasError = false;
     QString format;
     TrackMap trackMap = m_memColl->memoryCollection()->trackMap();
 
@@ -342,6 +342,7 @@ MediaDeviceHandler::copyTrackListToDevice(const Meta::TrackList tracklist)
         {
              const QString error = i18n("Unsupported format: %1", format);
              m_tracksFailed.insert( track, error );
+	     hasError = true;
              continue;
         }
 
@@ -377,7 +378,7 @@ MediaDeviceHandler::copyTrackListToDevice(const Meta::TrackList tracklist)
 
             // Track is already on there, break
             isDupe = true;
-            hasDupe = true;
+            hasError = true;
             break;
         }
 
@@ -393,7 +394,7 @@ MediaDeviceHandler::copyTrackListToDevice(const Meta::TrackList tracklist)
     }
 
     // NOTE: see comment at top of copyTrackListToDevice
-    if( hasDupe )
+    if( hasError )
         m_copyFailed = true;
 
     /* List ready, begin copying */
@@ -459,15 +460,39 @@ MediaDeviceHandler::copyNextTrackToDevice()
     DEBUG_BLOCK
     Meta::TrackPtr track;
 
-    // If there are more tracks to copy, copy the next one
+    debug() << "Tracks left to copy after this one is now done: " << m_numTracksToCopy;
+
     if ( !m_tracksToCopy.isEmpty() )
     {
         // Pop the track off the front of the list
         track = m_tracksToCopy.first();
         m_tracksToCopy.removeFirst();
 
-        // Copy the track
-        privateCopyTrackToDevice( track );
+        // Copy the track and check result
+        if ( !privateCopyTrackToDevice( track ) )
+            slotCopyTrackFailed( track );
+    }
+    else
+    {
+        if ( m_numTracksToCopy > 0 )
+            debug() << "Oops. \"Tracks to copy\" counter is not zero, but copy list is empty. Something missed?";
+
+        if ( m_copyFailed )
+        {
+            The::statusBar()->shortMessage( i18np( "%1 track failed to copy to the device",
+                                                   "%1 tracks failed to copy to the device", m_tracksFailed.size() ) );
+        }
+        // clear maps/hashes used
+
+        m_tracksCopying.clear();
+        m_trackSrcDst.clear();
+        m_tracksFailed.clear();
+        m_tracksToCopy.clear();
+
+        // copying done
+
+        m_isCopying = false;
+        emit copyTracksDone( true );
     }
 }
 
@@ -545,30 +570,7 @@ MediaDeviceHandler::slotFinalizeTrackCopy( const Meta::TrackPtr & track )
     addMediaDeviceTrackToCollection( destTrack );
 
     emit incrementProgress();
-
     m_numTracksToCopy--;
-
-    debug() << "Tracks left to copy after this one is now done: " << m_numTracksToCopy;
-
-    if( m_numTracksToCopy == 0 )
-    {
-        if( m_tracksFailed.size() > 0 )
-        {
-            The::statusBar()->shortMessage( i18np( "%1 track failed to copy to the device",
-                                                   "%1 tracks failed to copy to the device", m_tracksFailed.size() ) );
-        }
-        // clear maps/hashes used
-
-        m_tracksCopying.clear();
-        m_trackSrcDst.clear();
-        m_tracksFailed.clear();
-        m_tracksToCopy.clear();
-
-        // copying done
-
-        m_isCopying = false;
-        emit copyTracksDone( true );
-    }
 }
 
 void