Radios: add support for images
ClosedPublic

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

Details

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 21143
Build 21161: 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
2862

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
2862

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
2862

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.Nov 4 2019, 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.Nov 6 2019, 8:45 PM
  • Remove useless affectation in error handling
astippich added inline comments.Nov 7 2019, 5:48 PM
src/qml/ListBrowserDelegate.qml
172

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.Nov 7 2019, 6:17 PM
src/qml/ListBrowserDelegate.qml
172

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.

jguidon updated this revision to Diff 70532.Nov 28 2019, 10:25 PM
  • Url validation in models, before the data is sent to the view.

Thanks a lot for your feedback. I implemented some checking in elisautils, I hope this is the right place to put such a "global" method.
The code is more general in trackmetadatamodel, but there is still some specific case in DataModel::data for now.

I will make a rebase in the beginning of next week.

jguidon updated this revision to Diff 70842.Dec 3 2019, 6:27 PM

Rebase on master

jguidon updated this revision to Diff 70844.Dec 3 2019, 6:34 PM

Rebase: fix some not wanted changes

jguidon updated this revision to Diff 70861.Dec 3 2019, 9:29 PM

Image Url field was not present on creation mode

I would honestly prefer just to check within the QML side of things and display the default image in case of an error. This way, this would actually also catch issues with local tracks (in case an image was deleted or similar).
But @mgallien would like to check it before, so let's wait for his response.
Actually, I think the way the url checking is implemented is synchronous, e.g. blocking in case of no reply, if I am not mistaken. This should definitely be changed.

In any case, please follow the code style. Space after if statements:
if (...) {

src/elisautils.cpp
42 ↗(On Diff #70861)

If this is gonna be used, please use the new connect syntax. There are plenty of examples within Elisa.

src/qml/DataListView.qml
48

This can now be deleted altogether, right? It should be always true now.

@jguidon Thanks a lot for this new contribution. This is nice to see you continue to contribute on this part.

I did a quick general review and have one comment. Please have a look at it and do not hesitate if you have questions.

src/elisautils.cpp
44 ↗(On Diff #70861)

This is not a so good idea. Instead of blocking through a nested loop, please use an asynchronous API.
You can have a look at that https://stackoverflow.com/questions/35561182/why-should-nesting-of-qeventloops-be-avoided

jguidon updated this revision to Diff 72831.Jan 5 2020, 8:44 PM

Remove synchronous checking in the model, use qml image statusChanged to handle errors.

jguidon updated this revision to Diff 72833.Jan 5 2020, 8:56 PM
  • remove unused includes
Thanks a lot for this new contribution. This is nice to see you continue to contribute on this part.

Thanks, I will continue to contribute as I enjoy it, even if I may have busy periods like everyone :)

Thanks also for the reading about synchronous wait, I was also not happy with it and the solution brought some unecessary complexity.

I brought back the error handling in qml code and use some connections to set the model source when the user makes new changes.

I can rebase this work on master during the upcoming week.

Thanks, looking good! Just one question inline

src/qml/MediaTrackMetadataView.qml
246

This is needed because when the image is going into error state and changed later on, the image would not get updated, right?

jguidon added inline comments.Jan 14 2020, 6:38 PM
src/qml/MediaTrackMetadataView.qml
246

Yes exactly :)

jguidon updated this revision to Diff 73561.Jan 14 2020, 7:00 PM

Rebase on master

astippich accepted this revision.Jan 18 2020, 12:31 PM

good to go from my side

This revision is now accepted and ready to land.Jan 18 2020, 12:31 PM
mgallien accepted this revision.Tue, Jan 21, 9:11 PM

Thanks @jguidon !
Well done !

I will merge this manually given that Elisa repository is now managed through the server invent.kde.org using Gitlab.

I hope you will keep working on Elisa and the radio support !

Thanks @jguidon !
Well done !

Thanks !

I hope you will keep working on Elisa and the radio support !

Yes, I wanted this review done before going further :)