Remove PassiveNotification in favor of Kirigami.InlineMessage
Needs ReviewPublic

Authored by astippich on Wed, Jun 19, 5:53 PM.

Details

Reviewers
mgallien
ngraham
Summary

Remove entirely the PassiveNotification and use the already
present Kirigami.InlineMessage to consistently display
stuff related to playlists

Test Plan

load e.g. any file that is not a playlist
using the load playlist button, new notification is displayed

Diff Detail

Repository
R255 Elisa
Branch
playlist_notification
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13930
Build 13948: arc lint + arc unit
astippich requested review of this revision.Wed, Jun 19, 5:53 PM
astippich created this revision.

+1, though these messages are currently not very useful because they're not actionable. If loading or saving of the playlist failed, what can I do about it? it's not clear. Especially for persistent messages, it's worth it to try to make them actionable somehow. Options include a better error message, a "Try again" button to preserve the illusion of user agency, etc.

src/qml/MediaPlayListView.qml
64

This should be colored as a warning or error.

+1, though these messages are currently not very useful because they're not actionable. If loading or saving of the playlist failed, what can I do about it? it's not clear. Especially for persistent messages, it's worth it to try to make them actionable somehow. Options include a better error message, a "Try again" button to preserve the illusion of user agency, etc.

I do not think it is such a good idea.
We should better try to use the passive notification by using more components from Kirigami instead of removing them.
Elisa will need to show notification to the user (and much better than now).

+1, though these messages are currently not very useful because they're not actionable. If loading or saving of the playlist failed, what can I do about it? it's not clear. Especially for persistent messages, it's worth it to try to make them actionable somehow. Options include a better error message, a "Try again" button to preserve the illusion of user agency, etc.

I do not think it is such a good idea.
We should better try to use the passive notification by using more components from Kirigami instead of removing them.

InlineMessage is also from Kirigami. :)

The PassiveNotification component is unfortunately very easy to abuse because its intended use case is very narrow: it's only for non-critical informational messages. The only time it's acceptable to use them for errors is when the error automatically retries itself, e.g. "network not available, retrying in 5 seconds". For any error message where the user may have to do something to recover, it's not appropriate to use a PassiveNotification. We're trying to remove these from Discover, in fact: https://bugs.kde.org/show_bug.cgi?id=403791

+1, though these messages are currently not very useful because they're not actionable. If loading or saving of the playlist failed, what can I do about it? it's not clear. Especially for persistent messages, it's worth it to try to make them actionable somehow. Options include a better error message, a "Try again" button to preserve the illusion of user agency, etc.

There are certainly more improvements possible, but step by step :)

I do not think it is such a good idea.
We should better try to use the passive notification by using more components from Kirigami instead of removing them.
Elisa will need to show notification to the user (and much better than now).

Note that Elisa currently has 4(!) different notifications. There is

  • track import notification
  • top notification
  • passive notification
  • Kirigami.InlineMessage

The last two are only used for the media play list. The InlineMessage was already there for the "Undo" action, while the PassiveNotification was used for loading and saving. The PassiveNotification is IMHO not very visible, since it is very greyish and located over the content view.
As all buttons triggering the notification are the ones in the media play list section, I find the new location proposed in this patch quite suitable and IMHO provides a consistent and concise way of presenting the notifications

So, do we find an agreement here?

So, do we find an agreement here?

Yes, as said, I miss enough time until at least Sunday and will anyway have reduced bandwidth for two weeks.

I am perfectly okay with the main idea. I agree with the difference between PassiveNotification and InlineMessage .

Please see my comment and other comments

src/qml/MediaPlayListView.qml
147

In the message and in the comment you are once talking about save and once about load.

astippich updated this revision to Diff 61534.Wed, Jul 10, 6:29 PM
  • fix text and apply error type
astippich updated this revision to Diff 61738.Sun, Jul 14, 1:08 PM
  • fixup type

I tried to implement the actions for load and save playlist. However, InlineMessage seems only to be compatible with Kirigami Actions and not with the qqc2 ones, which is unfortunate

That's probably worth a Kirigami bug report.