Review Request: Some changes to the handling of cover art reading and writing

[Daniel Faust]

> On April 10, 2012, 4:57 p.m., Matěj Laitl wrote:
> > I agree that current hard-coded value of 200px is a bit suboptimal. But I doubt there is a significant group of users that makes use of multiple per-file covers. When I order Amarok to replace track cover, I want Amarok to "make this picture the one and only cover". I would suggest raising the dafaut size to something more reasonable like 500px (storage is really cheap these days) and making it a hidden confing-file-only option documented in the handbook. Alternatively, I have a mockup of perhaps rather ugly extension to the config dialog that wouldn't get in the way. (I will attach soon)
> > 
> > The work of cleaning up cover file reading and writing and fixing its bugs is of course welcome.

>When I order Amarok to replace track cover, I want Amarok to "make this picture the one and only cover"
That's what I want, too. But it's not the current behavior that's why I implemented the configuration option.

>making it a hidden confing-file-only option
The problem with hidden options is that nobody will know they are there. But still better than no option at all.

>Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this
You are right, it isn't perfect but it would be a shame to stop implementing features because the config dialog gets overcrowded.


- Daniel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104513/#review12303
-----------------------------------------------------------


On April 10, 2012, 5:01 p.m., Daniel Faust wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104513/
> -----------------------------------------------------------
> 
> (Updated April 10, 2012, 5:01 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> This patch includes a list of small changes that are supposed to give the user more control over the cover writing process and unify it for all file formats.
> 
> 1. The user can set the maximum cover size in the config dialog instead of having a fixed 200px. (BR 279493)
> 2. The user can set how existing covers should be handled:
>    - 'Replace existing front covers' (almost the current behavior except for m4a files and some cases where more than one 'front' cover exists)
>    - 'Replace all existing covers'
>    - 'Append new cover' (actually it's a prepend, doesn't work with wma files, though; the current behavior for m4a files)
> 3. Unify the reading of covers. Instead of searching for the biggest cover like it was implemented for some file formats from now on the first 'front' cover will be taken. If there is no 'front' cover, the first 'other' cover will be taken (if present). The 1kb limit is still present.
> 4. Fix a potential bug where covers couldn't be found in mp3 files if the first cover was neither a 'front' cover nor 'other' or smaller than 1kb.
> 
> 
> This addresses bug 279493.
>     https://bugs.kde.org/show_bug.cgi?id=279493
> 
> 
> Diffs
> -----
> 
>   shared/tag_helpers/ASFTagHelper.cpp 93e6031 
>   shared/tag_helpers/ID3v2TagHelper.cpp 27e0cf0 
>   shared/tag_helpers/MP4TagHelper.cpp faeae0a 
>   shared/tag_helpers/VorbisCommentTagHelper.cpp 1fbb437 
>   src/amarokconfig.kcfg 5610c4a 
>   src/core-impl/collections/db/sql/SqlMeta.cpp e663adf 
>   src/dialogs/CollectionSetup.h 3146f17 
>   src/dialogs/CollectionSetup.cpp f1b7850 
> 
> Diff: http://git.reviewboard.kde.org/r/104513/diff/
> 
> 
> Testing
> -------
> 
> I have tested writing covers with flac, mp3, and wma files.
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/104513/s/508/
> Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this.
>   http://git.reviewboard.kde.org/r/104513/s/514/
> Mockup of a possible GUI to configure cover size. I'm not a big fan of this.
>   http://git.reviewboard.kde.org/r/104513/s/515/
> 
> 
> Thanks,
> 
> Daniel Faust
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120411/eb4fcdc8/attachment.html>