momodalo / rubyripper

Automatically exported from code.google.com/p/rubyripper
3 stars 2 forks source link

musicbrainz: multiple artist cds misses artist infos #554

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
1) Please describe the steps to reproduce the situation:
a. Insert CD with multiple artists

2) What is the expected output? What do you see instead?
I expect Tracks with an Artist and a Track name. However, the Artist field is 
always empty.

3) What version of rubyripper are you using? On what operating system? The
gtk2 of commandline interface?
Current git, gtk2.

Original issue reported on code.google.com by hanno@hboeck.de on 5 Jan 2013 at 11:03

Attachments:

GoogleCodeExporter commented 8 years ago
I debugged into it and found that its not a musicbrainz-issue, same with cddb.

I finally found this function in gtkDisk.rb:
  def setVarArtist()
    return true if @md.various?
    @md.markVarArtist()
    @disc.audiotracks.times{|index| @varArtistEntryArray[index].text = @md.getVarArtist(index + 1)}
    @disc.audiotracks.times{|index| @trackEntryArray[index].text = @md.trackname(index + 1)}

This is somewhat bogus, because it gets only called if md.various is true. So 
it just returns true and the code never gets executed. I removed the "return 
true if @md.various?" and then everything works as expected.

Original comment by hanno@hboeck.de on 14 Jan 2013 at 12:26

Attachments:

GoogleCodeExporter commented 8 years ago
And while at it I found a second issue: If there are multiple artists on one 
track, rubyripper crashes. This seems to be a simple @artist vs. artist misuse, 
but please check if this patch makes sense. At least it "worksforme".

Original comment by hanno@hboeck.de on 14 Jan 2013 at 12:30

Attachments:

GoogleCodeExporter commented 8 years ago
This line prevents updating the disc to various if it's already marked as 
various. The real problem is, why isn't the various artist set in the metadata 
Data class? If you look here you see the function: def various? ; 
@varArtist.size > 0 ; end.

This should be fixed in the MusicBrainzReleaseParser class. Apparently the 
current musicbrainz code isn't using the setVarArtist(number, value) method 
currently if a various artist is available.

Original comment by boukewou...@gmail.com on 14 Jan 2013 at 7:25

GoogleCodeExporter commented 8 years ago
As I already said, the problem is not in the musicbrainz parser, because it 
happens with cddb as well.

I don't get how the current setVarArtist can be correct. Line 69 says:
      @md.setVarArtist(track, @varArtistEntryArray[track-1].text) if @md.various?
Also, line 283 says:
    setVarArtist() if @md.various?

So setVarArtist is both times only called if md.various is true. But the first 
line of setVarArtist is
    return true if @md.various?
Basically this turns setVarArtist into a "do nothing"-function, because it is 
only called if @md.various is true and it does nothing if @md.various is true.

And as I said, just removing that bogus line and it "just works".

Original comment by hanno@hboeck.de on 16 Jan 2013 at 11:03

GoogleCodeExporter commented 8 years ago
I can confirm that the solution by the reporter works (removing the return true 
if @md.various in setVarArtist).

I had the same issue with the latest git version (0.7.0a1) using freeDb as my 
cddb provider. CDs with multiple artists left the artist field blank. On 
toggling the various artist checkbox, the artists where visible in the title 
and on toggling it again stayed in the title (artist was filled with unknown).

The mentioned change fixed that. (Thanks ;))

Original comment by mathias....@gmail.com on 23 Dec 2013 at 7:46