ubuntu-touch-coreapps-reviewers team mailing list archive

[Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>]

Nekhelesh Ramananthan has proposed merging lp:~nik90/ubuntu-clock-app/improve-sound-detection into lp:ubuntu-clock-app.

Commit message:
Improve alarm sound detection in the EditAlarmPage using QFileInfo instead of looping over the QML FolderListModel which causes timing issues and leads to poor user experience.

Requested reviews:
  Ubuntu Clock Developers (ubuntu-clock-dev)

For more details, see:
https://code.launchpad.net/~nik90/ubuntu-clock-app/improve-sound-detection/+merge/268931

Background Info
---------------

In trunk, we wait for CustomSoundModel and DefaultSoundModel to finish loading (using a hackish Timer) and then perform a file check (by looping the model) to detect the existence of a custom sound or a default sound. If custom sound does not exist (due to being deleted by the user or any other reason), then we fallback to the default alarm sound.

This all works fine except for the part where we use a For loop to check for the file existence and use a QML Timer to ensure that the QML FolderListModel has fully loaded. This is ugly code and also is slightly visible in the UI.

Current MP
----------

This MP removes all those hacks and relies on QFileInfo class to detect the existence of a alarm sound file. So we no longer need to wait for any model to load. This is a much better solution and improves both reliability and speed of the alarm sound detection.

I also changed CustomSound Class to AlarmSound since it seems more appropriate now that the functions apply to all alarm sounds and not just the custom ones.
-- 
Your team Ubuntu Clock Developers is requested to review the proposed merge of lp:~nik90/ubuntu-clock-app/improve-sound-detection into lp:ubuntu-clock-app.

=== modified file 'app/alarm/AlarmSound.qml'
--- app/alarm/AlarmSound.qml	2015-08-22 21:19:54 +0000
+++ app/alarm/AlarmSound.qml	2015-08-24 16:45:32 +0000
@@ -94,8 +94,8 @@
             if (_alarmSoundPage.activeTransfer.state === ContentTransfer.Charged) {
                 _alarmSoundPage.importItems = _alarmSoundPage.activeTransfer.items
                 console.log("[LOG] Original Custom Alarm Sound URL: " + _alarmSoundPage.importItems[0].url)
-                customSound.prepareToAddAlarmSound(_alarmSoundPage.importItems[0].url)
-                if (_alarmSoundPage.importItems[0].move(customSound.alarmSoundDirectory) === true) {
+                alarmSoundHelper.prepareToAddAlarmSound(_alarmSoundPage.importItems[0].url)
+                if (_alarmSoundPage.importItems[0].move(alarmSoundHelper.customAlarmSoundDirectory) === true) {
                     // Wait for folder model to update and then select the imported sound
                     waitForCustomSoundModelToUpdate.start()
                 }
@@ -152,8 +152,8 @@
         }
     }
 
-    CustomAlarmSound {
-        id: customSound
+    AlarmSound {
+        id: alarmSoundHelper
     }
 
     Flickable {
@@ -245,7 +245,7 @@
                                         }
                                     }
 
-                                    customSound.deleteAlarmSound(fileName)
+                                    alarmSoundHelper.deleteCustomAlarmSound(fileName)
                                 }
                             }
                         ]

=== modified file 'app/alarm/EditAlarmPage.qml'
--- app/alarm/EditAlarmPage.qml	2015-08-21 22:15:32 +0000
+++ app/alarm/EditAlarmPage.qml	2015-08-24 16:45:32 +0000
@@ -56,6 +56,7 @@
         if(!isNewAlarm) {
             readAlarm()
         }
+        setAlarmSound()
     }
 
     // Function to save a new alarm
@@ -125,68 +126,34 @@
         }
     }
 
