eonpatapon / mpDris2

MPRIS V2.1 support for mpd
GNU General Public License v3.0
211 stars 46 forks source link

Fix #95 #96

Closed ArsenArsen closed 7 years ago

ArsenArsen commented 7 years ago

The issue in #95 was that the wrong tag was used for covers. After changing it, works like a charm

EDIT: @I-Al-Istannen helped me debug and fix it

grawity commented 7 years ago

Hmm, either this is specific to your MP3 files, or to the mutagen version... In other words, won't it break cover lookup for files which did work previously? (For example, I don't remember ever having such issues with files tagged by foobar2000 or Quod Libet.)

So IMHO it would be better to make this more generic and scan all APIC tags available, instead of expecting to find a specific name.

I-Al-Istannen commented 7 years ago

@grawity The format seems to be APIC:<Description>, where Description is the name associated with the cover as found here.

So yes, I'd agree it would make more sense to use any APIC provided as the standard seems to not specify a specifc value: Description is a short description of the picture, represented as a terminated text string.

You probably just had two programs storing no description, the program in question here stored "Frontcover" and the next might store "A cute bunny".

Should the pull request be altered accordingly?

grawity commented 7 years ago

Yes, and perhaps clarify the commit message while you're at it... (See the existing log for examples.)

ArsenArsen commented 7 years ago

So, to recap, should the first found APIC:* tag be used if there is no Frontcover one?

I-Al-Istannen commented 7 years ago

@ArsenArsen Using the first APIC:* tag that is of type 3 (Frontcover) sounds like the best solution. Just ignore the description altogether and use the provided type instead.

ArsenArsen commented 7 years ago

Any updates? I imagine a fix for this issue would be rather useful.

eonpatapon commented 7 years ago

I think it looks fine, but you could merge all commits into one and put a better description in the commit message

grawity commented 7 years ago

I was busy with other work. The final code looks fine; I'll just let GitHub squash into one commit when merging.