search

Bleuzen / SpotRec

Spotify desktop client recorder for PulseAudio
MIT License
75 stars 17 forks source link

Add -c flag to count the track number in a 3digit wide playlist order #13

Closed thescooby closed 3 years ago

thescooby commented 3 years ago

Wanted to have the following features:

Bleuzen commented 3 years ago

Hi, thanks for your contribution! However there are changes required before getting this merged:

1.) The playlist track counter is dependent on the default filename pattern. This locks users out from using the --filename-pattern option, which is a problem.

2.) The sort_in_folders option seems unnecessary. As an alternative idea, this would be cleaner and more flexible: implementing it into the existing --filename-pattern option and auto create sub folders when slashes are found in the pattern. So using it could look like this:

./spotrec.py [...] --filename-pattern "{artist} - {album}/{trackNumber} - {title}"

Bleuzen commented 3 years ago

Forgot this last night, but now that I think of it: The playlist-counter should also be implemented in the existing --filename-pattern option, allowing for more flexible filename/-paths. This way, no new command line option is needed, keeping it simpler and not forcing any format (where the playlist-count is) in the filename.

thescooby commented 3 years ago

My changes:

Works now with e.g.: spotrec.py [..] -p "{artist} - {album}/{trackNumber} - {title}" and even with spotrec.py [..] -p "{artist} - {album}/2020/{trackNumber} - {artist} - {title}"

Cool. ;-) Thanks for mentioning this, it is a much cleaner design now.

The playlist numbering suffered from having too many on_playing_uri_changed() events. Strange. Therefore I put the playlist counter increasing into stop_blocking(). But even here I get sometimes very small files (10-30kB) and then spotrec starts again by a file that sometimes is already completely downloaded. Then the playlist counter jumps of course. I couldn't find the bug here this time, e.g. 001 - Dermot Kennedy - Outnumbered.flac (23 MB) 002 - Nico Santos - Play With Fire.flac (23,1 MB) 003 - Imagine Dragons - Bad Liar.flac (26 kB) 004 - Nico Santos - Play With Fire.flac (23,1 MB) 005 - Imagine Dragons - Bad Liar.flac (27,1 MB)

Bleuzen commented 3 years ago

Somehow GitHub doesn't like the force pushes. Looking at the summary: https://github.com/Bleuzen/SpotRec/pull/13/files for example the pl_track_counter variable is still under the hard coded ones, even while you moved it. Don't know yet if more is broken.

Bleuzen commented 3 years ago

I like the new implementation of the playlist track counter and the fact that if enabled it just replaces the existing trackNumber in the metadata/filename pattern, which is otherwise gotten from the album.

Description of the command line option should be changed to reflect that.

Also, what do you think about renaming it to "internal track counter" since it does exactly this? The problem with the playlist track counter name is that it can be misleading. One may think it does really get the current playlist index from spotify, but actually it is just counting up. Meaning users can't just skip some songs in the playlist and still expect the counter to reflect the correct playlist index. Therefore I think the option should be better named to what it actually is.

thescooby commented 3 years ago

https://github.com/Bleuzen/SpotRec/pull/13#issuecomment-780550277: I can not see pl_track_counter still under "hard coded", just disable the folding and then it is under the runtime variables. Regarding force-push: I looked again through the file, looks good so far. But maybe the "github folding" let the file look strange.

Bleuzen commented 3 years ago

Thank you, looks good now! Think this can be merged now. I will do some testing and if everything goes well this will land in a new release soon.

Bleuzen commented 3 years ago

Found one problem left: the internal_track_counter increment happens too late. So for example the first two songs recorded are prefixed with "001".

This is because stopping the recording does not happen immediately, it happens in this order:

For now I put the increment code here: https://github.com/Bleuzen/SpotRec/commit/12249865c545999dde3e9941ca9d497de517eae2

Which is not optimal as it counts every track change (also the ones not recorded), but at least it reliably increments at time and puts the output files in order.

thescooby commented 3 years ago

Found one problem left: the internal_track_counter increment happens too late. So for example the first two songs recorded are prefixed with "001".

This is because stopping the recording does not happen immediately, it happens in this order:

* spotrec updates metadata on start, putting in 001 as trackNumber

* The first track plays

* spotrec uses current count (001) for the recording

* The first track finishes

* spotrec detects a song change

* spotrec updates the metadata

* spotrec starts the new recording (of the second song) (track counter is still 001 here)

* the old recording (of the first song) is stopped (and this is the place where you currently increment the counter)

For now I put the increment code here: 1224986

Which is not optimal as it counts every track change (also the ones not recorded), but at least it reliably increments at time and puts the output files in order.

thescooby commented 3 years ago

Yes, because of getting triggered many times I put it in stop_blocking(). But you are right at least it counts up, I got really big jumps in the counter like e.g. 001 and the next was 016 etc. having it in on_playing_uri_changed().

Do you know why there are so many (irrelevant) track changes? When using dbus-monitor I can not see them.

Bleuzen commented 3 years ago

@thescooby Your changes are now released! :) https://github.com/Bleuzen/SpotRec/releases/tag/v0.14.0

FYI I made some more changes, for your feature: improved stability of the track counter (https://github.com/Bleuzen/SpotRec/commit/bdd1c2a4cd8ea5452c651359fe7aaaaca14463fb) and added example in the command line option help text