-
-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use Gio for file system based device sync (allows mtp:// urls) #1054
Conversation
wow, that's a big chunk of code! Thanks, we'll look into it. |
bin/gpo
Outdated
@@ -935,6 +935,9 @@ class gPodderCli(object): | |||
def _not_applicable(*args, **kwargs): | |||
pass | |||
|
|||
def _mount_volume(*args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be improved to also mount in gpo, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe passing None as the Gio.MountOperation? Or implementing a Gio.MountOperation logging needed params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There is code in the MTPDevice class using pymtp that sits unused since 2010. |
pymtp is ctypes bindings to libmtp. gio mtp:// urls work using gvfs-mtp which uses libmtp, so features should be the same. |
…ix spawning 1 thread per sync task
In the code there is a 9 year old comment:
it seems like iPod support got re-enabled 8 years ago (the detection code for |
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:
This is triggered by the download code calling
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! |
…e to remove a folder (two threads could try to do this at the same time); log file copy errors (these seem to get lost by the ui)
… tasks if after adding a task it completed very quickly on another thread; fix use of Gtk.ListView as a multi-threaded container (sometimes returns None for an item and causes an exception)
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. |
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!
but how are you sure the UI and task lists are in sync now? Thanks, |
Also, could you update the sync section of the user manual? |
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.
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. |
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) |
… device fails; handle errors from querying device
I've removed the old MTP code and the comments related to re-enabling ipod/mtp support (ipod support has already been re-enabled) |
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. |
OK. Let's have a look again at the code, but it looks good to me. |
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. |
Seems reasonable to write a new file an move it then. PR is ready to merge. |
@elelay I agree. |
@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. |
Merged, thanks 👍 |
@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? |
@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()
|
The + 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.
|
Yes, in that case it would spawn one thread per download -- I've got a local fix for this |
|
This fixes a race condition - |
Fix linting issues from #1054
@@ -1539,7 +1546,7 @@ def _for_each_task_set_status(self, tasks, status, force_start=False): | |||
self.download_queue_manager.force_start_task(task) | |||
else: | |||
self.download_queue_manager.queue_task(task) | |||
self.enable_download_list_update() | |||
self.set_download_list_state(gPodderSyncUI.DL_ADDED_ONE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces gPodderSyncUI.DL_ADDED_ONE
, but this is not defined anywhere.
Steps to reproduce:
- Download a file
- Pause the download
- 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'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #1137.
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. |
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.