search

SecUpwN / Spotify-AdKiller

Your Party with Spotify - without ads!
https://github.com/SecUpwN/Spotify-AdKiller
GNU General Public License v3.0
836 stars 83 forks source link

fix ads not being muted due to pause state #63

Closed gmdfalk closed 7 years ago

gmdfalk commented 8 years ago

fixes #46 and #48

This change needs independent testing but it seems to work for me. The problem was liberal use of the spotify-dbus PlayPause action which toggles Spotify playback. It would sometimes pause Spotify when it shouldn't be paused (i.e. when an ad is playing) leading to unreliable ad tracking and playback state.

SecUpwN commented 8 years ago

@mikar, after merging your previous fixes, this PR seems to have conflicts. Please rebase it. Thanks!

gmdfalk commented 8 years ago

I fixed the merge conflict. Since this PR affects main functionality of Spotify-AdKiller it should be thoroughly tested by someone who uses this software on a regular basis.

Some examples of what might be unwanted changes: Example 1: Pausing an ad is now impossible. It will be immediately unpaused. I don't know if this is the desired behaviour since I haven't checked behaviour prior to this change. Example 2: Using the default configuration, personal music during ads will only be played once. However, if the personal/interlude music is shorter than the ad and a second ad starts playing after the first ad, the interlude music will start playing again when the second ad starts. I only used one 20-second mp3 to test this feature and haven't checked if you need to configure looping or what happens if there are multiple mp3s queued. Again, I haven't checked behaviour prior to this change so I'm not sure if this is desired behaviour.

SecUpwN commented 8 years ago

@OlegSmelov, please give branch mikar:fix-46-ad-is-not-muted a good test for merging. Thanks!

Micr0Bit commented 8 years ago

@mikar : what happens if i have no local music at all (no personal interlude) ? i mean on the first pause ... anyway , tested it a little bit and imo this bugfix fixes our current problems (i will continue to test it)

Micr0Bit commented 8 years ago

well , now i know what happens ... if i dont have a local track ...

its gonna use the "current songs" duration instead ... and overlapping the ad for that time ...

not good :/

OlegSmelov commented 7 years ago

Even if this doesn't solve those issues, using play and pause actions instead of a toggle seems a good idea.