fix issues related to radio and view details in playlist
ClosedPublic

Authored by mgallien on Aug 19 2019, 10:20 PM.

Details

Summary

partial removal of isRadio and duplicated code in TrackMetadataModel

Test Plan

could not spot regressions

all automatic tests are OK except elisaqmltests that does not run due to an issue with a missing so dependency on KIO

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgallien requested review of this revision.Aug 19 2019, 10:20 PM
mgallien created this revision.
ngraham accepted this revision.Aug 20 2019, 2:21 AM
This revision is now accepted and ready to land.Aug 20 2019, 2:21 AM
astippich requested changes to this revision.Aug 20 2019, 5:41 AM

deleting a radio is not possible anymore, and upon opening the metadata view qml shows some warnings

This revision now requires changes to proceed.Aug 20 2019, 5:41 AM
mgallien updated this revision to Diff 64131.Aug 20 2019, 3:48 PM
  • fix radio deletion and warning messages

I just have two questions about the behavior:

  • Does it make sense to display the Delete button while we are creating a radio ?
  • The Apply button is not disabled after the radio is saved, maybe the user is not sure if the data has been successfully saved? I tried to keep the same behavior as the System Settings for instance.

I just have two questions about the behavior:

  • Does it make sense to display the Delete button while we are creating a radio ?
  • The Apply button is not disabled after the radio is saved, maybe the user is not sure if the data has been successfully saved? I tried to keep the same behavior as the System Settings for instance.

Thanks for the feedback. I will fix those two issues.

mgallien updated this revision to Diff 64251.EditedAug 21 2019, 8:25 PM
  • do not show delete button when creating a new radio
  • partially fix the radio modification case
  • final fix for radio modification
  • fix radio modification by querying all data of modified radio

fix issues reported by @jguidon

I may have two wrong test cases and some minor comments about the code :)

The Delete button does not reappear after a saving a new radio.

The Apply button has no action after these steps:

1- Create a new radio -> Apply
2- Keep the window opened, try to modify a field
3- it looks like the Apply button does not trigger saving any more, the radio data is not saved in the database.
src/qml/MediaTrackMetadataView.qml
33–37

Is this variable used ?

202

I guess we do not care if the data has been successfully saved in the database ? (maybe I think too much like a web application :) )

astippich accepted this revision.Aug 22 2019, 6:29 PM

Okay from my side if @jguidon's feedback is incorporated

This revision is now accepted and ready to land.Aug 22 2019, 6:29 PM
mgallien updated this revision to Diff 64772.Tue, Aug 27, 8:33 PM
  • use MediaTrackMetadataView.editableMetadata to choose delegates
  • fix modification of a newly created radio

fix some issues reported by @jguidon

I may have two wrong test cases and some minor comments about the code :)

The Delete button does not reappear after a saving a new radio.

The Apply button has no action after these steps:

1- Create a new radio -> Apply
2- Keep the window opened, try to modify a field
3- it looks like the Apply button does not trigger saving any more, the radio data is not saved in the database.

This issue is fixed but deletion of a newly created radio is not working.

mgallien added inline comments.Tue, Aug 27, 8:37 PM
src/qml/MediaTrackMetadataView.qml
202

I do not think that update of a radio can reasonably fail no matter which data is set.

One nice addition could be a check for field length if it makes sense. At least for me, this is a subject for a latter patch.

What is the status here?

This issue is fixed but deletion of a newly created radio is not working.

Should I wait for it to be fixed before reviewing ?

Also, I locally made some work to add album image support in the radio view based on the work of this review. I will wait for this review to be landed before pushing a new one.

This issue is fixed but deletion of a newly created radio is not working.

Should I wait for it to be fixed before reviewing ?

Also, I locally made some work to add album image support in the radio view based on the work of this review. I will wait for this review to be landed before pushing a new one.

I will soon finish work on this and will post an update. Sorry for the delay.

mgallien updated this revision to Diff 65663.Sun, Sep 8, 8:32 PM
  • fix radio deletion and warning messages
  • do not show delete button when creating a new radio
  • partially fix the radio modification case
  • final fix for radio modification
  • fix radio modification by querying all data of modified radio
  • use MediaTrackMetadataView.editableMetadata to choose delegates
  • fix modification of a newly created radio
  • show delete button once the new radio has been created
  • fix deletion of a newly created radio
  • fix failling automatic tests

Should be good to land now.

Let me know.

Thanks @astippich for the ping on this

Should be good to land now.
Let me know.

Yes, I do not see anything wrong :)

This revision was automatically updated to reflect the committed changes.

Should be good to land now.
Let me know.

Yes, I do not see anything wrong :)

Thanks for your feedback.