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

Jump to bottom

added TXXX based tags support #122

Open

Code0987 wants to merge 11 commits into mpatric:master from Code0987:master

+407 −11

Code0987 commented Mar 23, 2017

Added a new class for TXXX style tags which have description and value as


<Header for 'User defined text information frame', ID: "TXXX">
Text encoding    $xx
Description    <text string according to encoding> $00 (00)
Value    <text string according to encoding>


          added TXXX based tags reader support

3a76bbc

Collaborator

hennr commented Apr 1, 2017

thanks for your PR.

Could you please make this patch compile on travis-ci? There is an import used that is not available in the project.
Also please add a test for your new code.

Collaborator

hennr commented Apr 1, 2017

Related to #55


          fixed import issue

43a4827

coveralls commented Apr 5, 2017

Coverage decreased (-3.2%) to 60.007% when pulling 43a4827 on Code0987:master into 368fe13 on mpatric:master.

Author

Code0987 commented Apr 5, 2017

Import fixed.

I'll write tests asap. Currently I'm not familiar with java tests much.

Collaborator

hennr commented Apr 5, 2017

Let me know if you have any questions regarding java / JUnit testing, @Code0987


          updates,

7c46d3b

added tests

Author

Code0987 commented Apr 9, 2017

@hennr I've added test and updated code also. But I'm not sure if it's right!

Collaborator

hennr commented Apr 10, 2017

yep, there are some things that should be changed. Add me as a collaborator to your forked project and we can work together on them.

hennr added 2 commits

April 10, 2017 22:30


          merge upstream master

cc8c12a


          resolve merge conflicts

4f68fc6

hennr reviewed

View reviewed changes

pom.xml Outdated

@@ @@ -46,6 +46,12 @@ @@
               			<version>4.12</version>
               			<scope>test</scope>
               		</dependency>
+              	 <dependency>

Collaborator

hennr Apr 10, 2017

this dependency is never used

coveralls commented Apr 10, 2017

