gPodder Bug Tracker – Bug 1579
iPod/MP3 player device synchronization for gPodder 3
Last modified: 2012-07-09 20:19:40 BST
Created attachment 704 [details]
Patch for sync code
My suggested code for adding device sync back into gPodder. I took the sync code from 2.20 (mainly sync.py and related modules) and looked to tie it back into the 3.0 codebase , following the approach used in 2.20
Thanks for the patch. Some feedback (assuming this code will be rewritten as extension):
* See http://help.github.com/set-your-user-name-email-and-github-token/ on how to set your real name and e-mail address in Git (you can ignore the GitHub Token things on that page) - right now, the patch says "Joseph <joseph@uchikoma.(none)>"
* Instead of menuDevice, we should have an "Extras" menu that will allow extensions to add menu items there.
* For the preferences, we should probably make this via some extension mechanism, although I'm not sure how exactly
* gPodderSyncProgress should go away -> We plan to use the "Downloads" tab in the main window as generic "progress" tab in future versions, so the sync progress should appear there instead of the modal dialog
* gPodderSettings_LegacySupport in config.py is (as the name says) only for "legacy support". As the device sync stuff gets rewritten as extension, these settings should be placed as default config into the extension (which will then be attached to the main config automatically)
* Stuff like scrobbler.log and rockbox cover art should be removed from the default extension. It could either be implemented as additional extension or be refactored extensively. It's rather irritating to configure for now
* gPodderSyncUI and everything around that should be moved to the extension
* libconverter should go and be replaced by something else (there are lots of converter extensions already available that convert files after download)
* is_played in PodcastEpisode is deprecated for a reason. Please don't resurrect it.
* With changes like the change to "sync_filename", don't comment out the old stuff - remove it (we have all the old stuff in the repository, anyway, so no need to clutter the codebase with dead, commented-out code)
* sync_to_devices and device_name should be removed from the model - these things can and should be configured as configuration options in the extension (e.g. through a list of podcast feed URLs that should be synced)
* As with the model, the database schema should also not be changed - "is_played" was changed to "is_new" (with the boolean meaning inverted)
* Remove the services.dependency_manager stuff from sync.py - it has been removed and should not be resurrected again (use extension dependencies for that)
* You call the non-existing function log() in util.py - use the "logger" object instead
* What is the purpose of the "Reverted 'visible headers' change to episodeselector.ui" commit? The commit message doesn't tell me :/
Apart from that, the patch looks like a good starting point for getting device sync back into gPodder - thanks for that! :)
Created attachment 710 [details]
Patch to add sync functionality, displaying progress via 'Downloads' tab
I switched to a 'one task per episode' approach and made some further progress. I'm including a patch to add device sync to gpodder which uses the 'Downloads' tab to show progress.
The synchronization process opens the device and creates a 'SyncTask' object for each episode synchronized. The SyncTask object is derived from DownloadTask, overriding the run() method. The task is then registered with the download status model and added to the download queue, which then executes it and updates the GUI in the same way as for downloads.
Presently the Device adds the Task objects to the queue, but when the tasks are executed they call the add_track method on the device to actually copy the files over. Probably not the cleanest way to do it, but this is still a work-in-progress :) This is currently only implemented for a filesystem-based mp3 player.
(In reply to comment #2)
> Created attachment 710 [details]
> I switched to a 'one task per episode' approach and made some further progress.
> I'm including a patch to add device sync to gpodder which uses the 'Downloads'
> tab to show progress.
> The synchronization process opens the device and creates a 'SyncTask' object
> for each episode synchronized. The SyncTask object is derived from
> DownloadTask, overriding the run() method. The task is then registered with the
> download status model and added to the download queue, which then executes it
> and updates the GUI in the same way as for downloads.
> Presently the Device adds the Task objects to the queue, but when the tasks are
> executed they call the add_track method on the device to actually copy the
> files over. Probably not the cleanest way to do it, but this is still a
> work-in-progress :) This is currently only implemented for a filesystem-based
> mp3 player.
Thanks for the updated patch, some feedback:
- The configuration section should be named "device_sync", not "sync"
fssync_channel_subfolder should be named "one_folder_per_podcast"
only_sync_not_played should be named "skip_played_episodes"
mp3_player_delete_played should be named "delete_played_episodes"
mp3_player_max_filename_length should be named "max_filename_length"
the on_sync_* options should be grouped into "after_sync", e.g.:
(the old configuration names were like that for historic reasons -
this new rewrite is a good opportunity to get rid of my bad
historic decisions ;)
- ACTION_NONE, ACTION_ASK, ACTION_MINIMIZED, ACTION_ALWAYS should
probably be removed from DeviceTypeActionList
- Be aware of trailing whitespace (remove it if possible, otherwise
I will do it when I merge the patch, so not really a problem)
- In "on_sync_to_ipod_activate", you have backslashes (\) for line
continuation. This is not required, and PEP-8 suggests against it,
and while other code in gPodder has this style, I want to get rid
of it (again, one of my bad historic style decisions ;). Also, in
some cases, the lines are longer than 80 chars which (while not a
hard rule) should be avoided where possible). Again, if you do not
want to deal with that, I can deal with that reformatting when
merging the patch.
- Not "directory_is_writable( path):" but "directory_is_writable(path):"
(yes, this is again something that we shouldn't have done in old code ;)
- disable_pre_sync_conversion in sync.py is not available in the config
(IIRC we said that for the first version, we don't want to have file
conversion - is that correct? if so, feel free to remove the code in
sync.py that relates to conversion [libconverter et al] and also remove
libconverter.py itself for now)
- When I click on Extras -> Sync to device (with no device configured),
I should either get an error message or the menu should not be shown
- I still get an error when syncing (remove sender=):
Traceback (most recent call last):
File "/usr/lib/python2.7/threading.py", line 551, in __bootstrap_inner
File "/usr/lib/python2.7/threading.py", line 504, in run
File "/home/thp/src/gpodder/src/gpodder/gtkui/desktop/sync.py", line 161, in sync_thread_func
File "/home/thp/src/gpodder/src/gpodder/sync.py", line 208, in add_sync_tasks
logger.info('Excluding %s from sync', track.title, sender=self)
File "/usr/lib/python2.7/logging/__init__.py", line 1140, in info
self._log(INFO, msg, args, **kwargs)
TypeError: _log() got an unexpected keyword argument 'sender'
Do you want me to clean up the patch a bit and send it back to you so you can continue working on it, or do you want to clean up the patch and re-submit it? Thanks for the work so far, you're doing a great job, and I hope to be able to merge the patch soon! :)
Created attachment 712 [details]
Updated patch to add device sync functionality, displaying progress via 'Downloads' tab
- Renamed the config section to 'device_sync' from sync
- Renamed the configuration items as suggested (including regrouping the 'after_sync' section separately
- Removed the 'ACTION' items from DeviceTypeActionList
- Tried to remove as much trailing whitespace as possible
- Removed the backslashes as much as possible (still used once or twice) and tried to adhere to the 80-char line limit as per PEP-8
- Fixed the spacing issues with directory_is_writable
- removed pre_sync conversion code & libconverter
- Fixed error from call to logger
- Note that when the GTK Notification Extension is loaded, you'll get an error message when you try to sync without a device configured.
(In reply to comment #4)
> Created attachment 712 [details]
> - Renamed the config section to 'device_sync' from sync
> - Renamed the configuration items as suggested (including regrouping the
> 'after_sync' section separately
> - Removed the 'ACTION' items from DeviceTypeActionList
> - Tried to remove as much trailing whitespace as possible
> - Removed the backslashes as much as possible (still used once or twice) and
> tried to adhere to the 80-char line limit as per PEP-8
> - Fixed the spacing issues with directory_is_writable
> - removed pre_sync conversion code & libconverter
> - Fixed error from call to logger
> - Note that when the GTK Notification Extension is loaded, you'll get an error
> message when you try to sync without a device configured.
Thanks for the updated patch. As discussed, I'll postpone my review for the next update or when you want to try out Github, I'll add comments to the pull request there (please feel free to post the link to the pull request in this bug report once one is opened).
*** Bug 1572 has been marked as a duplicate of this bug. ***
Hi Joseph - what's the current status of this patch? Is there something to review and merge? :)
Almost there - just a couple of minor UI things - need to update title bar notifications to distinguish downloads from sync tasks, and update the 'Downloads' tab label to also make the same distinction.
I'll upload an updated patch Sunday evening (US West Coast) at the latest, and figure out how to set up a github account next week.
(In reply to comment #8)
> I'll upload an updated patch Sunday evening (US West Coast) at the latest, and
> figure out how to set up a github account next week.
Perfect, thanks :) Hope I get around to review it next week (I'm traveling a bit), but if not, I'll definitely get to review it in early July :)
Created attachment 716 [details]
Updated patch to rename the 'Downloads' tab to 'Tasks', also tweaked title bar notifications.
I would consider the patch ready for merging at this point.
Also removed 'Remove played episodes from device' option as I want to clean up the way gpodder handles removing episodes. Eventually I would like to be able to delete an episode from the device and have it be deleted gPodder on the next sync.
(In reply to comment #10)
> Also removed 'Remove played episodes from device' option as I want to clean up
> the way gpodder handles removing episodes. Eventually I would like to be able
> to delete an episode from the device and have it be deleted gPodder on the next
I'll review the up to date patch at this URL (you'll find comments inline):
Merged into the master branch: http://gpodder.org/commit/972c045a