[vlc-devel] [PATCH 2/3] qml: MultiCoverPreview: clean up repeater

[Alexandre Janniaux]

Hi,

Thank you for your comments,

On Tue, Oct 15, 2019 at 03:15:45PM +0200, Romain Vimont wrote:
> On Wed, Oct 09, 2019 at 07:11:23PM +0200, Alexandre Janniaux wrote:
> > From: Pierre Lamot <pierre at videolabs.io>
> >
> > albums.get should be removed and the count property is useless.
> > ---
> >  modules/gui/qt/qml/utils/MultiCoverPreview.qml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/modules/gui/qt/qml/utils/MultiCoverPreview.qml b/modules/gui/qt/qml/utils/MultiCoverPreview.qml
> > index 5fc3c345bd7..2dc9e3e2c61 100644
> > --- a/modules/gui/qt/qml/utils/MultiCoverPreview.qml
> > +++ b/modules/gui/qt/qml/utils/MultiCoverPreview.qml
> > @@ -33,6 +33,7 @@
> >
> >  import QtQuick 2.11
> >  import QtQuick.Layouts 1.3
> > +import org.videolan.medialib 0.1
> >  import "qrc:///style"
> >
> >  Item {
> > @@ -49,7 +50,6 @@ Item {
> >          rowSpacing: VLCStyle.margin_xxxsmall
> >
> >          Repeater {
> > -            property int count: albums.rowCount()
> >              model: Math.min(albums.rowCount(), 4)
>
> (not related to this patch)
> The model is just a number?

Yes, it's the genre cover drawer. The result is explained
at the top of the file in comments but doesn't work anymore
on the newer Qt versions, because it was using window
capture features from QML it should be rendered offscreen
or prepared before now.

> >
> >              /* One cover */
> > @@ -60,7 +60,7 @@ Item {
> >                  Layout.columnSpan: albums.rowCount() === 1 ? 2 : 1
> >                  Layout.fillHeight: true
> >                  Layout.fillWidth: true
> > -                source: albums.get(index).cover || VLCStyle.noArtAlbum
> > +                source: albums.data(albums.index(index, 0), MLAlbumModel.ALBUM_COVER) || VLCStyle.noArtAlbum
>
> (not related to this patch)
> IMO the QML should not call .data() directly like this (it won't react
> to all model changes), but use the role "cover" directly.

The genre might need to expose its own attribute interface
in the future for cover, but this is just refactoring out
the dependency to albums.get.

It cannot use the role "cover" directly here.

Regards,
--
Alexandre Janniaux
Videolabs