sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

DirViewer - show cover images from metadata #130

Closed xeruf closed 5 years ago

xeruf commented 5 years ago

Most of my FLAC and MP3 files have embedded cover images, but the dirViewer does not show them.

sghpjuikit commented 5 years ago

Not implemented. It can be done, but it is a little unusual. The cover belongs to the album, not song. I'll look into it, shoulnt be that hard. As for performance: cant promise anything.

sghpjuikit commented 5 years ago

Closing as delivered.

Implementation was indeed simple, although puts additional burden to the messy legacy code. Performance should be good unless the covers are really big. Unfortunately ImageCover.getImage(size: ImageSize) is not fully implemented, i.e., returns full image, thus higher memory consumption (as necessary) should be expected. Implementing that is out of scope.

I want to point out two things about this feature:

I do think this feature should be implemented, but I also think covers do not belong into the tag, and the use case right here right now would benefit user tremendously. If he avoids using tag for covers, application would load single cover from a file instead of reading the same cover per every file. This is in user's hands. Something to consider.

PS: the features 'use parents cover' and 'display cover from tag' do not conflict. The tag has higher priority.

xeruf commented 5 years ago

1) This is your choice. Which album has 100 songs? I have quite a lot of singles with their own artwork, do you propose to put each Single in a folder with its artwork as a separate file? Doesn't sound sensible. 2) I don't see this behaviour in current master 3) Please stop "closing as delivered". Instead, use the fixes syntax in a commit message, that also associates commit and issue. Or, if you forgot it, do it like this:

Fixed by 899e1f00c2215f95763d7f18d2b3262cd51b48f5

Anyways, it looks great now :)

sghpjuikit commented 5 years ago

1 I have albums with 200+ songs. In fact 2000+, but that one is an exceptional case (rip). Well, I agree the solutions are trade-offs.

2 It appears it does not work when the parent directory is the root of the view. Fixed.