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

In general the approach looks good, but I have a few problems with how it's implemented.

@@ -292,7 +298,7 @@ rb_mtp_source_constructor (GType type, guint n_construct_properties,
 
 	g_signal_connect (G_OBJECT (source), "notify::name",
 			  (GCallback)rb_mtp_source_name_changed_cb, NULL);
-	
+
 	/* figure out supported file types */
 	if (LIBMTP_Get_Supported_Filetypes(priv->device, &types, &num_types) == 0) {
 		int i;


please don't include whitespace-only changes.

+/**
+ * Deletes a folder on the device if the folder is empty.
+ * Returns TRUE if successful otherwise FALSE
+ */
+
+static gboolean
+remove_folder_if_empty (LIBMTP_mtpdevice_t * device, guint32 folder_id) {

We don't generate API documentation for plugins, and static functions don't get included in API documentation anyway, so this shouldn't be a gtkdoc comment.
Also, the opening brace for function bodies goes on a new line, and pointer parameters should look like 'type *name', not 'type * name'.

+	// Don't delete default folders

No C++-style comments, please

+		rb_debug ("Didn'd delete default folder %i", folder_id);

"didn't" is misspelled here and in a few other places

+	file_list = LIBMTP_Get_Filelisting_With_Callback (device,NULL,NULL);

I really don't like the idea of fetching a full file list every time a track is deleted.  This is slow and it blocks the UI.  Doesn't the device itself stop you deleting folders if they're not empty?  At the very least, this should only be done if after check for subfolders.

+	// get folder list
+	folders = LIBMTP_Get_Folder_List (device);
+
+	// locate default music folder
+	if (device->default_music_folder > 0) {
+		music_folder = LIBMTP_Find_Folder(folders, device->default_music_folder);
+	}

These comments are useless, they don't add any useful information.

+	// search artist folder
+	folder_id = find_folder_by_name (music_folder->child, trackmeta->artist);
+	if (folder_id == 0) {
+		folder_id = LIBMTP_Create_Folder(device, trackmeta->artist,
+										device->default_music_folder,
+										trackmeta->storage_id);
+
+		rb_debug ("Artist folder %s with id %i created", trackmeta->artist, folder_id);
+
+		folder_id = LIBMTP_Create_Folder(device,
+							trackmeta->album,
+							folder_id,
+							trackmeta->storage_id);
+
+		rb_debug ("Album folder %s with id %i created", trackmeta->album, folder_id);
+
+		trackmeta->parent_id = folder_id;
+		ret = TRUE;

What should happen here when the first LIBMTP_Create_Folder() fails?
Also, it looks like you're formatting for 4-space tabs; we use 8.

Thank you for responding.

At first I'm sorry for not following the coding guidlines and my bad english at all.

Second: I think it makes no difference to the performance if you fetch every time a complete list of files or folders. That are commands running just under a second. The bigger deal is, this plugin is in this state unusable, in my opinion. If you try to put some GB of music to your device at once, rhythmbox has a bocked UI for hours (in my case). And at longer therm I want to remove that issue. But I wanted to become more familiar with the code and gObject at all. That's new for me, I'm used to program plain C or C++...
But I think you are right, this information should be hold in an internal structure.

Third comments: I think nearly every comment is use full. Especially at the top of functions. I didn't know that this is one was a API-doc comment. But look to the code, I see more than thousand lines, but very few comments wish explain whats going on...

Anyway I will work this code over when I find time. Rhythmbox is a very good player and I want help to make it a better peace of software :-)

Out of the libmtp doku:

	
int LIBMTP_Delete_Object 	( 	LIBMTP_mtpdevice_t *  	device,
		uint32_t  	object_id	 
	) 			

This function deletes a single file, track, playlist, folder or any other object off the MTP device, identified by the object ID.

If you delete a folder, there is no guarantee that the device will really delete all the files that were in that folder, rather it is expected that they will not be deleted, and will turn up in object listings with parent set to a non-existant object ID. The safe way to do this is to recursively delete all files (and folders) contained in the folder, then the folder itself.

(In reply to comment #3)
> Thank you for responding.
> 
> At first I'm sorry for not following the coding guidlines and my bad english at
> all.
> 
> Second: I think it makes no difference to the performance if you fetch every
> time a complete list of files or folders. That are commands running just under
> a second. 

Blocking the UI for 'just under a second' is not OK.  Adding extra steps that take just under a second per track isn't good either.

> The bigger deal is, this plugin is in this state unusable, in my
> opinion. If you try to put some GB of music to your device at once, rhythmbox
> has a bocked UI for hours (in my case).

I'm actually mostly done with fixing this.  Anyway, the existence of this issue doesn't make it OK to make it worse.

> 
> Third comments: I think nearly every comment is use full. Especially at the top
> of functions. I didn't know that this is one was a API-doc comment. But look to
> the code, I see more than thousand lines, but very few comments wish explain
> whats going on...

I was specifically talking about comments like "// get folder list" above a call to LIBMTP_Get_Folder_List().  That comment doesn't add any words that aren't already in the code, and if you're looking for comments that explain important issues, it only gets in your way.

You're right that the existing code could do with more comments, but overcommenting new code doesn't fix that.

> Anyway I will work this code over when I find time. Rhythmbox is a very good
> player and I want help to make it a better peace of software :-)
> 
> Out of the libmtp doku:
> 
> 
> int LIBMTP_Delete_Object     (     LIBMTP_mtpdevice_t *      device,
>         uint32_t      object_id     
>     )             
> 
> This function deletes a single file, track, playlist, folder or any other
> object off the MTP device, identified by the object ID.
> 
> If you delete a folder, there is no guarantee that the device will really
> delete all the files that were in that folder, rather it is expected that they
> will not be deleted, and will turn up in object listings with parent set to a
> non-existant object ID. The safe way to do this is to recursively delete all
> files (and folders) contained in the folder, then the folder itself.

OK, it sounds like we do need to check that a folder is empty before trying to remove it.

Perhaps rather than checking each folder as we delete each file, we should use a timeout to do the check after some delay, so if you delete multiple tracks from the same folder we only check once.  It'd probably be a good idea to scan for empty folders on newly added devices, too.

Created attachment 143402 [details] [review]
Version 2 of the patch

The reworked version of the last patch.

I hope it meets all requirements.

This patch didn't work with the thread model used in the MTP plugin.

Commits 8681fbf through c8a2377 implement folder support for MTP devices.