Radios: add support for images
Needs ReviewPublic

Authored by jguidon on Sep 10 2019, 10:03 PM.

Details

Reviewers
mgallien
ngraham
astippich
Group Reviewers
Elisa
Summary

Album images where disabled for radios. This patch adds support for images in the radio view. A field has been added to EditableMetaData to allow the user to choose the image he wants for a radio. It can be a local file or an url.

Linked to: T7567

Diff Detail

Repository
R255 Elisa
Branch
RadiosImages
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18545
Build 18563: arc lint + arc unit
jguidon requested review of this revision.Sep 10 2019, 10:03 PM
jguidon created this revision.
jguidon updated this revision to Diff 65811.Sep 10 2019, 10:10 PM
  • Fixed typo
jguidon updated this revision to Diff 66029.Sep 14 2019, 9:33 AM

Make MediaTrackMetadataView open with width as argument. It opens with larger width for radios where http links can be long.

Some screenshots:

  • image with valid http address:
  • with wrong http address:
jguidon edited the summary of this revision. (Show Details)Sep 14 2019, 9:35 AM

Sorry for the delay.
Thanks for that. The screen capture looks really nice with the images for radios.
I have to check another thing to complete my review but please find my inline comment.

src/databaseinterface.cpp
2849

Even though this branch has not been released, can you bump the database version and do this modification via a database upgrade. That will be nicer for people using builds from trunk (think of early testers via the flatpak package built by KDE for example).

jguidon updated this revision to Diff 67038.Sep 29 2019, 4:55 PM
  • Set a database upgrade
  • Rebase on master
jguidon updated this revision to Diff 67040.Sep 29 2019, 4:58 PM
  • Fixed wrong modification in databaseV14
jguidon added inline comments.Sep 29 2019, 5:02 PM
src/databaseinterface.cpp
2849

Thanks for the feedback.

I wonder if there is a particular reason for creating some tables to check the database version? (DatabaseVersionV13, DatabaseVersionV14 ...). Maybe we could use on table with a version field? It could be more efficient/readable in DatabaseInterface::initDatabase ?

mgallien added inline comments.Sep 29 2019, 6:50 PM
src/databaseinterface.cpp
2849

The current solution was what I found enough to do the job.
Feel free to propose another solution via a review, I will be happy to let that evolve.

Sorry for the late reply.
To be honest, I don't know how useful this is when the image must be added manually. Ideally, this would be retrieved automatically. However, this could provide the base for such a feature.

One issue I noticed is that local image files are not loaded correctly.
Also, is the resetAlbumImage call really necessary? Can this be implemented with the normal saveData call?

src/qml/MediaTrackMetadataView.qml
38

this looks to specific. why is this needed?

Thanks for your feedback,

To be honest, I don't know how useful this is when the image must be added manually. Ideally, this would be retrieved automatically. However, this could provide the base for such a feature.

I agree about the fact that images should be retrieved automatically in some way. We should define how, what do you think ?
I also think it can be useful if the user adds its how radio and wants a particular image to visually identify it in the list.

One issue I noticed is that local image files are not loaded correctly.

It is because it needs "file://" as a prefix (for instance file://tmp/image.png instead of /tmp/image.png), I can push a patch in this revision later.

Also, is the resetAlbumImage call really necessary? Can this be implemented with the normal saveData call?

resetAlbumImage is called by onStatusChanged in the Image qml object, in case anything goes wrong with the image address entered by the user. It was an "easy" way to handle errors without implementing some image fetching test from scratch. Maybe would you have another idea ?

src/qml/MediaTrackMetadataView.qml
38

It is used to make the window larger depending on the case. The default value is overrided in DataListView.qml l.52.

In the case of radios, the window is then larger as most of the time urls are larger than classic music fields.

What is the current status of this review?

Thanks for your feedback,

To be honest, I don't know how useful this is when the image must be added manually. Ideally, this would be retrieved automatically. However, this could provide the base for such a feature.

I agree about the fact that images should be retrieved automatically in some way. We should define how, what do you think ?
I also think it can be useful if the user adds its how radio and wants a particular image to visually identify it in the list.

I don't know if the streams provide a picture in the metadata.
But I guess we have to somehow retrieve from the web, which is not supported yet in Elisa. As I said, I think this can be a useful first step implementing this, so I am not opposed merging this.

One issue I noticed is that local image files are not loaded correctly.

It is because it needs "file://" as a prefix (for instance file://tmp/image.png instead of /tmp/image.png), I can push a patch in this revision later.

Also, is the resetAlbumImage call really necessary? Can this be implemented with the normal saveData call?

resetAlbumImage is called by onStatusChanged in the Image qml object, in case anything goes wrong with the image address entered by the user. It was an "easy" way to handle errors without implementing some image fetching test from scratch. Maybe would you have another idea ?

I have not looked into it, but my feeling is that this can be achieved resetting the image url and calling the saveData afterwards.

jguidon updated this revision to Diff 69292.Mon, Nov 4, 8:38 PM

Fix binding loop qml message and add file:// to a local uri, if it is not an url.

I don't know if the streams provide a picture in the metadata.

We can extract libvlc_meta_ArtworkURL from the stream, but from the streams I tested the field was always empty.

I have not looked into it, but my feeling is that this can be achieved resetting the image url and calling the saveData afterwards.

If I understand it well, what I did is reset the displayed image if the input is not a correct image, while the data entered is saved whatever it is right or not.
This way the user sees there is something wrong and he can still see in the field what he has entered.

jguidon updated this revision to Diff 69363.Wed, Nov 6, 8:45 PM
  • Remove useless affectation in error handling
astippich added inline comments.Thu, Nov 7, 5:48 PM
src/qml/ListBrowserDelegate.qml
196

I've been thinking about this a little bit, since I don't like the special handling for the images here. I think it would actually be much simpler to change the source of the image to the default one in case of an error. This would remove the need for the extra function, and it would actually allow the user to edit the image url (e.g. in case of a typo).
Same in the metadata view.
Does this sound sensible?

mgallien added inline comments.Thu, Nov 7, 6:17 PM
src/qml/ListBrowserDelegate.qml
196

I agree, it is better not to special case for radio. I have been removing the special cases and improving the common code to be able to handle radio correctly without if everywhere.

Would it be possible to do some kind of validation directly in the text field ? It might be possible to verify that the URL is well formed and does not do 404.