Bug 138409

Created attachment 18805 [details]
The patch

Following bugs still exist:
1. Amarok tries to connect to the MTP device upon starting amarok if it is enabled or when adding new mtp device from settings.
2. CopyToDevice checks if track exists on device, but ignores album information.

Mikko,

Thanks for the patch. I won't have time to look at it properly until next week.
I think you are right that not having any tracks on a device is not an error situation and should be dealt with gracefully.

With regards to the other bugs, I think (1) is the case for all Media Devices (they all try and connect at start-up). This should probably remain the behaviour but the GUI should be allowed to finish loading while the device is probed for contents. It's something I've been meaning to investigate for a bit. Thanks for the reminder.

When you say that it ignores album information, can you give an example. As far as I know, it should check for title/album/artist as a combination to detect duplicates.

Libnjb plugin doesnt try to connect on startup (I updated Creative Zen sleek from NJB firmware to MTP). I tried to figure out how it does it but im not a c++ nor c programmer :p. When Amarok starts and it tries to connect (you can see on debug messages it runs ::openDevice on MTP plugin upon loading it), and I have the player plugged in since it charges from usb, it takes 11 minutes to connect (large collection and no usb2.0) and amarok wont show the main interface until it has connected (as you stated).
For the second one: I got 2 songs, their filenames are exact same, and their tracknumber is same and even lenght is same. Bitrate, ablum and year differ. When the other is on device and other starts to transfer, it informs that the track exists on device in both interface and debug. I can upload these files for test purposes if you want. The code should also check for tracknumber(?) since i got sets with "same" songs (same artist, title and album) but different tracknumber (some top 100 lists from different stations).

Seems like there is two things to check if file exists on device, some tracks with same title, album and artist never get to copyTrackToDevice, only difference is lenght and tracknumber Those tracks are also removed from queue, whereas the one on copytracktodevice leaves em to queue.
This checking is in mediabrowser.cpp around lines 2940 (which uses trackExists on mtpmediadevice.cpp) and sending to copyTrackToDevice occours one line 2999 where it never gets. Smells like a bug (add checking for tracknumber if there is one).
The example in comment #4 gets passed from trackExists but fails the code on copyTrackToDevice, maybe it should be removed (the 'if( m_fileNameToItem[ bundle.filename() ] != 0 )' code)?

Autoconnecting is on mtpmediadevice.h line 112, njb misses this variable so it doesnt connect. Setting it to false solves the "problem".

SVN commit 612921 by kelk:

Internationalize the strings for unknown artist, album, etc on MTP media devices.
Don't give an error if a device has no tracks on it. This is valid.
Don't automatically connect to MTP media devices.
CCBUG: 138409



 M  +7 -0      ChangeLog  
 M  +6 -13     src/mediadevice/mtp/mtpmediadevice.cpp  
 M  +1 -1      src/mediadevice/mtp/mtpmediadevice.h  


--- trunk/extragear/multimedia/amarok/ChangeLog #612920:612921
@@ -32,6 +32,13 @@
       you move and rename them.
 
   CHANGES:
+    * MTP media devices are not automatically connected on start-up. This 
+      should solve slow loading times for those with large collections on an 
+      MTP media device. Contributed by Mikko Seppälä. (BR 138409)
+    * Internationalize unknown artist/album/genre strings. Contributed by Mikko
+      Seppälä. (BR 138409)
+    * Don't assume that a device returning 0 tracks is invalid. It could just
+      have no tracks on. Contributed by Mikko Seppälä. (BR 138409)
     * Magnatune store look now matches rest of Amarok much better
     * Album art is displayed on the Magnatune purchase dialog
     * Generic media device now has an option to force VFAT-safe filenames even
--- trunk/extragear/multimedia/amarok/src/mediadevice/mtp/mtpmediadevice.cpp #612920:612921
@@ -59,7 +59,7 @@
 
 MtpMediaDevice::MtpMediaDevice() : MediaDevice()
 {
-    m_name = "MTP Device";
+    m_name = i18n("MTP Media Device");
     m_device = 0;
     m_folders = 0;
     m_playlistItem = 0;
@@ -224,7 +224,7 @@
 
     if( bundle.title().isEmpty() )
     {
-        trackmeta->title = qstrdup( "<Unknown title>" );
+        trackmeta->title = qstrdup( i18n( "Unknown title" ).utf8() );
     }
     else
     {
@@ -233,7 +233,7 @@
 
     if( bundle.album().isEmpty() )
     {
-        trackmeta->album = qstrdup( "<Unknown album>" );
+        trackmeta->album = qstrdup( i18n( "Unknown album" ).utf8() );
     }
     else
     {
@@ -242,7 +242,7 @@
 
     if( bundle.artist().isEmpty() )
     {
-        trackmeta->artist = qstrdup( "<Unknown artist>" );
+        trackmeta->artist = qstrdup( i18n( "Unknown artist" ).utf8() );
     }
     else
     {
@@ -251,7 +251,7 @@
 
     if( bundle.genre().isEmpty() )
     {
-        trackmeta->genre = qstrdup( "<Unknown genre>" );
+        trackmeta->genre = qstrdup( i18n( "Unknown genre" ).utf8() );
     }
     else
     {
@@ -1484,14 +1484,7 @@
 
     if( tracks == 0 )
     {
-        debug()<< "Error reading tracks. 0 returned." << endl;
-        Amarok::StatusBar::instance()->shortLongMessage(
-            genericError,
-            i18n( "Could not read MTP Device tracks" ),
-            KDE::StatusBar::Error
-        );
-        hideProgress();
-        return -1;
+        debug() << "0 tracks returned. Empty device..." << endl;
     }
     else
     {
--- trunk/extragear/multimedia/amarok/src/mediadevice/mtp/mtpmediadevice.h #612920:612921
@@ -128,7 +128,7 @@
 
     public:
         MtpMediaDevice();
-        virtual bool            autoConnect()          { return true; }
+        virtual bool            autoConnect()          { return false; }
         virtual bool            asynchronousTransfer() { return false; }
         bool                    isConnected();
         LIBMTP_mtpdevice_t      *current_device();

Patch was applied.

*** Bug 138741 has been marked as a duplicate of this bug. ***