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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> DeviceItem.qml:38
>  
> -    property bool mounted
> +    property bool mounted: false
>      property bool expanded: devicenotifier.expandedDevice == udi

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

> DeviceItem.qml:90
>  
> +    function unMount() {
> +        if (mounted) {

`unmount`?

> DeviceItem.qml:92
> +        if (mounted) {
> +            actionTriggered();
> +        }

Can we assume "action" is always unmount?

> DeviceItem.qml:96
> +
> +    ListView.onAdd: {
> +        if (model["Removable"]) {

It looks like you instead want to use

  Connections {
      target: unmountAll
      onClicked: ...
  }

> DeviceItem.qml:120
> +
> +        if (model["Removable"]) {
> +            unmountAll.clicked.disconnect(unMount)

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

> FullRepresentation.qml:113
>  
> -    ColumnLayout {
> +    Item {
>          anchors.fill: parent

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

> FullRepresentation.qml:118
> +            id: unmountAll
> +            enabled: true
> +            visible: devicenotifier.mountedRemovables.length > 1;

This is the default value anyway

> FullRepresentation.qml:204
> +                var index = devicenotifier.mountedRemovables.indexOf(udi);
> +                if (mounted && sdSource.data[udi] && sdSource.data[udi]["Removable"] && index < 0) {
> +                    devicenotifier.mountedRemovables.push(udi);

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

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

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

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, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190116/4467dd8c/attachment-0001.html>