Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Podcast support #54
base: master
Are you sure you want to change the base?
Podcast support #54
Changes from 4 commits
fd6150c
bab8af0
5714079
7dc7d4c
368a9b2
dd0ccd9
78e1f59
020433a
e0a2f6c
3ca83b6
0b277a9
dde2e3b
d4a9a14
767f371
a61317d
3d7189c
aa740c2
299813b
622f55f
c68063e
ce2b20e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time, because I assumed I needed to specify that it was an unsigned short instead of two bytes, but I suppose they're equivalent. I do think it's useful to indicate the actual type now that it'll change values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce the noise I would revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested and actually can't revert this. Packing the structure to build the database throws an exception because the struct has a
short
but is expectingbytes
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed H seems correct here. Can you give it the value
0xFFFF
?I am also wondering if a non-podcasts count makes sense. I guess a podcast count makes more sense, but that is what the docs from reverse engineering say. But maybe those are wrong!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically,
("H", b"\xFF\xFF")
would be accepted, but I think that just looks confusing, given that it's representing a number.I also don't think I'm going to question the docs. If it was actually a "podcast count", 0 would make more sense for the case when there aren't any podcast playlists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0xFFFF is more clear than the number. Maybe it is also a signed short, which would be -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to print that to the user? Also shouldnt the listtype be 3 instead of 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove that printout. The intention was to match the print level of other messages.
Also, this section is excluding podcasts from the All Songs playlist. That's a bit more clear with the enum, but I added a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not run the program for ages. If you say the print fits, we can keep it. But please use a capital
Not
at the beginning :-DI currently try to understand the code again. Isnt there a track object inside that playlist of which we can check if it is a podcast. But at another point we also accept id3 tags with Genre set to Podcast, but this code here does not support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a track object, just a path to the track. In order to detect by id3 at this point, the code would have to reference the track header and look up each track object, or reload the metadata with mutagen. I think that would be better handled by a bigger re-architecture that loads all data, building the database objects and then writes everything at once, rather than building up some data as the database is built.