use Gio for file system based device sync (allows mtp:// urls)

[blushingpenguin]

This allows synchronisation with devices that don't support normal file system mounting (in my case a Garmin Fenix which only supports the mtp protocol and not mass storage). I also added a checkbox to allow deleting tracks from the device when they are deleted in gPodder. I'm happy to address any feedback and rework as necessary, e.g. there's some currently dead code that supports libmtp that I haven't removed -- this would maybe a good time to do that.

[elelay]

wow, that's a big chunk of code! Thanks, we'll look into it.
Do you know if gio works equally well (at least for standard filesystem) on macOS and windows?

[elelay]

this should be improved to also mount in gpo, no?

[elelay]

maybe passing None as the Gio.MountOperation? Or implementing a Gio.MountOperation logging needed params?

[blushingpenguin]

What happens in Gtk is that when the mount operation is handled, if additional information is needed then it shows a dialog requesting username, password, truecrypt key, etc if necessary. This dialog is built into Gtk and doesn't need to be user implemented -- for the command line it looks like I'd need to implement something what's in gvfs-mount where it figures out the needed info in the mount callback and requests it:
https://gitlab.gnome.org/GNOME/glib/-/blob/master/gio/gio-tool-mount.c
It's also possible to skip authentication which I assume would fail the mount. I can do this if you think it's useful, I figured given gpo is likely for scripting people would probably just call gvfs-mount with the relevant parameters before gpo. That said, I'm not really sure how people do use it.

[elelay]

It's also used for accessibility reasons or by users prefering the command-line.
Can you try passing None instead of Gtk.MountOperation.new() in your syncui code and see if MTP mounting works? If so we should implement this in gpo since it's useful to users.

[blushingpenguin]

Yes, that works, the device gets mounted if it starts unmounted. I have added code to run the mount before sync for the CLI as well, and also fixed the hang if opening the device fails (this already exists -- it waits for done_callback which never happens, however it's possible this couldn't be triggered before with an fs sync).

As a side note I did run into what looks like https://bugs.gentoo.org/788133 along the way where starting gpodder, unmounting the device then running a sync mounts the volume but throws that error on the first fs operation. A subsequent sync then succeeds.

[elelay]

There is code in the MTPDevice class using pymtp that sits unused since 2010.
Is it not relevant anymore?

[elelay]

There is code in the MTPDevice class using pymtp that sits unused since 2010.
Is it not relevant anymore?

pymtp is ctypes bindings to libmtp. gio mtp:// urls work using gvfs-mtp which uses libmtp, so features should be the same.
But merging normal filesystem and mtp operations is a benefit of your code. Just ensure you grabbed any relevant goodness in the MTPDevice...

[blushingpenguin]

In the code there is a 9 year old comment:

#
# TODO: Re-enable iPod and MTP sync support
#

it seems like iPod support got re-enabled 8 years ago (the detection code for import gpod is present), but libmtp was never re-enabled (the detection code is commented out so I think it's safe to remove it. I don't think there's anything in that code that's needed -- possibly the filename sanitization (the device I have is picky about at least some non-ASCII characters so I could carry that across as it would fix one sync issue -- it's a corner case of emoji in filename though)

[blushingpenguin]

I've also run into a couple of issues with the multi-threaded sync code, you can see in the last commit I've fixed limited the thread count limitation (it was spawning one thread per task because the limiting code wasn't taking into account the number of currently running threads). There's a more fiddly sporadic issue which comes up sometimes:

mark@pollux ~/d/Tools (master)> Exception in thread Thread-105:
Traceback (most recent call last):
  File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.9/threading.py", line 892, in run
    self._target(*self._args, **self._kwargs)
  File "/home/mark/gpodder/src/gpodder/download.py", line 387, in run
    task = self.queue.get_next()
  File "/home/mark/gpodder/src/gpodder/gtkui/download.py", line 153, in get_next
    result = next(self._work_gen())
  File "/home/mark/gpodder/src/gpodder/gtkui/download.py", line 160, in <genexpr>
    if task.status == task.QUEUED)
AttributeError: 'NoneType' object has no attribute 'status'

This is triggered by the download code calling task = self.queue.get_next() where queue is derived from Gtk.ListStore. There is a lock in there to try and prevent threading issues but it doesn't seem to prevent the issue. I tried to look up the rules for threading and it seems the correct method is to only access GTK gui objects from the main thread, from https://pygobject.readthedocs.io/en/latest/guide/threading.html:

In the background example_target() gets executed and calls GLib.idle_add() and time.sleep() in a loop. In this example
time.sleep() represents the blocking operation. GLib.idle_add() takes the update_progess() function and arguments that will get
passed to the function and asks the main loop to schedule its execution in the main thread. This is needed because GTK isn’t
thread safe; only one thread, the main thread, is allowed to call GTK code at all times."

I can try and rework this to (hopefully) fix the problem -- I think that would mean splitting up the progress list object and the work queue. Does this seem like a reasonable approach? (I'm not a python or a Gtk programmer so any advice is appreciated). Also should I split the threading fixes into a separate PR?

Thanks!

[blushingpenguin]

In 729dbee I've fixed some issues where errors were being thrown due to multiple threads trying to manipulate directories (e.g. both trying to remove an empty directory at the same time, or one thread removing an empty directory then another checking if it was empty after it had been removed). (Likely these are bugs introduced by the PR).

In b40f283 I've split DownloadStatusModel up so that it has a thread safe work queue and also tries to encapsulate the gtk list rather than derive from it to prevent external manipulation (that didn't work out that well -- main.py still ends up manipulating it directly). In my testing I haven't seen the "AttributeError: 'NoneType' object has no attribute 'status'" since making this change.

I've also fixed an issue where the sync ui would stick sometimes (particularly when debugging) -- this could happen if ui updates were enabled, then a task was added and completed immediately via a background thread (very common for files that were already present on a device being synchronised with). I've also running sync multiple times leading to the same problem (ctrl+s,ctrl+s reproduces) -- the ui should probably prevent double sync, but it doesn't hurt to cope if it happens.

[elelay]

In 729dbee I've fixed some issues where errors were being thrown due to multiple threads trying to manipulate directories (e.g. both trying to remove an empty directory at the same time, or one thread removing an empty directory then another checking if it was empty after it had been removed). (Likely these are bugs introduced by the PR).

How useful is threaded sync, really? Last time I checked with an actual MP3 device, it hurt performance so much to have multiple files written at once!

In b40f283 I've split DownloadStatusModel up so that it has a thread safe work queue and also tries to encapsulate the gtk list rather than derive from it to prevent external manipulation (that didn't work out that well -- main.py still ends up manipulating it directly). In my testing I haven't seen the "AttributeError: 'NoneType' object has no attribute 'status'" since making this change.

but how are you sure the UI and task lists are in sync now?

Thanks,

[elelay]

Also, could you update the sync section of the user manual?

[blushingpenguin]

How useful is threaded sync, really? Last time I checked with an actual MP3 device, it hurt performance so much to have multiple files written at once!

Not that useful for my device -- only one file at a time ends up being written, however it's a few seconds quicker to do a synchronisation that doesn't actually write anything. It's worth nothing that the bug can still be hit with only one thread -- there's no way of doing a non-threaded sync.

In b40f283 I've split DownloadStatusModel up so that it has a thread safe work queue and also tries to encapsulate the gtk list rather than derive from it to prevent external manipulation (that didn't work out that well -- main.py still ends up manipulating it directly). In my testing I haven't seen the "AttributeError: 'NoneType' object has no attribute 'status'" since making this change.

but how are you sure the UI and task lists are in sync now?

That was the point of trying to encapsulation the list manipulation -- all of the list manipulation calls go through functions on the list wrapper, so the work queue and the ui should be kept in sync.

[blushingpenguin]

wow, that's a big chunk of code! Thanks, we'll look into it.
Do you know if gio works equally well (at least for standard filesystem) on macOS and windows?

I tested on Windows -- synchronising to file system locations that can be accessed with a drive letter appears to work well. It's not possible to select an MTP device or a network mounted device using the GTK file browser (only things with a drive letter appear to work, this is also the case without this PR and I think is a GLib limitation)

[blushingpenguin]

pymtp is ctypes bindings to libmtp. gio mtp:// urls work using gvfs-mtp which uses libmtp, so features should be the same.
But merging normal filesystem and mtp operations is a benefit of your code. Just ensure you grabbed any relevant goodness in the MTPDevice...

I've removed the old MTP code and the comments related to re-enabling ipod/mtp support (ipod support has already been re-enabled)

[blushingpenguin]

Also, could you update the sync section of the user manual?

I added gpodder/gpodder.github.io#36. I'm not really sure how to update the images short of redoing them all as gPodder looks quite different on my system though.

[elelay]

but how are you sure the UI and task lists are in sync now?

That was the point of trying to encapsulation the list manipulation -- all of the list manipulation calls go through functions on the list wrapper, so the work queue and the ui should be kept in sync.

OK. Let's have a look again at the code, but it looks good to me.

[blushingpenguin]

Thanks for spotting that. I've updated the code to handle writing playlists to gvfs uris and validated that the output is the same as from the original code. I've also checked that this works using file:// and smb:// urls.

I've added something to handle sending playlists to mtp devices -- this works with my device (which just ignores the copied playlists) and I don't have any other mtp devices to test with so I'm not sure how useful it is. The code is a bit clunky as I couldn't get any of the GFile open_readwrite, replace, etc methods to work (they all returned 'operation not supported'), so I ended up having to save the playlist to a temp file then copy that file over.

[elelay]

Seems reasonable to write a new file an move it then.

PR is ready to merge.
@auouymous let's get the new release out and merge just after, don't you think?

[auouymous]

@elelay I agree.

[blushingpenguin]

@elelay @auouymous if you are considering code for a release you might want to cherry pick c9bbc4f which fixes deleting episodes on sync -- currently that causes a syntax error (62bb7ef changed the function signature). I've done a quick search of the open issues but I don't see one that matches that problem.

It might also worth considering the change to download.py in e7fab75 which fixes spawning one thread per download task (currently that code isn't taking into account the number of running threads). I'm happy to split that one out if you want it.

[auouymous]

c9bbc4f is already in master as 63196ae

[elelay]

Merged, thanks 👍

[auouymous]

@blushingpenguin https://github.com/gpodder/gpodder/blob/master/src/gpodder/download.py#L444 subtracts running count from spawn_limit a second time (when there is download limit set) and causes #1111. Was this needed for the case when there is no download limit?

[auouymous]

@blushingpenguin And why was this removed? Reverting that line fixes the second issue in #1111.

@@ -461,7 +461,6 @@ def force_start_task(self, task):
     def queue_task(self, task):
         """Marks a task as queued
         """
-        task.status = DownloadTask.QUEUED
         self.__spawn_threads()
 
 

[auouymous]

The move_after and move_before methods in TaskQueue appear to take an element and lookup its index.

+    def move_after(self, task, after):
+                index = self.tasks.index(after)

But they both appear to be called with an index, not an element. I don't know how to invoke the code that calls this to test if it is working or not.

        pos_task = self.list.get_value(position, DownloadStatusModel.C_TASK)
        self.work_queue.move_after(iter_task, pos_task)

[blushingpenguin]

@blushingpenguin https://github.com/gpodder/gpodder/blob/master/src/gpodder/download.py#L444 subtracts running count from spawn_limit a second time (when there is download limit set) and causes #1111. Was this needed for the case when there is no download limit?

Yes, in that case it would spawn one thread per download -- I've got a local fix for this

[blushingpenguin]

The move_after and move_before methods in TaskQueue appear to take an element and lookup its index.

+    def move_after(self, task, after):
+                index = self.tasks.index(after)

But they both appear to be called with an index, not an element. I don't know how to invoke the code that calls this to test if it is working or not.

        pos_task = self.list.get_value(position, DownloadStatusModel.C_TASK)
        self.work_queue.move_after(iter_task, pos_task)

pos_task = self.list.get_value(position, DownloadStatusModel.C_TASK) fetches the task associated with the given position from the list view, so move_after in TaskQueue is called with two task objects. I have checked this, and it seems to be working ok. You can invoke this code by capping the number of concurrent downloads to 1, then starting some downloads, then re-ordering them in the progress tab.

[blushingpenguin]

@blushingpenguin And why was this removed? Reverting that line fixes the second issue in #1111.

@@ -461,7 +461,6 @@ def force_start_task(self, task):
     def queue_task(self, task):
         """Marks a task as queued
         """
-        task.status = DownloadTask.QUEUED
         self.__spawn_threads()
 
 

This fixes a race condition - download_episode_list calls register_task which adds the tasks to the queue of things to download. It then calls util.idle_add(queue_tasks, new_tasks, queued_existing_task), which calls queue_task on each task. queue_task calls __spawn_threads which starts existing tasks downloading - those tasks then have their state set to downloading. The next call to queue_task call then possibly moves an existing task from the downloading to the queued state and this unexpected state change causes the download to get stuck. I didn't notice the unintended side-effect of removing this -- I will have a go at fixing it at the weekend.

[thp]

This PR introduces gPodderSyncUI.DL_ADDED_ONE, but this is not defined anywhere.
Steps to reproduce:

1. Download a file
2. Pause the download
3. Try to resume the download

Traceback (most recent call last):
  File ".../gpodder/src/gpodder/gtkui/main.py", line 1622, in <lambda>
    item.connect('activate', lambda item: self._for_each_task_set_status(tasks, status, force_start))
  File ".../gpodder/src/gpodder/gtkui/main.py", line 1567, in _for_each_task_set_status
    self.set_download_list_state(gPodderSyncUI.DL_ADDED_ONE)
AttributeError: type object 'gPodderSyncUI' has no attribute 'DL_ADDED_ONE'

[thp]

Filed #1137.

[greenythebeast]

Can anyone let me know if the current version of gpodder supports MTP sync on Windows? Can't figure out if there is a version that does that.