ubuntu-touch-coreapps-reviewers team mailing list archive

[Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>]

Nekhelesh Ramananthan has proposed merging lp:~nik90/ubuntu-clock-app/replace-alarmsound-checkbox into lp:ubuntu-clock-app.

Commit message:
- Replaces alarm sound page checkboxes with tick icons instead to match system-settings app
- Added section headers to separate custom and default alarm sounds
- Bumped to v3.6

Requested reviews:
  Ubuntu Clock Developers (ubuntu-clock-dev)
Related bugs:
  Bug #1487717 in Ubuntu Clock App: "[Clock] Alarm sounds page should show tick icon instead of checkbox to indicate chosen alarm sound"
  https://bugs.launchpad.net/ubuntu-clock-app/+bug/1487717
  Bug #1487735 in Ubuntu Clock App: "[Clock] Differentiate between custom sounds and default sounds in the alarm sound page"
  https://bugs.launchpad.net/ubuntu-clock-app/+bug/1487735

For more details, see:
https://code.launchpad.net/~nik90/ubuntu-clock-app/replace-alarmsound-checkbox/+merge/269328

- Replaces alarm sound page checkboxes with tick icons instead to match system-settings app
- Added section headers to separate custom and default alarm sounds
- Bumped to v3.6

The use of tick-marks allowed me to move the QML FolderListModel from the EditAlarmPage.qml to AlarmSound.page. This means we shave of the loading times of the EditAlarmPage.qml a bit.
-- 
Your team Ubuntu Clock Developers is requested to review the proposed merge of lp:~nik90/ubuntu-clock-app/replace-alarmsound-checkbox into lp:ubuntu-clock-app.

=== modified file 'app/alarm/AlarmSound.qml'
--- app/alarm/AlarmSound.qml	2015-08-24 15:08:21 +0000
+++ app/alarm/AlarmSound.qml	2015-08-27 19:33:34 +0000
@@ -21,6 +21,7 @@
 import QtMultimedia 5.0
 import Ubuntu.Content 1.1
 import Ubuntu.Components 1.2
