John McNeil wrote:
> I wrote it for a8562e8a9f8d325416f5d3896e129b97c46ec7bc, as it's both
> the current tip of master and the current HEAD. It actually applies
> just fine to both. Which is to say, that bug occurs in both. The
> problem is that it assumes a stop->pause/play transition before use,
> which, in the case of it being disabled at startup and then enabling
> scrobbling mid-song, doesn't occur.
Thanks, I was mistakenly comparing between your patch and my patch on
top of your patch causing me confusion.
>
> The attached patch fixes both these problems. Note that you can't
> simply initialize the attributes in scrobbler.__init__(). While that
> avoids the exception, it trades it off for instead possibly incorrect
> (possibly *very* incorrect) play durations on the song that's playing
> when scrobbling is enabled. Which is less bad, but still not good.
>
> diff --git a/sonata/main.py b/sonata/main.py
> index 943a9f4..5b50904 100644
> --- a/sonata/main.py
> +++ b/sonata/main.py
> @@ -178,7 +178,7 @@ class Base(object):
> self.plugintabs = dict()
>
> self.config = Config(_('Default Profile'), _("by") + " %A " +
> _("from") + " %B", library.library_set_data)
> - self.preferences = preferences.Preferences(self.config,
> + self.preferences = preferences.Preferences(self, self.config,
> self.on_connectkey_pressed, self.on_currsong_notify,
> self.update_infofile, self.settings_save,
> self.populate_profiles_for_menu)
> --- a/sonata/preferences.py
> +++ b/sonata/preferences.py
> @@ -66,9 +66,10 @@ class Preferences():
> Many changes are applied instantly with respective
> callbacks. Closing the dialog causes a response callback.
> """
> - def __init__(self, config, reconnect, renotify, reinfofile,
> + def __init__(self, main, config, reconnect, renotify, reinfofile,
> settings_save, populate_profiles_for_menu):
>
> + self.main = main
> self.config = config
>
> # These are callbacks to Main
> @@ -689,6 +690,8 @@ class Preferences():
> if self.scrobbler.imported():
> self.config.as_enabled = checkbox.get_active()
> self.scrobbler.init()
> + if self.main.status:
> + self.scrobbler.handle_change_status('stop',
> self.main.status['state'], self.main.prevsonginfo, self.main.songinfo)
Can we somehow avoid this? This allows easy abuse of main.foo in the
future, when we'd like to promote more separate modules. I think its OK
to lose the scrobble (scribble?) for the track currently playing when we
enable scrobbling. Which further questions, should we add the playing
time that's already completed when we start-up mid-play?
> +
> + if prevstate != 'play' and state != 'stop':
> + # No need to check if the song is 30 seconds or longer,
> + # audioscrobbler.py takes care of that.
> + self.np(songinfo)
> + self.scrob_prev_time = time.time()
> + elif prevstate == 'play' and state != 'play':
> + self.scrob_playing_duration += time.time() - self.scrob_prev_time
> + if prevstate == 'stop':
> + # New track playing, prepare to submit it later.
> + if audioscrobbler is not None:
> + self.scrob_start_time = ""
> + self.scrob_playing_duration = 0
> + if self.config.as_enabled and songinfo:
> + self.scrob_start_time = str(int(self.scrob_prev_time))
Isn't self.config.as_enabled always true at this point? Why do we need
to test for songinfo when it isn't even used in the if block?
audioscrobbler should have been imported by now too.
I know you've just moved code around, but we can use this opportunity to
clean up code that is irrelevant.
> + elif state == 'stop':
> + # Track stopped playing, submit it if the submission criteria
> have
> + # been met. The song must have been playing for at least 4
> minutes
> + # or have been at least halfway into its total length to be
> submitted.
> + prevlen = float(mpdh.get(prevsonginfo, 'time'))
> + if self.scrob_playing_duration >= min(4*60, prevlen/2):
> + if self.scrob_start_time:
> + self.post(prevsonginfo)
What if I'm playing a stream without a 'time'? Do we need to test for
scrob_start_time or shouldn't it have already been set?
Now I think this patch is doing too much. Can we fix the problem that
started this whole long thread with only a few lines changed? Something
like this?
diff --git a/sonata/scrobbler.py b/sonata/scrobbler.py
index 6a8d104..452fd23 100644
--- a/sonata/scrobbler.py
+++ b/sonata/scrobbler.py
@@ -32,6 +32,7 @@ def __init__(self, config):
self.scrob_time_now = None
self.elapsed_now = None
+ self.got_time = True
def import_module(self, _show_error=False):
"""Import the audioscrobbler module"""
@@ -72,7 +73,9 @@ def init_thread(self):
def iterate(self):
"""Update the running time"""
- self.scrob_time_now = time.time()
+ if self.got_time:
+ self.scrob_time_now = time.time()
+ self.got_time = False
def handle_change_status(self, playing, prevsonginfo, songinfo=None,
switched_from_stop_to_play=None, mpd_time_now=None):
"""Handle changes to play status, submitting info as appropriate"""
@@ -106,6 +109,7 @@ def handle_change_status(self, playing, prevsonginfo,
songinfo=None, switched_fr
# Keep track of the total amount of time that the current song
# has been playing:
self.scrob_playing_duration += time.time() -
self.scrob_time_now
+ self.got_time = True
else: # stopped:
self.elapsed_now = 0
if prevsong_time:
_______________________________________________
Sonata-users mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/sonata-users