-    function getSoundName(chosenSoundPath) {
-        for(var i=0; i<defaultSoundModel.count; i++) {
-            if(chosenSoundPath === Qt.resolvedUrl(defaultSoundModel.get(i, "filePath"))) {
-                return defaultSoundModel.get(i, "fileBaseName")
-            }
-        }
-
-        for(var j=0; j<customSoundModel.count; j++) {
-            if(chosenSoundPath === Qt.resolvedUrl(customSoundModel.get(j, "filePath"))) {
-                return customSoundModel.get(j, "fileBaseName")
-            }
-        }
-
-        return ""
-    }
-
-    function getSoundPath(chosenSoundName) {
-        for(var i=0; i<defaultSoundModel.count; i++) {
-            if(chosenSoundName === defaultSoundModel.get(i, "fileBaseName")) {
-                return defaultSoundModel.get(i, "filePath")
-            }
-        }
-
-        for(var j=0; j<customSoundModel.count; j++) {
-            if(chosenSoundName === customSoundModel.get(i, "fileBaseName")) {
-                return customSoundModel.get(i, "filePath")
-            }
-        }
-    }
-
     /*
      #TODO: The default alarm sound was changed to "Alarm clock" which only lands
      in OTA-6. This function is need to maintain backwards compatibility with
      OTA-5 users.
     */
     function fallbacktoOldDefaultAlarmSound() {
-        _alarm.sound = getSoundPath("Suru arpeggio")
+        _alarm.sound = alarmSoundHelper.getDefaultAlarmSoundPath("Suru arpeggio.ogg")
         _alarmSound.subText = "Suru arpeggio"
     }
 
     function setDefaultAlarmSound() {
-        _alarm.sound = getSoundPath(_alarmSound.defaultAlarmSound)
+        _alarm.sound = alarmSoundHelper.getDefaultAlarmSoundPath(_alarmSound.defaultAlarmSoundFileName)
         _alarmSound.subText = _alarmSound.defaultAlarmSound
     }
 
     function setAlarmSound() {
         if(isNewAlarm) {
-            if (!getSoundPath(_alarmSound.defaultAlarmSound)) {
+            if (!alarmSoundHelper.isAlarmSoundValid(_alarmSound.defaultAlarmSoundFileName)) {
                 fallbacktoOldDefaultAlarmSound()
             } else {
                 setDefaultAlarmSound()
             }
         }
         else {
-            _alarmSound.subText = getSoundName(_alarm.sound.toString())
-            /*
-             If the custom alarm sound of an alarm was deleted by the user,
-             then fall back to the default alarm sound instead of showing an
-             empty string.
-            */
-            if (_alarmSound.subText === "") {
-                if (!getSoundPath(_alarmSound.defaultAlarmSound)) {
+            if (alarmSoundHelper.isAlarmSoundValid(_alarm.sound)) {
+                _alarmSound.subText = alarmSoundHelper.getSoundName(_alarm.sound.toString())
+            } else {
+                if (!alarmSoundHelper.isAlarmSoundValid(_alarmSound.defaultAlarmSoundFileName)) {
                     fallbacktoOldDefaultAlarmSound()
                 } else {
                     setDefaultAlarmSound()
@@ -251,47 +218,19 @@
 
     FolderListModel {
         id: defaultSoundModel
-
-        property bool isModelLoaded: false
-
         showDirs: false
         nameFilters: [ "*.ogg", "*.mp3" ]
         folder: "/usr/share/sounds/ubuntu/ringtones"
-        Component.onCompleted: {
-            isModelLoaded = true
-        }
     }
 
     FolderListModel {
         id: customSoundModel
-
-        property bool isModelLoaded: false
-
         showDirs: false
-        folder: customSound.alarmSoundDirectory
-        Component.onCompleted: {
-            isModelLoaded = true
-        }
-    }
-
-    /*
-     QML FolderListModel does not provide any signals to indicate that it is fully loaded other
-     than the onCompleted() signal which is not sufficient. This introduces a race-condition where
-     file checks are done *before* the folder models have fully loaded. This timer introduces a
-     delay to workaround that issue.
-    */
-    Timer {
-        id: delaySettingAlarmSoundTimer
-        interval: 100
-        running: defaultSoundModel.isModelLoaded && customSoundModel.isModelLoaded
-        onTriggered: {
-            setAlarmSound()
-        }
-    }
-
-    // Custom C++ Component that returns the clock app directory /home/phablet/.local/share/com.ubuntu.clock
-    CustomAlarmSound {
-        id: customSound
+        folder: alarmSoundHelper.customAlarmSoundDirectory
+    }
+
+    AlarmSound {
+        id: alarmSoundHelper
     }
 
     AlarmUtils {
@@ -367,6 +306,7 @@
             objectName: "alarmSound"
 
             readonly property string defaultAlarmSound: "Alarm clock"
+            readonly property string defaultAlarmSoundFileName: "Alarm clock.ogg"
 
             text: i18n.tr("Sound")
             onClicked: pageStack.push(Qt.resolvedUrl("AlarmSound.qml"), {

=== modified file 'app/ubuntu-clock-app.qml'
--- app/ubuntu-clock-app.qml	2015-08-23 00:53:57 +0000
+++ app/ubuntu-clock-app.qml	2015-08-24 16:45:32 +0000
@@ -83,10 +83,10 @@
         updateInterval: 1000
     }
 
-    CustomAlarmSound {
-        id: customSound
+    AlarmSound {
+        id: alarmSoundHelper
         // Create CustomSounds directory if it does not exist on app startup
-        Component.onCompleted: createAlarmSoundDirectory()
+        Component.onCompleted: createCustomAlarmSoundDirectory()
     }
 
     onApplicationStateChanged: {

=== modified file 'backend/CMakeLists.txt'
--- backend/CMakeLists.txt	2015-08-20 21:15:00 +0000
+++ backend/CMakeLists.txt	2015-08-24 16:45:32 +0000
@@ -37,7 +37,7 @@
 set(
     clockutility_SRCS
     modules/Clock/Utility/backend.cpp
-    modules/Clock/Utility/customalarmsound.cpp
+    modules/Clock/Utility/alarmsound.cpp
 )
 
 set(

=== renamed file 'backend/modules/Clock/Utility/customalarmsound.cpp' => 'backend/modules/Clock/Utility/alarmsound.cpp'
--- backend/modules/Clock/Utility/customalarmsound.cpp	2015-08-22 21:19:54 +0000
+++ backend/modules/Clock/Utility/alarmsound.cpp	2015-08-24 16:45:32 +0000
@@ -20,20 +20,21 @@
 #include <QFileInfo>
 #include <QStandardPaths>
 
-#include "customalarmsound.h"
+#include "alarmsound.h"
 
-CustomAlarmSound::CustomAlarmSound(QObject *parent):
+AlarmSound::AlarmSound(QObject *parent):
     QObject(parent),
-    m_customAlarmDir(QStandardPaths::standardLocations(QStandardPaths::AppDataLocation).first() + "/CustomSounds/")
+    m_customAlarmDir(QStandardPaths::standardLocations(QStandardPaths::AppDataLocation).first() + "/CustomSounds/"),
+    m_defaultAlarmDir("/usr/share/sounds/ubuntu/ringtones/")
 {
 }
 
-QString CustomAlarmSound::alarmSoundDirectory() const
+QString AlarmSound::customAlarmSoundDirectory() const
 {
     return m_customAlarmDir;
 }
 
-void CustomAlarmSound::deleteAlarmSound(const QString &soundName)
+void AlarmSound::deleteCustomAlarmSound(const QString &soundName)
 {
     QDir dir(m_customAlarmDir);
     if (dir.exists(soundName))
@@ -42,14 +43,14 @@
     }
 }
 
-void CustomAlarmSound::prepareToAddAlarmSound(const QString &soundPath)
+void AlarmSound::prepareToAddAlarmSound(const QString &soundPath)
 {
     QFileInfo soundFile(soundPath);
     QString soundFileName = soundFile.fileName();
-    deleteAlarmSound(soundFileName);
+    deleteCustomAlarmSound(soundFileName);
 }
 
-void CustomAlarmSound::createAlarmSoundDirectory()
+void AlarmSound::createCustomAlarmSoundDirectory()
 {
     QDir dir(m_customAlarmDir);
 
@@ -59,3 +60,33 @@
 
     dir.mkpath(m_customAlarmDir);
 }
+
+bool AlarmSound::isAlarmSoundValid(const QString &soundFileName)
+{
+    QFileInfo soundFile;
+
+    if (soundFile.exists(m_defaultAlarmDir + soundFileName)) {
+        return true;
+    } else if (soundFile.exists(m_customAlarmDir + soundFileName)) {
+        return true;
+    } else {
+        return false;
+    }
+}
+
+bool AlarmSound::isAlarmSoundValid(const QUrl &soundUrl)
+{
+    QFileInfo soundFile;
+    return soundFile.exists(soundUrl.path());
+}
+
+QString AlarmSound::getDefaultAlarmSoundPath(const QString &soundFileName) const
+{
+    return m_defaultAlarmDir + soundFileName;
+}
+
+QString AlarmSound::getSoundName(const QString &soundPath) const
+{
+    QFileInfo soundFile(soundPath);
+    return soundFile.baseName();
+}

=== renamed file 'backend/modules/Clock/Utility/customalarmsound.h' => 'backend/modules/Clock/Utility/alarmsound.h'
--- backend/modules/Clock/Utility/customalarmsound.h	2015-08-23 09:56:48 +0000
+++ backend/modules/Clock/Utility/alarmsound.h	2015-08-24 16:45:32 +0000
@@ -16,39 +16,45 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef CUSTOMALARMSOUND_H
-#define CUSTOMALARMSOUND_H
+#ifndef ALARMSOUND_H
+#define ALARMSOUND_H
 
 #include <QObject>
+#include <QUrl>
 
-class CustomAlarmSound: public QObject
+class AlarmSound: public QObject
 {
     Q_OBJECT
 
     // READONLY Property to return the custom alarm sound directory path
-    Q_PROPERTY( QString alarmSoundDirectory
-                READ alarmSoundDirectory
+    Q_PROPERTY( QString customAlarmSoundDirectory
+                READ customAlarmSoundDirectory
                 CONSTANT)
 
 public:
-    CustomAlarmSound(QObject *parent = 0);
+    AlarmSound(QObject *parent = 0);
 
-    // Function to return the custom alarm sound directory path
-    QString alarmSoundDirectory() const;
+    QString customAlarmSoundDirectory() const;
 
 public slots:
-    // Function to delete a custom alarm sound
-    void deleteAlarmSound(const QString &soundName);
+    void deleteCustomAlarmSound(const QString &soundName);
 
     // Function to delete old alarm file sound according to file name from full path.
     // It will able to replace sound alarm with new version
     void prepareToAddAlarmSound(const QString &soundPath);
 
-    // Function to create the CustomSounds alarm directory
-    void createAlarmSoundDirectory();
+    void createCustomAlarmSoundDirectory();
+
+    bool isAlarmSoundValid(const QString &soundFileName);
+    bool isAlarmSoundValid(const QUrl &soundUrl);
+
+    QString getDefaultAlarmSoundPath(const QString &soundFileName) const;
+
+    QString getSoundName(const QString &soundPath) const;
 
 private:
     QString m_customAlarmDir;
+    QString m_defaultAlarmDir;
 };
 
 #endif

=== modified file 'backend/modules/Clock/Utility/backend.cpp'
--- backend/modules/Clock/Utility/backend.cpp	2015-08-20 19:08:56 +0000
+++ backend/modules/Clock/Utility/backend.cpp	2015-08-24 16:45:32 +0000
@@ -19,13 +19,13 @@
 #include <QtQml>
 #include <QtQml/QQmlContext>
 #include "backend.h"
-#include "customalarmsound.h"
+#include "alarmsound.h"
 
 void BackendPlugin::registerTypes(const char *uri)
 {
     Q_ASSERT(uri == QLatin1String("Clock.Utility"));
 
-    qmlRegisterType<CustomAlarmSound>(uri, 1, 0, "CustomAlarmSound");
+    qmlRegisterType<AlarmSound>(uri, 1, 0, "AlarmSound");
 }
 
 void BackendPlugin::initializeEngine(QQmlEngine *engine, const char *uri)