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.

Sign up for GitHub

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

Accept both #AUDIO and #MP3 as header tag for the audio file. #789

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Accept both #AUDIO and #MP3 as header tag for the audio file. #789

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/base/USong.pas
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,8 +1124,8 @@ function TSong.ReadTXTHeader(SongFile: TTextFileStream; ReadCustomTags: Boolean)
Done := Done or 2;
end

//MP3 File
else if (Identifier = 'MP3') then
//Audio File (MP3 is deprecated in favor of AUDIO)
else if (Identifier = 'AUDIO') or (Identifier = 'MP3') then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure this will work if we define either MP3 or AUDIO, but what happens in scenarios like this:

  • first AUDIO, then MP3, but AUDIO is a non-existing file
  • first AUDIO, then MP3, but MP3 is a non-existing file
  • first AUDIO, then MP3, but neither exists
  • the above three, but MP3 is earlier in the txt file
  • is any of this dependent on the existence and, if it exists, the value of a VERSION tag

what kind of behaviour should the error messages have?

in all of the above scenarios (plus if the file only defines one of them): what if we edit it in the editor and want to save it? what should it write?

Especially that last one is why I requested changes, because the proposed PR implementation will actually destroy one of the tags! (I think? I haven't actually compiled it but I'm pretty sure it does. Correct me if I'm wrong though) See below for a longer explanation. Ideally all that refactoring comes first in a new PR, and only once that's in do we come back to this one. Said refactoring doesn't need to be 100% perfect straight away, but the bulk of it should not be part of a PR that also adds new functionality.

It's a bit of a chicken-egg problem, but I think one thing is very clear: the current way of reading/validating/writing tags is insufficient going forward. I think it would be better to first redo the current behaviour (no AUDIO handling yet) into something like this:

  • read ALL tags into some kind of dictionary, but without any validation applied
  • figure out what version/tag handling logic we actually need to apply (this step wouldn't do much initially)
  • actually handle the tags, do validation, etc

this does assume that there currently isn't some sneaky/hacky behaviour where the order of the tags matters.

I get that the proposed minimal change would make it somewhat work in the short term, but we're going to need that version logic at some point anyway. Might as well do it before we start adding stuff. It's possible that this also means big changes to add some extra layer of abstraction/subclassing/overriding in what is currently USong, because the rest of USDX should just be able to do "give me the audio file" and "save this" and not have to care about how it was actually represented in the txt.

PS I'm not actually sure if my proposed refactoring will solve every situation in the future. But at the very least it would split out the parsing and the validating, which is still an improvement.
PS2 if this entire functionality does depend on a VERSION tag, this also opens up a whole new can of worms because of the special treatment of P3 which you probably don't want at some point anymore.
PS3 if this kind of refactoring is a bit too much Pascal, I'll do it once I find some time. There's lots of places in USDX in need of it, but if this one specifically is preventing people from contributing, obviously it's going to get more priority.

begin
EncFile := DecodeFilename(Value);
if (Self.Path.Append(EncFile).IsFile) then
Expand Down