D16212: [Device Notifier] Add a button to unmount all devices

[Thomas Surrel]

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

REVISION SUMMARY
When at least one removable device is mounted, a button shows up
that will allow to unmount all mounted removable devices.
This is convenient for removable drives with several partitions,
each of which have to be unmounted to be able to safely plug the
device out.

BUG: 395644

TEST PLAN
Plug and mount two devices.
Click on the new 'unmount all' button.

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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, #vdg
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Nathaniel Graham]

ngraham added a comment.


Since the icon is identical to others used on the same pop-up, and there is no real universal "Unmount all" icon, I would support the use of a text label for this button to go along with the icon.

REPOSITORY
R120 Plasma Workspace

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

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

[Thomas Surrel]

thsurrel added a comment.


What wording do you think would fit ? It seems quite some effort has been put in avoiding the work "mount".
I am also a bit scared of the size the button would take in some languages and if it could collide with "Storage Volume". But maybe it should be somewhere else ?

REPOSITORY
R120 Plasma Workspace

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

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

[Nathaniel Graham]

ngraham added a comment.


"Safely remove all?" "Remove all"?

I know that giving these buttons text often isn't very popular, but if the layout is an issue with that, IMHO we should revisit it to provide more space. Labels are important for all but the most unambiguous, universal symbols (of which there is none for "unmount/eject/safely remove all").

REPOSITORY
R120 Plasma Workspace

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

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

[Thomas Surrel]

thsurrel added a comment.


F6330071: screenshot2.png <https://phabricator.kde.org/F6330071>

F6330070: screenshot1.png <https://phabricator.kde.org/F6330070>

Visually, I find it better with the label: two similar icons do not end up in the same area.

REPOSITORY
R120 Plasma Workspace

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

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

[Thomas Surrel]

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


Add a text label to the new button
Fixes to hide the button correctly when devices unmount

REPOSITORY
R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16212?vs=43624&id=43929

BRANCH
arc_unmountall (branched from master)

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

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, #vdg
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a subscriber: broulik.
ngraham added a comment.
This revision is now accepted and ready to land.


Consider it VDG-approved! The button label vastly improves the situation IMHO.

Now, is it @broulik -approved? ;)

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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

[Nathaniel Graham]

ngraham added a comment.


Ping! Can we get a review from a #plasma <https://phabricator.kde.org/tag/plasma/> person?

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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

[Thomas Surrel]

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


Fix button width

REPOSITORY
R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16212?vs=43929&id=46939

BRANCH
arc_unmountall (branched from master)

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

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, #vdg, ngraham
Cc: broulik, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

FullRepresentation.qml:119
+ enabled: true
+ visible: devicenotifier.mountedRemouvables.length > 0;
+ anchors.right: parent.right

What do you think about changing this to `> 1`? It seems kind of redundant to have an "unmount all" button when there's only one item.

devicenotifier.qml:51
property int currentIndex: -1
+ property var mountedRemouvables: []

mountedRemouvables -> mountedRemovables (and change all other instances, obviously)

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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

[Thomas Surrel]

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


Improvements as per Nate's comments

REPOSITORY
R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16212?vs=46939&id=46999

BRANCH
arc_unmountall (branched from master)

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

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, #vdg, ngraham
Cc: broulik, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Thomas Surrel]

thsurrel marked 2 inline comments as done.
thsurrel added inline comments.

INLINE COMMENTS

ngraham wrote in devicenotifier.qml:51
mountedRemouvables -> mountedRemovables (and change all other instances, obviously)

I put too much French into this one ;)

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

thsurrel wrote in devicenotifier.qml:51
I put too much French into this one ;)

En Anglais, les mots n'utilisent pas la lettre «u» aussi souvent qu'en Français. :)

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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

[Thomas Surrel]

thsurrel marked an inline comment as done.
thsurrel added inline comments.

INLINE COMMENTS

ngraham wrote in devicenotifier.qml:51
En Anglais, les mots n'utilisent pas la lettre «u» aussi souvent qu'en Français. :)

Je ne savais pas que tu parlais français ! 👍

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


Still looks great to me! Would be nice to get a #plasma <https://phabricator.kde.org/tag/plasma/> person's comments too.

REPOSITORY
R120 Plasma Workspace

BRANCH
arc_unmountall (branched from master)

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

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