Merge lp:~ken-vandine/ubuntu-system-settings/storage_page_speedup into lp:ubuntu-system-settings

[Ken VanDine]

* Use a loader for the Storage page and show an ActivityIndicator until the page is ready
* Improved performance of building the list of mountedVolumes

* Use a loader for the Storage page and show an ActivityIndicator until the page is ready
* Improved performance of building the list of mountedVolumes

[lp:~ken-vandine/ubuntu-system-settings/storage_page_speedup]

1631. By Ken VanDine on 2016-03-28

Trying to speed up device checking

1632. By Ken VanDine on 2016-03-28

Merged trunk

1633. By Ken VanDine on 2016-03-29

Added a label to show while the page is loading

1634. By Ken VanDine on 2016-03-29

Simplified string to just "Scanning"

[Jonas G. Drange (jonas-drange)]

Added a couple of comments. Also, you're basically solving bug 1488005 for the storage panel only. Should we consider solving it for all pages on the pageStack?

review: Needs Fixing

[lp:~ken-vandine/ubuntu-system-settings/storage_page_speedup]

1635. By Ken VanDine on 2016-03-30

remove debug logging

1636. By Ken VanDine on 2016-03-30

Only build the list of mountedVolumes once

1637. By Ken VanDine on 2016-03-30

removed some debug logging

[Matthew Paul Thomas (mpt)]

Agreed with Jonas. If you’re just “show[ing] an ActivityIndicator until the page is ready”, please fix bug 1488005 by making that ActivityIndicator part of a class that every System Settings page inherits from — or will inherit from, starting with this page.

[Ken VanDine (ken-vandine)]

I took a look at fixing bug 148805 in a generic way and it's a bit more involved than this plus it would need to be a fix with the way we use the PageStack. We should fix that when we switch to the AdaptivePageLayout. For now let's just land the handling in this slow page.

[Jonas G. Drange (jonas-drange)]

WFM!

review: Approve

=== modified file 'plugins/about/Storage.qml'
--- plugins/about/Storage.qml	2015-12-09 21:18:41 +0000
+++ plugins/about/Storage.qml	2016-03-30 17:37:59 +0000
@@ -28,100 +28,125 @@
 
 ItemPage {
     id: storagePage
-
     objectName: "storagePage"
     title: i18n.tr("Storage")
-    flickable: scrollWidget
-
-    property var allDrives: {
-        var drives = ["/"] // Always consider /
-        var paths = [backendInfo.getDevicePath("/")]
-        var systemDrives = backendInfo.mountedVolumes
-        for (var i = 0; i < systemDrives.length; i++) {
-            var drive = systemDrives[i]
-            var path = backendInfo.getDevicePath(drive)
-            /* only deal with the device's storage for now, external mounts
-               handling would require being smarter on the categories
-               computation as well and is not in the current design */
-            if (backendInfo.isInternal(drive) &&
-                paths.indexOf(path) == -1 && // Haven't seen this device before
-                path.charAt(0) === "/") { // Has a real mount point
-                drives.push(drive)
-                paths.push(path)
-            }
-        }
-        return drives
-    }
-    property real diskSpace: {
-        var space = 0
-        for (var i = 0; i < allDrives.length; i++) {
-            space += backendInfo.getTotalSpace(allDrives[i])
-        }
-        return space
-    }
-    /* Limit the free space to the user available one (see bug #1374134) */
-    property real freediskSpace: {
-        return backendInfo.getFreeSpace("/home")
-    }
-
-    property real usedByUbuntu: diskSpace -
-                                freediskSpace -
-                                backendInfo.homeSize -
-                                backendInfo.totalClickSize
-    property real otherSize: diskSpace -
-                             freediskSpace -
-                             usedByUbuntu -
-                             backendInfo.totalClickSize -
-                             backendInfo.moviesSize -
-                             backendInfo.picturesSize -
-                             backendInfo.audioSize
-    property variant spaceColors: [
-        UbuntuColors.orange,
-        "red",
-        "blue",
-        "green",
-        "yellow",
-        UbuntuColors.lightAubergine]
-    property variant spaceLabels: [
-        i18n.tr("Used by Ubuntu"),
-        i18n.tr("Videos"),
-        i18n.tr("Audio"),
-        i18n.tr("Pictures"),
-        i18n.tr("Other files"),
-        i18n.tr("Used by apps")]
-    property variant spaceValues: [
-        usedByUbuntu, // Used by Ubuntu
-        backendInfo.moviesSize,
-        backendInfo.audioSize,
-        backendInfo.picturesSize,
-        otherSize, //Other Files
-        backendInfo.totalClickSize]
-    property variant spaceObjectNames: [
-        "usedByUbuntuItem",
-        "moviesItem",
-        "audioItem",
-        "picturesItem",
-        "otherFilesItem",
-        "usedByAppsItem"]
-
-    GSettings {
-        id: settingsId
-        schema.id: "com.ubuntu.touch.system-settings"
-    }
-
-    UbuntuStorageAboutPanel {
-        id: backendInfo
-        property bool ready: false
-        // All of these events come simultaneously
-        onMoviesSizeChanged: ready = true
-        Component.onCompleted: populateSizes()
-        sortRole: settingsId.storageSortByName ?
-                      ClickRoles.DisplayNameRole :
-                      ClickRoles.InstalledSizeRole
-
-    }
-
-    Flickable {
+
+    Column {
+        anchors.centerIn: parent
+        visible: progress.running
+        spacing: units.gu(2)
+        Label {
+            anchors {
+                left: parent.left
+                right: parent.right
+            }
+            horizontalAlignment: Text.AlignHCenter
+            text: i18n.tr("Scanning")
+        }
+        ActivityIndicator {
+            id: progress
+            visible: running
+            running: !pageLoader.visible
+        }
+    }
+
+    Loader {
+        id: pageLoader
+        anchors.fill: parent
+        onStateChanged: {
+            if (state === Loader.Ready)
+                storagePage.flickable = scrollWidget;
+        }
+
+        asynchronous: true
+        visible: status == Loader.Ready
+        sourceComponent: Item {
+            anchors.fill: parent
+            property var allDrives: {
+                var drives = ["/"] // Always consider /
+                var paths = [backendInfo.getDevicePath("/")]
+                var systemDrives = backendInfo.mountedVolumes
+                for (var i = 0; i < systemDrives.length; i++) {
+                    var drive = systemDrives[i]
+                    var path = backendInfo.getDevicePath(drive)
+                    if (paths.indexOf(path) == -1 && // Haven't seen this device before
+                        path.charAt(0) === "/") { // Has a real mount point
+                        drives.push(drive)
+                        paths.push(path)
+                    }
+                }
+                return drives
+            }
+            property real diskSpace: {
+                var space = 0
+                for (var i = 0; i < allDrives.length; i++) {
+                    space += backendInfo.getTotalSpace(allDrives[i])
+                }
+                return space
+            }
+            /* Limit the free space to the user available one (see bug #1374134) */
+            property real freediskSpace: {
+                return backendInfo.getFreeSpace("/home")
+            }
+
+            property real usedByUbuntu: diskSpace -
+                                        freediskSpace -
+                                        backendInfo.homeSize -
+                                        backendInfo.totalClickSize
+            property real otherSize: diskSpace -
+                                     freediskSpace -
+                                     usedByUbuntu -
+                                     backendInfo.totalClickSize -
+                                     backendInfo.moviesSize -
+                                     backendInfo.picturesSize -
+                                     backendInfo.audioSize
+            property variant spaceColors: [
+                UbuntuColors.orange,
+                "red",
+                "blue",
+                "green",
+                "yellow",
+                UbuntuColors.lightAubergine]
+            property variant spaceLabels: [
+                i18n.tr("Used by Ubuntu"),
+                i18n.tr("Videos"),
+                i18n.tr("Audio"),
+                i18n.tr("Pictures"),
+                i18n.tr("Other files"),
+                i18n.tr("Used by apps")]
+            property variant spaceValues: [
+                usedByUbuntu, // Used by Ubuntu
+                backendInfo.moviesSize,
+                backendInfo.audioSize,
+                backendInfo.picturesSize,
+                otherSize, //Other Files
+                backendInfo.totalClickSize]
+            property variant spaceObjectNames: [
+                "usedByUbuntuItem",
+                "moviesItem",
+                "audioItem",
+                "picturesItem",
+                "otherFilesItem",
+                "usedByAppsItem"]
+
+            GSettings {
+                id: settingsId
+                schema.id: "com.ubuntu.touch.system-settings"
+            }
+
+            UbuntuStorageAboutPanel {
+                id: backendInfo
+                property bool ready: false
+                // All of these events come simultaneously
+                onMoviesSizeChanged: ready = true
+                Component.onCompleted: populateSizes()
+                sortRole: settingsId.storageSortByName ?
+                              ClickRoles.DisplayNameRole :
+                              ClickRoles.InstalledSizeRole
+
+            }
+
+            Flickable {
         id: scrollWidget
         anchors.fill: parent
         contentHeight: columnId.height
@@ -202,4 +227,6 @@
             }
         }
     }
+        }
+    }
 }
 
