D19796: [Device Notifier] Restore Solid notification messages

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  This is just much to complicated, adding layer over layer does not help.
  
  Also, the second problem is unrelated to the change you mention - the 
"hasMessage" property could only be true for one DeviceItem, before and after.

INLINE COMMENTS

> DeviceItem.qml:50
>  
> -readonly property bool hasMessage: statusSource.lastUdi == udi && 
> statusSource.data[statusSource.last] ? true : false
> -readonly property var message: hasMessage ? 
> statusSource.data[statusSource.last] || ({}) : ({})
> +readonly property bool hasMessage: statusSource.trigger && 
> statusSource.lastMessages[udi]
> +readonly property var message: hasMessage ? 
> statusSource.lastMessages[udi] : ({})

trigger is essentially always true

> DeviceItem.qml:125
>  // otherwise the last message will show again when this device 
> reappears
> -statusSource.clearMessage()
> +statusSource.clearMessage(udi)
>  

the item should not mess with the internals of the statusSource

> FullRepresentation.qml:108
>  if (!plasmoid.expanded) {
> -statusSource.clearMessage()
> +statusSource.clearMessages()
>  }

This is IMHO wrong, if the plasmoid shows an error message and I happen to 
close it by clicking elsewhere, I can no longer see the error message, although 
it still applies

> FullRepresentation.qml:181
>  state: sdSource.data[udi] ? sdSource.data[udi].State : 0
> -isRoot: sdSource.data[udi]["File Path"] === "/"
> +isRoot: sdSource.data[udi] ? sdSource.data[udi]["File Path"] === 
> "/" : false
>  

This is an unrelated change, and fixes an error introduced in 
https://phabricator.kde.org/R120:d1a5507bd57aa74c18392354dcd43b65e15ee491

> devicenotifier.qml:218
> +// Source is formatted as follows: " notification"
> +var udi = sources[i].split(' ')[0]
> +if (data[sources[i]].error.length > 0) {

would be much simpler to use `udi = data[sources[i]].udi`

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19796

To: thsurrel, #plasma, broulik, bruns
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19796: [Device Notifier] Restore Solid notification messages

[Thomas Surrel]

thsurrel updated this revision to Diff 57736.
thsurrel added a comment.


  Fix comments.
  Thank you for the review!

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19796?vs=57722=57736

BRANCH
  arc_removed_devices

REVISION DETAIL
  https://phabricator.kde.org/D19796

AFFECTED FILES
  applets/devicenotifier/package/contents/ui/DeviceItem.qml
  applets/devicenotifier/package/contents/ui/FullRepresentation.qml
  applets/devicenotifier/package/contents/ui/devicenotifier.qml

To: thsurrel, #plasma, broulik, bruns
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19796: [Device Notifier] Restore Solid notification messages

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> DeviceItem.qml:50
>  
> -readonly property bool hasMessage: statusSource.lastUdi == udi && 
> statusSource.data[statusSource.last] ? true : false
> -readonly property var message: hasMessage ? 
> statusSource.data[statusSource.last] || ({}) : ({})
> +readonly property bool hasMessage: statusSource.trigger && 
> statusSource.lastMessages[udi] ? true : false
> +readonly property var message: hasMessage ? 
> statusSource.lastMessages[udi] : ({})

do we need `? true : false` i think no?

> devicenotifier.qml:226
> +} else {
> +delete(lastMessages[udi])
>  }

clearMessage(udi)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19796

To: thsurrel, #plasma, broulik, bruns
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19796: [Device Notifier] Restore Solid notification messages

[Thomas Surrel]

thsurrel updated this revision to Diff 57722.
thsurrel added a comment.


  Rebase

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19796?vs=53978=57722

BRANCH
  arc_removed_devices

REVISION DETAIL
  https://phabricator.kde.org/D19796

AFFECTED FILES
  applets/devicenotifier/package/contents/ui/DeviceItem.qml
  applets/devicenotifier/package/contents/ui/FullRepresentation.qml
  applets/devicenotifier/package/contents/ui/devicenotifier.qml

To: thsurrel, #plasma, broulik, bruns
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19796: [Device Notifier] Restore Solid notification messages

[Thomas Surrel]

thsurrel added a comment.


  Anyone could have a look at this ? Thanks in advance.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19796

To: thsurrel, #plasma, broulik
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19796: [Device Notifier] Restore Solid notification messages

[Thomas Surrel]

thsurrel created this revision.
thsurrel added reviewers: Plasma, broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
thsurrel requested review of this revision.

REVISION SUMMARY
  Commit 17380886 
 in 
the devicenotifications data engine brought in
  two problems with the Device Notifier:
  
  - the notifications are now cleared once a device is unmounted,
  
  this results in the "You can now safely remove this device"
  message not to be shown anymore.
  
  - since each notification is no longer its own data source, it
  
  is possible to get into a situation where one device would only
  receive a first notification but not any subsequent ones.
  
  This patch attempts to fix these two problems.

TEST PLAN
  Mount a removable device and unmount it through the Device
  Notifier. A message should appear for a few seconds saying you
  can safely remove that device.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arc_removed_devices

REVISION DETAIL
  https://phabricator.kde.org/D19796

AFFECTED FILES
  applets/devicenotifier/package/contents/ui/DeviceItem.qml
  applets/devicenotifier/package/contents/ui/FullRepresentation.qml
  applets/devicenotifier/package/contents/ui/devicenotifier.qml

To: thsurrel, #plasma, broulik
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart