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
mgallien | |
ngraham | |
astippich |
Elisa |
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
No Linters Available |
No Unit Test Coverage |
Buildable 19446 | |
Build 19464: arc lint + arc unit |
Make MediaTrackMetadataView open with width as argument. It opens with larger width for radios where http links can be long.
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 | ||
---|---|---|
2896 | 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). |
src/databaseinterface.cpp | ||
---|---|---|
2896 | 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 ? |
src/databaseinterface.cpp | ||
---|---|---|
2896 | The current solution was what I found enough to do the job. |
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. |
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.
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.
src/qml/ListBrowserDelegate.qml | ||
---|---|---|
196 ↗ | (On Diff #69363) | 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). |
src/qml/ListBrowserDelegate.qml | ||
---|---|---|
196 ↗ | (On Diff #69363) | 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. |
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.
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 | 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 | This is not a so good idea. Instead of blocking through a nested loop, please use an asynchronous API. |
Remove synchronous checking in the model, use qml image statusChanged to handle errors.
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 | ||
---|---|---|
239 | This is needed because when the image is going into error state and changed later on, the image would not get updated, right? |
src/qml/MediaTrackMetadataView.qml | ||
---|---|---|
239 | Yes exactly :) |
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 !
Integrated on master branch https://invent.kde.org/kde/elisa/commit/eb209bf8ae3d7f9dff4b93d5264f68fc4cec0913
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 :)