=== modified file 'plugins/about/storageabout.cpp'
--- plugins/about/storageabout.cpp	2015-07-13 18:26:10 +0000
+++ plugins/about/storageabout.cpp	2016-03-30 17:37:59 +0000
@@ -315,18 +315,41 @@
                                 m_cancellable));
 }
 
-QStringList StorageAbout::getMountedVolumes() const
-{
-    QStringList out;
-
-    Q_FOREACH (const QStorageInfo &storage, QStorageInfo::mountedVolumes())
-        if (storage.isValid() && storage.isReady())
-            out.append(storage.rootPath());
-
-    return out;
-}
-
-QString StorageAbout::getDevicePath(const QString mount_point)
+QStringList StorageAbout::getMountedVolumes()
+{
+    if (m_mountedVolumes.isEmpty())
+        prepareMountedVolumes();
+
+    return m_mountedVolumes;
+}
+
+void StorageAbout::prepareMountedVolumes()
+{
+    QStringList checked;
+
+    Q_FOREACH (const QStorageInfo &storage, QStorageInfo::mountedVolumes()) {
+        if (storage.isValid() && storage.isReady()) {
+            QString drive(storage.rootPath());
+            /* Only check devices once */
+            if (checked.contains(drive))
+                continue;
+             
+            checked.append(drive);
+            QString devicePath(getDevicePath(drive));
+            if (devicePath.isEmpty() || m_mountedVolumes.contains(drive))
+                continue;
+
+            /* only deal with the device's storage for now, external mounts
+               handling would require being smarter on the categories
+               computation as well and is not in the current design */
+            if (isInternal(drive)) {
+                m_mountedVolumes.append(drive);
+            }
+        }
+    }
+}
+
+QString StorageAbout::getDevicePath(const QString mount_point) const
 {
     QString s_mount_point;
     GUnixMountEntry * g_mount_point = nullptr;
@@ -362,7 +385,7 @@
  * met: http://www.gnu.org/copyleft/gpl.html.
  *
  */
-bool StorageAbout::isInternal(const QString &drive)
+bool StorageAbout::isInternal(const QString &drive) const
 {
     bool ret = false;
     FILE *fsDescription = setmntent(_PATH_MOUNTED, "r");
 
=== modified file 'plugins/about/storageabout.h'
--- plugins/about/storageabout.h	2015-07-13 18:26:10 +0000
+++ plugins/about/storageabout.h	2016-03-30 17:37:59 +0000
@@ -108,11 +108,11 @@
     quint64 getPicturesSize();
     quint64 getHomeSize();
     Q_INVOKABLE void populateSizes();
-    QStringList getMountedVolumes() const;
-    Q_INVOKABLE QString getDevicePath (const QString mount_point);
+    QStringList getMountedVolumes();
+    Q_INVOKABLE QString getDevicePath (const QString mount_point) const;
     Q_INVOKABLE qint64 getFreeSpace (const QString mount_point);
     Q_INVOKABLE qint64 getTotalSpace (const QString mount_point);
-    Q_INVOKABLE bool isInternal(const QString &drive);
+    Q_INVOKABLE bool isInternal(const QString &drive) const;
     bool getDeveloperMode();
     void setDeveloperMode(bool newMode);
 
@@ -121,6 +121,8 @@
     void sizeReady();
 
 private:
+    void prepareMountedVolumes();
+    QStringList m_mountedVolumes;
     QString m_serialNumber;
     QString m_vendorString;
     QString m_deviceBuildDisplayID;