+import Qt.labs.folderlistmodel 2.1
 
 Page {
     id: _alarmSoundPage
@@ -39,12 +40,6 @@
     // Property to store the alarm object
     property var alarm
 
-    // Property to set the alarm sound model in the edit alarm page
-    property var defaultSoundModel
-
-    // Property to set the custom alarm sound model in the edit alarm page
-    property var customSoundModel
-
     /*
      Properties to store the previously set alarm sound values to detect
      if the user changed the alarm sound or not
@@ -83,6 +78,19 @@
         }
     }
 
+    FolderListModel {
+        id: defaultSoundModel
+        showDirs: false
+        nameFilters: [ "*.ogg", "*.mp3" ]
+        folder: "/usr/share/sounds/ubuntu/ringtones"
+    }
+
+    FolderListModel {
+        id: customSoundModel
+        showDirs: false
+        folder: alarmSoundHelper.customAlarmSoundDirectory
+    }
+
     ContentTransferHint {
         anchors.fill: parent
         activeTransfer: _alarmSoundPage.activeTransfer
@@ -117,9 +125,9 @@
         for (var i=0; i<customSoundModel.count; i++) {
             if (soundUrl === customSoundModel.get(i, "fileURL")) {
                 alarmSound.subText = customSoundModel.get(i, "fileBaseName")
-                alarm.sound = soundUrl
-                previewAlarmSound.source = soundUrl
+                alarm.sound = soundUrl                
                 _customAlarmSounds.itemAt(i).isChecked = true
+                previewAlarmSound.controlPlayback(soundUrl)
                 return
             }
         }
@@ -162,7 +170,8 @@
         anchors.fill: parent
         contentHeight: defaultSoundModel.count * units.gu(7) +
                        customSoundModel.count * units.gu(7) +
-                       customSoundListItem.height
+                       customSoundListItem.height +
+                       2*customSoundsHeader.implicitHeight + units.gu(4)
 
         Column {
             id: _alarmSoundColumn
@@ -183,6 +192,21 @@
                 }
             }
 
+            ListItem {
+                visible: customSoundModel.count !== 0
+                height: customSoundsHeader.implicitHeight + units.gu(2)
+                Label {
+                    id: customSoundsHeader
+                    text: i18n.tr("Custom Sounds")
+                    font.weight: Font.DemiBold
+                    anchors {
+                        left: parent.left
+                        leftMargin: units.gu(2)
+                        verticalCenter: parent.verticalCenter
+                    }
+                }
+            }
+
             Repeater {
                 id: _customAlarmSounds
                 objectName: "customAlarmSounds"
@@ -191,8 +215,10 @@
 
                 ListItem {
                     id: _customAlarmSoundDelegate
+                    objectName: "customAlarmSoundDelegate" + index
 
-                    property alias isChecked: _customSoundStatus.checked
+                    property bool isChecked: alarmSound.subText === _customSoundName.text ? true
+                                                                                          : false
 
                     height: units.gu(7)
 
@@ -219,9 +245,9 @@
                                             for (var i=0; i<defaultSoundModel.count; i++) {
                                                 if (defaultSoundModel.get(i, "fileBaseName") === alarmSound.subText) {
                                                     alarm.sound = defaultSoundModel.get(i, "fileURL")
-                                                    oldAlarmSoundUrl = alarm.sound
-                                                    previewAlarmSound.source = defaultSoundModel.get(i, "fileURL")
+                                                    oldAlarmSoundUrl = alarm.sound                                                    
                                                     _alarmSounds.itemAt(i).isChecked = true
+                                                    previewAlarmSound.controlPlayback(defaultSoundModel.get(i, "fileURL"))
                                                 }
                                             }
                                         }
@@ -229,7 +255,7 @@
                                         else {
                                             alarmSound.subText = oldAlarmSoundName
                                             alarm.sound = oldAlarmSoundUrl
-                                            previewAlarmSound.source = alarm.sound
+                                            previewAlarmSound.controlPlayback(alarm.sound)
 
                                             for (var j=0; j<defaultSoundModel.count; j++) {
                                                 if (defaultSoundModel.get(j, "fileBaseName") === alarmSound.subText) {
@@ -258,7 +284,7 @@
                         anchors {
                             left: parent.left
                             leftMargin: units.gu(2)
-                            right: _customSoundStatus.left
+                            right: tickIcon.left
                             rightMargin: units.gu(2)
                             verticalCenter: parent.verticalCenter
                         }
@@ -269,52 +295,59 @@
                         text: fileBaseName
                     }
 
-                    CheckBox {
-                        id: _customSoundStatus
-                        objectName: "customSoundStatus" + index
+                    Icon {
+                        id: tickIcon
+                        width: units.gu(2)
+                        height: width
+                        name: "tick"
+                        visible: _customAlarmSoundDelegate.isChecked
 
                         anchors {
                             right: parent.right
                             rightMargin: units.gu(2)
                             verticalCenter: parent.verticalCenter
                         }
-
-                        checked: alarmSound.subText === _customSoundName.text ? true
-                                                                              : false
-                        onCheckedChanged: {
-                            if (checked) {
-                                previewAlarmSound.controlPlayback(fileURL)
-                                alarmSound.subText = _customSoundName.text
-                                alarm.sound = fileURL
-
-                                // Ensures only one alarm sound is selected
-                                for(var i=0; i<customSoundModel.count; i++) {
-                                    if(_customAlarmSounds.itemAt(i).isChecked &&
-                                            i !== index) {
-                                        _customAlarmSounds.itemAt(i).isChecked = false
-                                    }
-                                }
-
-                                // Ensures only one alarm customSoundModelsound is selected
-                                for(i=0; i<defaultSoundModel.count; i++) {
-                                    _alarmSounds.itemAt(i).isChecked = false
+                    }
+
+                    onIsCheckedChanged: {
+                        if (isChecked) {
+                            alarmSound.subText = _customSoundName.text
+                            alarm.sound = fileURL
+
+                            // Ensures only one custom alarm sound is selected
+                            for(var i=0; i<customSoundModel.count; i++) {
+                                if(_customAlarmSounds.itemAt(i).isChecked && i !== index) {
+                                    _customAlarmSounds.itemAt(i).isChecked = false
                                 }
                             }
-                        }
 
-                        onClicked: {
-                            if (!checked) {
-                                checked = true
+                            // Uncheck all the default alarm sounds
+                            for(i=0; i<defaultSoundModel.count; i++) {
+                                _alarmSounds.itemAt(i).isChecked = false
                             }
                         }
                     }
 
                     onClicked: {
-                        if (!_customSoundStatus.checked) {
-                            _customSoundStatus.checked = true
-                        } else {
-                            previewAlarmSound.controlPlayback(fileURL)
+                        if (!_customAlarmSoundDelegate.isChecked) {
+                            _customAlarmSoundDelegate.isChecked = true
                         }
+                        previewAlarmSound.controlPlayback(fileURL)
+                    }
+                }
+            }
+
+            ListItem {
+                visible: customSoundModel.count !== 0
+                height: defaultSoundsHeader.implicitHeight + units.gu(2)
+                Label {
+                    id: defaultSoundsHeader
+                    text: i18n.tr("Default Sounds")
+                    font.weight: Font.DemiBold
+                    anchors {
+                        left: parent.left
+                        leftMargin: units.gu(2)
+                        verticalCenter: parent.verticalCenter
                     }
                 }
             }
@@ -327,8 +360,10 @@
 
                 ListItem {
                     id: _alarmSoundDelegate
+                    objectName: "alarmSoundDelegate" + index
 
-                    property alias isChecked: _soundStatus.checked
+                    property bool isChecked: alarmSound.subText === _soundName.text ? true
+                                                                                    : false
 
                     height: units.gu(7)
 
@@ -347,52 +382,44 @@
                         text: fileBaseName
                     }
 
-                    CheckBox {
-                        id: _soundStatus
-                        objectName: "soundStatus" + index
+                    Icon {
+                        width: units.gu(2)
+                        height: width
+                        name: "tick"
+                        visible: _alarmSoundDelegate.isChecked
 
                         anchors {
                             right: parent.right
                             rightMargin: units.gu(2)
                             verticalCenter: parent.verticalCenter
                         }
-
-                        checked: alarmSound.subText === _soundName.text ? true
-                                                                        : false
-                        onCheckedChanged: {
-                            if (checked) {
-                                previewAlarmSound.controlPlayback(fileURL)
-                                alarmSound.subText = _soundName.text
-                                alarm.sound = fileURL
-
-                                // Ensures only one alarm sound is selected
-                                for(var i=0; i<defaultSoundModel.count; i++) {
-                                    if(_alarmSounds.itemAt(i).isChecked &&
-                                            i !== index) {
-                                        _alarmSounds.itemAt(i).isChecked = false
-                                    }
-                                }
-
-                                // Ensures only one alarm sound is selected
-                                for(i=0; i<customSoundModel.count; i++) {
-                                    _customAlarmSounds.itemAt(i).isChecked = false
+                    }
+
+                    onIsCheckedChanged: {
+                        if (isChecked) {
+                            alarmSound.subText = _soundName.text
+                            alarm.sound = fileURL
+
+                            // Ensures only one alarm sound is selected
+                            for(var i=0; i<defaultSoundModel.count; i++) {
+                                if(_alarmSounds.itemAt(i).isChecked &&
+                                        i !== index) {
+                                    _alarmSounds.itemAt(i).isChecked = false
                                 }
                             }
-                        }
 
-                        onClicked: {
-                            if (!checked) {
-                                checked = true
+                            // Uncheck all the custom alarm sounds
+                            for(i=0; i<customSoundModel.count; i++) {
+                                _customAlarmSounds.itemAt(i).isChecked = false
                             }
                         }
                     }
 
                     onClicked: {
-                        if (!_soundStatus.checked) {
-                            _soundStatus.checked = true
-                        } else {
-                            previewAlarmSound.controlPlayback(fileURL)
+                        if (!_alarmSoundDelegate.isChecked) {
+                            _alarmSoundDelegate.isChecked = true
                         }
+                        previewAlarmSound.controlPlayback(fileURL)
                     }
                 }
             }

=== modified file 'app/alarm/EditAlarmPage.qml'
--- app/alarm/EditAlarmPage.qml	2015-08-25 11:39:19 +0000
+++ app/alarm/EditAlarmPage.qml	2015-08-27 19:33:34 +0000
@@ -20,7 +20,6 @@
 import DateTime 1.0
 import Clock.Utility 1.0
 import Ubuntu.Components 1.2
-import Qt.labs.folderlistmodel 2.1
 import Ubuntu.Components.Pickers 1.0
 import "../components"
 
@@ -215,19 +214,6 @@
         }
     }
 
-    FolderListModel {
-        id: defaultSoundModel
-        showDirs: false
-        nameFilters: [ "*.ogg", "*.mp3" ]
-        folder: "/usr/share/sounds/ubuntu/ringtones"
-    }
-
-    FolderListModel {
-        id: customSoundModel
-        showDirs: false
-        folder: alarmSoundHelper.customAlarmSoundDirectory
-    }
-
     AlarmSound {
         id: alarmSoundHelper
     }
@@ -310,9 +296,7 @@
             text: i18n.tr("Sound")
             onClicked: pageStack.push(Qt.resolvedUrl("AlarmSound.qml"), {
                                           "alarmSound": _alarmSound,
-                                          "alarm": _alarm,
-                                          "defaultSoundModel": defaultSoundModel,
-                                          "customSoundModel": customSoundModel
+                                          "alarm": _alarm
                                       })
         }
     }

=== modified file 'app/worldclock/WorldCityList.qml'
--- app/worldclock/WorldCityList.qml	2015-08-14 05:34:49 +0000
+++ app/worldclock/WorldCityList.qml	2015-08-27 19:33:34 +0000
@@ -124,7 +124,7 @@
                         var url = String("%1%2%3")
                         .arg("http://geoname-lookup.ubuntu.com/?query=";)
                         .arg(searchField.text)
-                        .arg("&app=com.ubuntu.clock&version=3.5.x")
+                        .arg("&app=com.ubuntu.clock&version=3.6.x")
                         console.log("Online URL: " + url)
                         if (jsonTimeZoneModelLoader.status === Loader.Ready) {
                             jsonTimeZoneModel.source = Qt.resolvedUrl(url)

=== modified file 'debian/changelog'
--- debian/changelog	2015-08-25 11:36:52 +0000
+++ debian/changelog	2015-08-27 19:33:34 +0000
@@ -1,4 +1,12 @@
-ubuntu-clock-app (3.5) UNRELEASED; urgency=medium
+ubuntu-clock-app (3.6) UNRELEASED; urgency=medium
+
+  [Nekhelesh Ramananthan]
+  * Replaced checkboxes in alarm sound page with tick-marks (LP: #1487717)
+  * Added sections headers to separate custom and default alarm sounds (LP: #1487735)
+
+ -- Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>  Thu, 27 Aug 2015 20:56:08 +0200
+
+ubuntu-clock-app (3.5) vivid; urgency=medium
 
   [Bartosz Kosiorek]
   * Disable automatic translation and update README.translations

=== modified file 'manifest.json.in'
--- manifest.json.in	2015-08-03 20:09:51 +0000
+++ manifest.json.in	2015-08-27 19:33:34 +0000
@@ -12,7 +12,7 @@
             "urls": "share/url-dispatcher/urls/com.ubuntu.clock_clock.url-dispatcher"
         }
     },
-    "version": "3.5.@BZR_REVNO@",
+    "version": "3.6.@BZR_REVNO@",
     "maintainer": "Ubuntu App Cats <ubuntu-touch-coreapps@xxxxxxxxxxxxxxxxxxx>",
     "x-test": {
         "autopilot": {

=== modified file 'tests/unit/tst_alarmSound.qml'
--- tests/unit/tst_alarmSound.qml	2015-08-21 14:14:21 +0000
+++ tests/unit/tst_alarmSound.qml	2015-08-27 19:33:34 +0000
@@ -34,18 +34,9 @@
         sound: "file:///usr/share/sounds/ubuntu/ringtones/Bliss.ogg"
     }
 
-    FolderListModel {
-        id: _defaultSoundModel
-
-        showDirs: false
-        nameFilters: [ "*.ogg", "*.mp3" ]
-        folder: "/usr/share/sounds/ubuntu/ringtones"
-    }
-
     AlarmSound {
         id: alarmSoundPage
         alarm: _alarm
-        defaultSoundModel: _defaultSoundModel
         alarmSound: { "subText": "Bliss" }
     }
 
@@ -68,15 +59,15 @@
             alarmSoundPage.alarmSound.subText = "Bliss"
             _alarm.sound = "file:///usr/share/sounds/ubuntu/ringtones/Bliss.ogg"
             for(var i=0; i<repeater.count; i++) {
-                var alarmSoundSwitch = findChild(alarmSoundPage, "soundStatus"+i)
+                var alarmSoundListItem = findChild(alarmSoundPage, "alarmSoundDelegate"+i)
                 var alarmSoundLabel = findChild(alarmSoundPage, "soundName"+i)
 
                 if(alarmSoundPage.alarmSound.subText === alarmSoundLabel.text) {
-                    alarmSoundSwitch.checked = true
+                    alarmSoundListItem.isChecked = true
                 }
 
                 else {
-                    alarmSoundSwitch.checked = false
+                    alarmSoundListItem.isChecked = false
                 }
             }
         }
@@ -86,15 +77,15 @@
         */
         function test_defaultAlarmSoundIsChecked() {
             for(var i=0; i<repeater.count; i++) {
-                var alarmSoundSwitch = findChild(alarmSoundPage, "soundStatus"+i)
+                var alarmSoundListItem = findChild(alarmSoundPage, "alarmSoundDelegate"+i)
                 var alarmSoundLabel = findChild(alarmSoundPage, "soundName"+i)
 
                 if(alarmSoundPage.alarmSound.subText === alarmSoundLabel.text) {
-                    compare(alarmSoundSwitch.checked, true, "Default alarm sound is not checked by default")
+                    compare(alarmSoundListItem.isChecked, true, "Default alarm sound is not checked by default")
                 }
 
                 else {
-                    compare(alarmSoundSwitch.checked, false, "Switch for alarm sounds not default is enabled incorrectly")
+                    compare(alarmSoundListItem.isChecked, false, "Switch for alarm sounds not default is enabled incorrectly")
                 }
             }
         }
@@ -104,19 +95,19 @@
         */
         function test_onlyOneAlarmSoundIsSelected() {
             // Click on some random alarm sounds
-            var secondSwitch = findChild(alarmSoundPage, "soundStatus"+2)
+            var secondSwitch = findChild(alarmSoundPage, "alarmSoundDelegate"+2)
             mouseClick(secondSwitch, centerOf(secondSwitch).x, centerOf(secondSwitch).y)
 
-            var fourthSwitch = findChild(alarmSoundPage, "soundStatus"+4)
+            var fourthSwitch = findChild(alarmSoundPage, "alarmSoundDelegate"+4)
             mouseClick(fourthSwitch, centerOf(fourthSwitch).x, centerOf(fourthSwitch).y)
 
             // Check if only that alarm sound is check while the rest is disabled
             for(var i=0; i<repeater.count; i++) {
-                var alarmSoundSwitch = findChild(alarmSoundPage, "soundStatus"+i)
+                var alarmSoundListItem = findChild(alarmSoundPage, "alarmSoundDelegate"+i)
                 var alarmSoundLabel = findChild(alarmSoundPage, "soundName"+i)
 
                 if(i !== 4) {
-                    compare(alarmSoundSwitch.checked, false, "More than one alarm sound selected")
+                    compare(alarmSoundListItem.isChecked, false, "More than one alarm sound selected")
                 }
             }
         }
@@ -128,11 +119,11 @@
         function test_soundListHasNoEmptySelection() {
 
             for(var i=0; i<repeater.count; i++) {
-                var alarmSoundSwitch = findChild(alarmSoundPage, "soundStatus"+i)
+                var alarmSoundListItem = findChild(alarmSoundPage, "alarmSoundDelegate"+i)
 
-                if(alarmSoundSwitch.checked) {
-                    mouseClick(alarmSoundSwitch, centerOf(alarmSoundSwitch).x, centerOf(alarmSoundSwitch.height).y)
-                    compare(alarmSoundSwitch.checked, true, "Clicking on the only selected alarm sound disabled it")
+                if(alarmSoundListItem.isChecked) {
+                    mouseClick(alarmSoundListItem, centerOf(alarmSoundListItem).x, centerOf(alarmSoundListItem.height).y)
+                    compare(alarmSoundListItem.isChecked, true, "Clicking on the only selected alarm sound disabled it")
                     break;
                 }
             }
@@ -146,14 +137,20 @@
             compare(saveButton.enabled, false, "save header button is not disabled despite no alarm sound change")
 
             // Change sound and check if save button is enabled
-            var fifthSwitch = findChild(alarmSoundPage, "soundStatus"+5)
+            var fifthSwitch = findChild(alarmSoundPage, "alarmSoundDelegate"+5)
             mouseClick(fifthSwitch, centerOf(fifthSwitch).x, centerOf(fifthSwitch).y)
             compare(saveButton.enabled, true, "save header button is not enabled despite alarm sound change")
 
             // Set sound to old value and check if save button is disabled
-            var firstSwitch = findChild(alarmSoundPage, "soundStatus"+1)
-            mouseClick(firstSwitch, centerOf(firstSwitch).x, centerOf(firstSwitch).y)
-            compare(saveButton.enabled, false, "save header button is not disabled despite no alarm sound change")
+            for(var i=0; i<repeater.count; i++) {
+                var alarmSoundListItem = findChild(alarmSoundPage, "alarmSoundDelegate"+i)
+                var alarmSoundLabel = findChild(alarmSoundPage, "soundName"+i)
+
+                if(alarmSoundLabel.text === "Bliss") {
+                    mouseClick(alarmSoundListItem, centerOf(alarmSoundListItem).x, centerOf(alarmSoundListItem).y)
+                    compare(saveButton.enabled, false, "save header button is not disabled despite no alarm sound change")
+                }
+            }
         }
     }
 }