mpatric / mp3agic

A java library for reading mp3 files and reading / manipulating the ID3 tags (ID3v1 and ID3v2.2 through ID3v2.4).
MIT License
1.2k stars 309 forks source link

Inconsistent error behaviour while saving a file back to its source #139

Open Aehrraid opened 7 years ago

Aehrraid commented 7 years ago

Now:

    Mp3File mp3file = new Mp3File("mpthreetest.mp3");

    ID3v2 id3v24Tag = new ID3v24Tag();
    mp3file.setId3v2Tag(id3v24Tag);
    id3v24Tag.setArtist("Test");

    mp3file.save("mpthreetest.mp3");

(Error: java.lang.IllegalArgumentException: Save filename same as source filename)

    File file = new File ("mpthreetest.mp3");
    Mp3File mp3file = new Mp3File(file);

    ID3v2 id3v24Tag = new ID3v24Tag();
    mp3file.setId3v2Tag(id3v24Tag);
    id3v24Tag.setArtist("Test");

    mp3file.save("mpthreetest.mp3");

(legal, but produces mp3s with artifacts)

Should be: Both cases illegal: Error: java.lang.IllegalArgumentException: Save filename same as source filename

Took me quite a few hours to figure out why there are breaks and bumps in my song after converting it. 👎

Keep up the good work!

solimant commented 6 years ago

@Aehrraid there's an existing test case for this, and it passes as expected; i.e. an IllegalArgumentException is thrown. Can you elaborate?

Aehrraid commented 6 years ago

Note the signatures mp3file.save(String) instead of mp3file.save(File).

The test case I mean would be (sort of):

public void shouldThrowExceptionIfSavingMp3WithSameNameAsSourceFileForFileConstructor() throws Exception {
    Mp3File mp3File = new Mp3File(new File(MP3_WITH_ID3V1_AND_ID3V23_AND_CUSTOM_TAGS));   //<-- File
    testShouldThrowExceptionIfSavingMp3WithSameNameAsSourceFile(mp3File.getPath()); //<-- String
}

This should fail.

Can you validate this? Is this relevant? I see, that using File once and String at the other place is bad style.