Skip to content
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

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

Merged
merged 7 commits into from
Feb 17, 2021

Conversation

thescooby
Copy link
Contributor

@thescooby thescooby commented Feb 6, 2021

Wanted to have the following features:

  • The files shall be sorted into folders like <artist> - <album>
  • The files shall have a file name prefix (e.g. 012_*) that counts up per playlist track.
    This way the files are stored in the same order on disk as they are in the playlist.

spotrec.py Outdated Show resolved Hide resolved
@Bleuzen
Copy link
Owner

Bleuzen commented Feb 16, 2021

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}"

@thescooby thescooby changed the title Add a and p flag Add -c flag to count the track number in a 3digit wide playlist order Feb 17, 2021
@Bleuzen
Copy link
Owner

Bleuzen commented Feb 17, 2021

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.

spotrec.py Outdated Show resolved Hide resolved
@thescooby
Copy link
Contributor Author

thescooby commented Feb 17, 2021

My changes:

  • removed '-a' flag, did not fully understand the filename-pattern feature completely at first, but feature had issues with folders specified
  • fixed this folder handling

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

This comment has been minimized.

@Bleuzen
Copy link
Owner

Bleuzen commented Feb 17, 2021

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

This comment has been minimized.

spotrec.py Outdated Show resolved Hide resolved
spotrec.py Outdated Show resolved Hide resolved
spotrec.py Show resolved Hide resolved
@Bleuzen
Copy link
Owner

Bleuzen commented Feb 17, 2021

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 Bleuzen merged commit 04fe979 into Bleuzen:master Feb 17, 2021
@thescooby thescooby deleted the add_a_and_p_flag branch February 17, 2021 15:09
@Bleuzen
Copy link
Owner

Bleuzen commented Feb 17, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

thescooby commented Feb 17, 2021

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
Copy link
Owner

Bleuzen commented Feb 17, 2021

@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 (bdd1c2a) and added example in the command line option help text

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants