[Device Notifier] Add a button to unmount all devices
ClosedPublic

Authored by thsurrel on Oct 14 2018, 9:06 PM.

Details

Summary

When at least two removable devices are 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.

FEATURE: 395644

Test Plan

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

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thsurrel created this revision.Oct 14 2018, 9:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 14 2018, 9:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
thsurrel requested review of this revision.Oct 14 2018, 9:06 PM
thsurrel edited the summary of this revision. (Show Details)Oct 14 2018, 9:06 PM

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.

thsurrel added a comment.EditedOct 14 2018, 9:26 PM

What wording do you think would fit ? It seems quite some effort has been put in avoiding the word "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 ?

"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").

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

thsurrel updated this revision to Diff 43929.Oct 19 2018, 2:55 PM

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

ngraham accepted this revision.Oct 19 2018, 2:58 PM
ngraham added a subscriber: broulik.

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

Now, is it @broulik -approved? ;)

This revision is now accepted and ready to land.Oct 19 2018, 2:58 PM
thsurrel edited the summary of this revision. (Show Details)Oct 24 2018, 8:16 AM

Ping! Can we get a review from a Plasma person?

thsurrel updated this revision to Diff 46939.Dec 6 2018, 8:39 AM

Fix button width

ngraham added inline comments.Dec 6 2018, 7:45 PM
applets/devicenotifier/package/contents/ui/FullRepresentation.qml
119

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.

applets/devicenotifier/package/contents/ui/devicenotifier.qml
51

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

thsurrel updated this revision to Diff 46999.Dec 6 2018, 8:14 PM

Improvements as per Nate's comments

thsurrel marked 2 inline comments as done.Dec 6 2018, 8:15 PM
thsurrel added inline comments.
applets/devicenotifier/package/contents/ui/devicenotifier.qml
51

I put too much French into this one ;)

ngraham added inline comments.Dec 6 2018, 8:23 PM
applets/devicenotifier/package/contents/ui/devicenotifier.qml
51

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

thsurrel marked an inline comment as done.Dec 6 2018, 8:38 PM
thsurrel added inline comments.
applets/devicenotifier/package/contents/ui/devicenotifier.qml
51

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

ngraham accepted this revision.Dec 6 2018, 8:45 PM

Still looks great to me! Would be nice to get a Plasma person's comments too.

A last minute review in case this can get into 5.15 ? Please :)

broulik added inline comments.Jan 16 2019, 10:05 AM
applets/devicenotifier/package/contents/ui/DeviceItem.qml
38–39

Isn't this overridden anyway? It's also the default anyway

91

unmount?

93

Can we assume "action" is always unmount?

97

It looks like you instead want to use

Connections {
    target: unmountAll
    onClicked: ...
}
118

Careful about accessing model properties after the item has been removed from the model, I don't think this works?

applets/devicenotifier/package/contents/ui/FullRepresentation.qml
113

Why this change? ColumnLayout should layout everything automatically without the need for nachors

118

This is the default value anyway

210

Check index first to avoid potentially heavy access on data in case the condition isn't met

applets/devicenotifier/package/contents/ui/devicenotifier.qml
51

Can this be done more declaratively? I'm not a huge fan of having the delegate (which may not even be created in case it is scrolled out of view) programmatically do this.

I would at least opt for updating it in onDataChanged of the DataSource or onRowsAdded/onRowsRemoved or something along the lines of that

thsurrel updated this revision to Diff 49640.Jan 16 2019, 2:35 PM

Trying to improve based on broulik comments.
Thank you very much for the review!

Looking good, please check again the layout change you didn't explain. Otherwise I think shis hould be good. Sorry for the long delay.

applets/devicenotifier/package/contents/ui/FullRepresentation.qml
113

Can you please investigate this change?

applets/devicenotifier/package/contents/ui/devicenotifier.qml
123

This is for having the bindings refresh, right?

thsurrel marked 6 inline comments as done.Jan 16 2019, 2:38 PM
thsurrel added inline comments.
applets/devicenotifier/package/contents/ui/DeviceItem.qml
93

If the device is removable and mounted, than the action can only be unmounting, yes.

applets/devicenotifier/package/contents/ui/FullRepresentation.qml
113

I do not manage to get the layout I want with ColumnLayout: I need to align that ScrollArea to the same level than the 'unmountAll' button

applets/devicenotifier/package/contents/ui/devicenotifier.qml
123

Yes indeed. Any better ways ?

broulik added inline comments.Jan 16 2019, 2:42 PM
applets/devicenotifier/package/contents/ui/FullRepresentation.qml
113

How about adding the following to the button:

Layout.alignment: Qt.AlignRight
Layout.preferredWidth: minimumWidth
applets/devicenotifier/package/contents/ui/devicenotifier.qml
123

Should be fine, I thought one needed to put it in a variable first

var removables = devicenotifier.connectedRemovables;
removables.push(source);
devicenotifier.connectedRemovables = removables;
thsurrel added inline comments.Jan 16 2019, 2:52 PM
applets/devicenotifier/package/contents/ui/FullRepresentation.qml
113

That does not seem to do the trick.
I am trying to have the button at a fix location at the top right, but it is 'over' the scrollArea. Otherwise the scrollArea will be pushed down when the button appears, I was trying to avoid that.

broulik accepted this revision.Jan 16 2019, 2:53 PM

Let's go for it, I guess

applets/devicenotifier/package/contents/ui/FullRepresentation.qml
113

Ah, I see. Confused over and on top :) I see what you mean now.

There is a regression, I did not notice at first. By storing the connectedRemovables and not the mountedRemovables, the unmountAll button will appear even if nothing is mounted. That's not good.
I have to update a list in the onMountedChanged slot as I was doing before, or do you see a better way ?

Thank you for your time and guidance, QML still confuses me sometimes ...

thsurrel updated this revision to Diff 49644.Jan 16 2019, 3:16 PM

Display the unmountAll button only when there are some MOUNTED devices.

This revision was automatically updated to reflect the committed changes.