Merge lp:~alextu/ubuntu-system-settings/incorrect-storage-size-2 into lp:ubuntu-system-settings

[Alex Tu]

for Arale device:
fixed:LP#1417459 Storage size not correct

for Arale device:
fixed:LP#1417459 Storage size not correct

[Sebastien Bacher (seb128)]

Thanks for the work

It would be useful if you explained a bit more what you are doing in the description (please add a commit message as well), e.g "deal with symlink mounts and with partition number being more than 1 digit"

I've added some review comments inline as well

review: Needs Fixing

[lp:~alextu/ubuntu-system-settings/incorrect-storage-size-2]

1351. By Alex Tu on 2015-03-25

1. correct the indent.
2. replace canonicalize_file_name() by QDir's canonicalPath()

1352. By Alex Tu on 2015-03-25

remove debug message

[Alex Tu (alextu)]

Fixed the incorrect point.
And also leave message for explain the logic.
Please help to review it again.

[Sebastien Bacher (seb128)]

thanks, the identic is still wrong though, you have 4 extra spaces indent after l432 without reason

that's because before the function was conditional to "if (fsName.startsWith(QString(QStringLiteral("mmc")))) {", e.g if the string wouldn't start with mmc it would fallback, which you remove

that points to another potential issue in your change, if index_mmc is -1, why do you continue the work rather than going to the fallback case as in the old code?

review: Needs Fixing

[lp:~alextu/ubuntu-system-settings/incorrect-storage-size-2]

1353. By Alex Tu on 2015-03-25

revised the logic to go fallback code when path doesn't have expected mmc string.

[Alex Tu (alextu)]

sorry, missed the fallback case, already revised it.

[Sebastien Bacher (seb128)]

thanks, that looks good to me now ;-)

review: Approve

[Sebastien Bacher (seb128)]

oh, you could move the "QString mmcString;" in the if() section, no need to allocate it if the index is -1

[lp:~alextu/ubuntu-system-settings/incorrect-storage-size-2]

1354. By Alex Tu on 2015-03-26

move the "QString mmcString;" in the if() section, no need to allocate it if the index is -1

[Alex Tu (alextu)]

Hi Sebastien Bacher,

Thanks! I also revised the location of "QString mmcString".
Because of I do have permission to merge this change back to lp:ubuntu-system-settings

Can someone help me to do that or what I should do for next step?

[Sebastien Bacher (seb128)]

Thanks Alex!

The touch components are landed through the CI machinery and not manually, please let it to us, it's on our list

review: Approve

[Alex Tu (alextu)]

Thanks a lot!

[Alex Tu (alextu)]

Hi Sebastien,
I find there already have several releases of ubuntu-system-settings after this change been approved. But they all looks not included my change.

Because of there have an issue on Arale waiting for this change.
https://bugs.launchpad.net/tangxi/+bug/1417459

So, can I have schedule about when is this change will be merged?

Thanks a lot.

=== modified file 'plugins/about/storageabout.cpp'
--- plugins/about/storageabout.cpp	2015-02-18 12:06:06 +0000
+++ plugins/about/storageabout.cpp	2015-03-26 06:25:59 +0000
@@ -414,7 +414,7 @@
         }
 
         // Now need to guess if it's InternalDrive or RemovableDrive
-        QString fsName(QString::fromLatin1(entry.mnt_fsname));
+        QString fsName = QDir(entry.mnt_fsname).canonicalPath();
         if (fsName.contains(QString(QStringLiteral("mapper")))) {
             struct stat status;
             stat(entry.mnt_fsname, &status);
@@ -424,11 +424,14 @@
             if (!fsName.isEmpty()) {
                 if (fsName.length() > 3) {
                     // only take the parent of the device
-                    if (fsName.at(fsName.size() -1).isDigit() && fsName.at(fsName.size() - 2) == QChar(QLatin1Char('p')))
-                        fsName.chop(2);
-                    if (fsName.startsWith(QString(QStringLiteral("mmc")))) {
+                    int index_mmc = fsName.indexOf("mmc",0,Qt::CaseInsensitive);
+                    if (index_mmc != -1) {
+                        QString mmcString;
+                        int index_p = fsName.indexOf('p',index_mmc,Qt::CaseInsensitive);
+                        mmcString = fsName.mid(index_mmc, index_p - index_mmc);
+
                         // "removable" attribute is set only for removable media, and we may have internal mmc cards
-                        fsName = QString(QStringLiteral("/sys/block/")) + fsName + QString(QStringLiteral("/device/uevent"));
+                        fsName = QString(QStringLiteral("/sys/block/")) + mmcString + QString(QStringLiteral("/device/uevent"));
                         QFile file(fsName);
                         if (file.open(QIODevice::ReadOnly)) {
                             QByteArray buf = file.readLine();