Cover Files Extraction Branch

[Matthieu Gallien]

On dimanche 18 novembre 2018 12:25:11 CET Alexander Stippich wrote:
> Hello Matthieu,
> 
> On Sonntag, 18. November 2018 10:56:01 CET you wrote:
> > Hello Alexander,
> > 
> > Thanks for your work on this.
> > 
> > On samedi 17 novembre 2018 18:01:18 CET Alexander Stippich wrote:
> > > Hello everyone,
> > > 
> > > I've pushed a branch called cover_manager.
> > > It contains a working solution for the extraction of cover images
> > > embedded
> > > in the files, but also does some more stuff. So here is a list:
> > > -reorder all platform-agnostic scanning files to a single folder
> > > -factors out the current code for cover searching into own class called
> > > cover manager
> > > -add tests and simple benchmarks
> > > -optimizations for cover searching
> > > -extract the cover art from music files
> > > -resize when image is too large (512x512 current maximum)
> > > -save all covers to standard data location, e.g. the same location where
> > > the database is located
> > 
> > I have done a quick review of the branch.
> > There are several blocking points.
> > 
> > I do not like that files and code unrelated to embedded cover images are
> > modified in the same branch. Please separate each topic you want to work
> > on.
> This is really just the first commit. I will cherry-pick it for separate
> review and will rebase afterwards.
> 
> > As far as I understand what you did, you are saving all cover images you
> > found (in directories or in metadata to a directory dedicated to store
> > application data).
> > Is my understanding correct ? In case yes, this is also a blocking point.
> > We should not store cover images in this type of directory. They are not
> > application data. We should also not duplicate existing files.
> 
> Your understanding is correct. The location of the data can easily be
> changed. I checked where Cantata is saving the files, and it uses the
> /.cache location. That's probably the best location, since after all they
> are used as a cache. Is that okay for you?

Qt provides a set of standard directory locations. Please have a look at 
QStandardPaths (QStandardPaths::CacheLocation is probably what you are looking 
for).

> Regarding the duplication of existing cover files in the folders, I also
> wasn't sure about this. But there are some good arguments in favor of it
> imho. They are scaled down automatically to the maximum size of 512x512
> currently (which may still be too large). Thus they will serve as a fast
> cache for the covers and loading of possibly large files is avoided. Having
> all cover files loaded from Elisa in one single place makes future features
> imho more easier to implement. This should greatly ease the implementation
> of a
> QQuickImageProvider later.
> And in case Elisa will download cover art automatically, this will also be
> the location.

Duplication local covers is a no go. This is not in the best interest of our 
users.
Covers embedded in metadata can be fed to Image qml elements without needing 
to be saved as a real file. In this case, creating such a file is also a no 
go.
You would need to manage their lifetime and that will require some amount of 
propertly tested code. This is a big work.

Without that, we will create files in the home directory of our users that 
will just accumulate for cases that could be done without them.

Please have a look at the following page, the described approach is simple and 
efficient ( https://forum.qt.io/topic/69209/how-to-display-a-qpixmap-qimage-in-qml/4 ).

Another solution could be to use a QQuickAsyncImageProvider derived class.

> Best regards,
> Alexander
> 
> > > Future things I would like to look at:
> > > -add a QQuickImageProvider for the cover manager (opinions?)
> > > -add functions to create artist pictures from the album covers (like in
> > > https://community.kde.org/KDE_Visual_Design_Group/Music_Player)
> > > -make maximum cover size configurable
> > > 
> > > I would appreciate any feedback. I _think_ currently it is necessary to
> > > delete the old database in order to get the new images, but this is an
> > > issue unrelated to this branch.
> > > 
> > > Best regards,
> > > Alexander
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > Elisa mailing list
> > > Elisa at kde.org
> > > https://mail.kde.org/mailman/listinfo/elisa
> > 
> > Best regards
> > 
> > --
> > Matthieu Gallien
> 
> _______________________________________________
> Elisa mailing list
> Elisa at kde.org
> https://mail.kde.org/mailman/listinfo/elisa

Best regards

--
Matthieu Gallien