Coverage increased (+0.7%) to 62.093% when pulling 4f68fc6 on Code0987:master into 3bd2057 on mpatric:master.

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+              import org.junit.Test;
+              import static org.junit.Assert.*;
+              /**

Collaborator

hennr Apr 10, 2017

This info is already held in the git history, please remove

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+               */
+              public class ID3v2TXXXFrameDataTest {
+                  public ID3v2TXXXFrameDataTest() {

Collaborator

hennr Apr 10, 2017

unneeded

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+                  public ID3v2TXXXFrameDataTest() {
+                  }
+                  @BeforeClass

Collaborator

hennr Apr 10, 2017

Implementing these annotations are optional. Simply delete all of the if empty.

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+                  public void tearDown() {
+                  }
+                  @org.junit.Test

Collaborator

hennr Apr 10, 2017

@test without the complete path should work.

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+                  @org.junit.Test
+                  public void test() throws Exception {
+                      System.out.println("test for txxx frame");

Collaborator

hennr Apr 10, 2017 •

edited

Loading

please remove the system.out

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+                  }
+                  @org.junit.Test
+                  public void test() throws Exception {

Collaborator

hennr Apr 10, 2017

Please describe what the test ensures and rename test() according.

hennr reviewed

View reviewed changes

src/test/java/com/mpatric/mp3agic/ID3v2TXXXFrameDataTest.java Outdated

+                              "my_custom_text",
+                              "value",
+                              true);
+                      assertEquals(1, frameSets.size());

Collaborator

hennr Apr 10, 2017

It is a best practice to have one assertion per @test annotated method.
The method-/test name should describe what it ensures.


          rm unused dependency

051486f

Collaborator

hennr commented Apr 10, 2017

@Code0987 I merged the upstream master into your fork and added some comments in the test.
Could you update the test please?

coveralls commented Apr 10, 2017

Coverage increased (+0.7%) to 62.093% when pulling 051486f on Code0987:master into 3bd2057 on mpatric:master.


          updates tests

aaf7b5f

Author

Code0987 commented Apr 11, 2017

I followed your comments and have updated tests, let me know anything else to do.

coveralls commented Apr 11, 2017

Coverage increased (+0.7%) to 62.093% when pulling aaf7b5f on Code0987:master into 3bd2057 on mpatric:master.


          cosmetique

cdfc523

coveralls commented Apr 11, 2017

Coverage increased (+0.7%) to 62.093% when pulling cdfc523 on Code0987:master into 3bd2057 on mpatric:master.

Collaborator

hennr commented Apr 11, 2017

Thanks @Code0987
Did you have a specific reason why you implemented most public methods in ID3v2TXXXFrameData as static methods?
I would prefer to use instance methods and spare the useFrameUnsynchronisation parameter by re-using the unsynchronisation variable already held in the super class.

Author

Code0987 commented Apr 11, 2017

@hennr No, I was just trying to keep all related code together, as it was a little different for custom tags then all other tags already implemented. I was using your work in a android project using gradle dependency, so I just copied this TXXX extra file, since I wanted not to overwrite over your code, that would break auto-updating and maybe license too.

Collaborator

hennr commented Apr 11, 2017

OK, so we can change that to instance methods now?

Collaborator

hennr commented Apr 11, 2017

Another question:
If your code goes upstream the API for most users would be something like this, right?

// given
Mp3File mp3File = new Mp3File("src/test/resources/v23tagwithchapters.mp3");
ID3v2 id3tag = mp3File.getId3v2Tag();
assertNull(id3tag.getCustomText("foo"));

// when
id3tag.setCustomText("foo", "bar");

// then
assertEquals("bar", id3tag.getCustomText("foo"));

This does not work for me at the moment.
Could you add a test that uses the setter and getter of AbstractID3v2Tag? ID3v2TagTest would be a good place for that IMO.

Author

Code0987 commented Apr 12, 2017

Ok, I'll see and update.

Author

Code0987 commented Apr 12, 2017

@hennr
Yes we can change it. I'll update.
And I'll update test also.


          tests updated

66bfa17

bug fix

coveralls commented Apr 13, 2017

Coverage increased (+1.03%) to 62.439% when pulling 66bfa17 on Code0987:master into 3bd2057 on mpatric:master.

Collaborator

hennr commented Apr 19, 2017 •

edited

Loading

Hi @Code0987 , thanks for fixing the code.

Are you willing to change the static methods as well? If so, @mpatric should have another look at this pull request.
Cheers!

mpatric reviewed

View reviewed changes

src/main/java/com/mpatric/mp3agic/ID3v2TXXXFrameData.java Outdated

+              	protected void unpackFrameData(byte[] bytes) throws InvalidDataException {
+              		int marker = BufferTools.indexOfTerminatorForEncoding(bytes, 1, bytes[0]);
+              		description = new EncodedText(bytes[0], BufferTools.copyBuffer(bytes, 1, marker - 1));

Owner

mpatric May 29, 2017

If the description has zero length, the marker position could be 0, in which case the copyBuffer will fail. I suggest adding something like what is done in ID3v2UrlFrameData, which checks if marker >= 0 before doing the copy, otherwise sets the description to an empty string.

mpatric reviewed

View reviewed changes

src/main/java/com/mpatric/mp3agic/ID3v2TXXXFrameData.java Outdated


		marker += description.getTerminator().length;

		value = new EncodedText(bytes[0], BufferTools.copyBuffer(bytes, marker, bytes.length - marker));

Owner

mpatric May 29, 2017

As above, check if bytes.length - marker >= 0

mpatric reviewed

View reviewed changes

src/main/java/com/mpatric/mp3agic/ID3v2TXXXFrameData.java Outdated

+              		if (getClass() != obj.getClass()) {
+              			return false;
+              		}
+              		return false;

Owner

mpatric May 29, 2017

Is there a reason why the equals method is not fully implemented? It's returning false for separate object instances that contain the same description and value, but it should return true. Your IDE should be able to generate it for you.

Owner

mpatric commented May 29, 2017

To be consistent with how the code is currently structured, the static methods on ID3v2TXXXFrameData should rather be instance methods on AbstractID3v2Tag (even though AbstractID3v2Tag is getting too big... a refactor here is probably long overdue..)

Code0987 added 2 commits

June 2, 2017 01:34


          updated

663ba70


          Merge remote-tracking branch 'upstream/master'

9ef8e1f

* upstream/master:
  introduce changelog.md and prefer id3v2 over id3v1 for genre and genre description; closes mpatric#120 (mpatric#124)

coveralls commented Jun 1, 2017

Coverage increased (+7.8%) to 69.207% when pulling 9ef8e1f on Code0987:master into 00a620b on mpatric:master.

coveralls commented Jun 1, 2017

Coverage increased (+7.8%) to 69.207% when pulling 9ef8e1f on Code0987:master into 00a620b on mpatric:master.

dargmuesli commented Jun 17, 2022

@mpatric @hennr What's the state on this?

de5car7es commented Nov 20, 2022

Hi

Its really too bad that you still did not approved this pull request.
It works great and I encourage you to combine it.

Is there anything I can do to help in this matter ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet