Allow to undo the clear actions on the playlist
ClosedPublic

Authored by jguidon on Mar 8 2019, 2:09 PM.

Details

Summary

When a clear action is performed on the playlist, the user might lose valuable work.
Add the possibility to undo this operation via passive notification.
Keep it simple by allowing only one undo and no redo.

  • The patch now enables to restore the playlist and the player status.

For the notification, I use KNotification here, you can see in the screenshot the notification with an Undo button.

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.
jguidon requested review of this revision.Mar 8 2019, 2:09 PM
jguidon created this revision.

Hi,

you can see three screenshots:

  • Before:
  • Just after the clear playlist action with the notification:
  • After the undo action:

Best regards,
Jerome

mgallien added a project: Elisa.
ngraham added a subscriber: ngraham.Mar 8 2019, 5:08 PM

Instead of using a system notification, I would recommend showing a Kirigami InlineMessage at the bottom of the playlist pane with an Undo button in it.

Thanks, I can take a look at the Kirigami InlineMessage.

jguidon updated this revision to Diff 53504.Mar 9 2019, 10:35 AM

Use Kirigami InlineMessage instead of the system notification

Here is one screenshot with the inline message :

Here is one screenshot with the inline message :

This is very nice looking and I will try to review it properly as fast as I can.
Thanks

mgallien requested changes to this revision.Mar 10 2019, 8:56 PM

Thanks for your work, I have been able to successfully test it.

I have a few comment on the behavior.

When the playlist is cleared by using the replace button (clear the playlist ad add new music) the notification is not shown. Please, add support for undo in this case as an user could press the wrong button and could appreciate the undo feature.

Once one as pressed the close button on the notification, Elisa needs to be restarted to show one again. Please fix that.

This revision now requires changes to proceed.Mar 10 2019, 8:56 PM
jguidon updated this revision to Diff 53759.Mar 12 2019, 9:22 PM
  • For Inline Message, use of Kirigami.Action's button instead of InlineMessage closeButton
  • Added support for undo in case of replace

Thank you for your feedback !

Here is a new capture :

I used another action button because the integrated closeButton of the Inline Message had a strange behavior. I could spend more time to use it but the action button does not look so bad I think :)

Thank you for your feedback !

Here is a new capture :

I used another action button because the integrated closeButton of the Inline Message had a strange behavior.

In general we shouldn't work around problems in our own upstream components. I would recommend using the default close button and looking into fixing whatever problem you encountered (it's likely that others have encountered it too, so it would make sense to fix universally).

jguidon updated this revision to Diff 53875.Mar 14 2019, 11:23 AM

Fixed use of the closeButton of Kirigami.InlineMessage with Connections in qml. I asked to Marco Martin and I thank him for his help.

Thanks for your work. I will try to provide feedback as soon as I can. Sorry for the delay.

Nice work.

I noticed one issue when the empty playlist message is shown (after pressing the clear playlist button), the text moves if one closes the message. Could you please fix the layout such that the text is always centered at the same place even when the message is shown or is closing.

Could you have a look at adding automated tests for the new c++ code if feasible ?

Please have a look at my inline comments.

src/mediaplaylist.h
165–166

Better use isUndoDisplayedInline

266–267

As far as I know, Q_SCRIPTABLE is not necessary to call it from qml. Can you remove it ?

jguidon updated this revision to Diff 54846.Mar 26 2019, 11:18 AM

Thanks a lot for your feedback

I added two automated tests :

  • one for the clearPlaylist action
  • one for the replace an play action

For the layout, I changed the preferredHeight of the Item above the Image so the text stays centered.

mgallien accepted this revision.Wed, Mar 27, 6:31 AM

Thanks for your work.
Please fix the small issues.
Do you have a contributor account or do you need somebody to land your patch ?

src/mediaplaylist.cpp
604

Please remove the empty lines.

634

Please use only one empty lines between methods

src/qml/MediaPlayListView.qml
177

Please fix the typo.

This revision is now accepted and ready to land.Wed, Mar 27, 6:31 AM

@jguidon I hope everything is fine for you. It would really great to have your contribution land in master branch. This is a really nice improvement.
It seems you are not yet a KDE contributor. I should land your branch but there is very small issues to fix. Do you want me to finish the work or are you able to do it ?

jguidon updated this revision to Diff 56040.Fri, Apr 12, 8:27 AM
  • Fixed some typo and removed empty lines
jguidon marked 3 inline comments as done.Fri, Apr 12, 8:59 AM

Hi,

I am deeply sorry for the silence, I was quite busy these days and did not see the notifications going on.

I fixed the issues and indeed I do not have a KDE developer account yet.

I can apply for it, but if you want to land the branch quickly, do not hesitate of course.


However, while testing this task, I noticed a small issue when the first track is removed :

Before the action ;


After :

The track 1 is removed, but the track 2 (now 1) is not visible.

I tried to investigate and the bug comes from the commit fc9736e1d124f40002d835890af82a6103a0c322 . I tried to take a look but could not figure out it yet.

Would you like me to submit a bug report ?

Thank you

This revision was automatically updated to reflect the committed changes.

Hi,

I am deeply sorry for the silence, I was quite busy these days and did not see the notifications going on.

Do not worry. Your contribution is really nice (at least from my point of view) and very much appreciated.

I fixed the issues and indeed I do not have a KDE developer account yet.

I can apply for it, but if you want to land the branch quickly, do not hesitate of course.

In fact, you are supposed to contribute some changes before applying. I am supposed to land (or other KDE contributors) your changes in the meantime.


However, while testing this task, I noticed a small issue when the first track is removed :

Before the action ;


After :

The track 1 is removed, but the track 2 (now 1) is not visible.

I tried to investigate and the bug comes from the commit fc9736e1d124f40002d835890af82a6103a0c322 . I tried to take a look but could not figure out it yet.

I am not able to reproduce it. I have started using section headers because of this kind of bugs being easy to have before this commit.

Would you like me to submit a bug report ?

Yes please. The problem probably comes from a bad interaction between the model and the view. My hypothesis is that the model is sending changes in a way the view cannot handle (like multiple changes for the same line). Do you think you could try to find a solution ?

Thank you