Merge lp:~nik90/ubuntu-clock-app/improve-header-confirmation-behavior into lp:ubuntu-clock-app

[Nekhelesh Ramananthan]

Fixes the confusing back & save button behaviour in the alarm pages.

This MP fixes the confusing back & save button behaviour in the alarm pages.

Original issue was that the back button was sometimes used for "Saving" & "Going Back". This alternating behaviour was confusing to users as seen in the bug report attached. So according to UX Suggestion, the back button will now behave as "Go Back" or "Cancel" action. While the save button will be the save to confirm your changes.

[Nekhelesh Ramananthan (nik90)]

Need to fix unit tests

review: Needs Fixing

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:330
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/739/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/84/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/739/rebuild

review: Needs Fixing (continuous-integration)

[Nekhelesh Ramananthan (nik90)]

Fixed unit tests

review: Abstain

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:333
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/740/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/85

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/740/rebuild

review: Approve (continuous-integration)

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:334
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/741/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/86/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/741/rebuild

review: Needs Fixing (continuous-integration)

[Bartosz Kosiorek (gang65)]

It works perfectly for me

review: Approve

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:335
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/743/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/88

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/743/rebuild

review: Approve (continuous-integration)

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://91.189.93.70:8080/job/ubuntu-clock-app-autolanding/281/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-autolanding/38/console

review: Needs Fixing (continuous-integration)

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

review: Approve (continuous-integration)

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Autolanding.
Unapproved changes made after approval.
http://91.189.93.70:8080/job/ubuntu-clock-app-autolanding/283/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-autolanding/40

review: Needs Fixing (continuous-integration)

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:336
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/745/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/90

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/745/rebuild

review: Approve (continuous-integration)

=== modified file 'app/alarm/AlarmLabel.qml'
--- app/alarm/AlarmLabel.qml	2015-05-27 16:03:23 +0000
+++ app/alarm/AlarmLabel.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-2015 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *
@@ -26,6 +26,9 @@
     // Property to set the alarm label in the edit alarm page
     property var alarm
 
+    // Property to store the old alarm label to detect if user changed it or not
+    property string oldAlarmLabel: alarm.message
+
     visible: false
     title: i18n.tr("Label")
 
@@ -33,7 +36,19 @@
         id: backAction
         iconName: "back"
         onTriggered: {
-            alarm.message = _labelEntry.text
+            // Restore old alarm label if user presses the back button
+            alarm.message = oldAlarmLabel
+            pop()
+        }
+    }
+
+    head.actions: Action {
+        id: saveAction
+        objectName: "saveAction"
+        iconName: "tick"
+        enabled: oldAlarmLabel !== _labelEntry.text.trim() && _labelEntry.text.trim()
+        onTriggered: {
+            alarm.message = _labelEntry.text.trim()
             pop()
         }
     }
@@ -63,10 +78,6 @@
             text: alarm.message
             width: parent.width
             inputMethodHints: Qt.ImhNoPredictiveText
-
-            onTextChanged: {
-                backAction.enabled = !!text.trim()
-            }
         }
     }
 }

=== modified file 'app/alarm/AlarmRepeat.qml'
--- app/alarm/AlarmRepeat.qml	2015-06-03 22:49:29 +0000
+++ app/alarm/AlarmRepeat.qml	2015-08-12 21:01:35 +0000
@@ -29,9 +29,31 @@
     // Property to hold the alarm utils functions passed from edit alarm page
     property var alarmUtils
 
+    // Property to store the previously set alarm frequency to detect user changes
+    property int oldAlarmDaysOfWeek
+
     visible: false
     title: i18n.tr("Repeat")
 
+    // Function to detect if alarm is OneTime or Repeating
+    function detectAlarmType() {
+        if (alarm.daysOfWeek > 0) {
+            alarm.type = Alarm.Repeating
+        } else {
+            alarm.type = Alarm.OneTime
+        }
+    }
+
+    head.backAction: Action {
+        iconName: "back"
+        onTriggered: {
+            // Restore alarm frequency and type if user presses the back button
+            alarm.daysOfWeek = oldAlarmDaysOfWeek
+            detectAlarmType()
+            pop()
+        }
+    }
+
     head.actions: [
         Action {
             text: i18n.tr("Select All")
@@ -56,6 +78,16 @@
                     }
                 }
             }
+        },
+
+        Action {
+            id: saveAction
+            objectName: "saveAction"
+            iconName: "tick"
+            enabled: oldAlarmDaysOfWeek !== alarm.daysOfWeek
+            onTriggered: {
+                pop()
+            }
         }
     ]
 
@@ -69,6 +101,9 @@
     Component.onCompleted: {
         if (alarm.type === Alarm.OneTime)
             alarm.daysOfWeek = 0
+
+        // Record the current alarm repeat values (frequency)
+        oldAlarmDaysOfWeek = alarm.daysOfWeek
     }
 
     Component.onDestruction: {
@@ -147,11 +182,7 @@
                             alarm.daysOfWeek &= ~flag
                         }
 
-                        if (alarm.daysOfWeek > 0) {
-                            alarm.type = Alarm.Repeating
-                        } else {
-                            alarm.type = Alarm.OneTime
-                        }
+                        detectAlarmType()
                     }
                 }
 

=== modified file 'app/alarm/AlarmSound.qml'
--- app/alarm/AlarmSound.qml	2015-06-03 22:49:29 +0000
+++ app/alarm/AlarmSound.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-15 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *
@@ -37,9 +37,35 @@
     // Property to set the alarm sound model in the edit alarm page
     property var soundModel
 
+    /*
+     Properties to store the previously set alarm sound values to detect
+     if the user changed the alarm sound or not
+    */
+    property url oldAlarmSoundUrl
+    property string oldAlarmSoundName
+
+    Component.onCompleted: {
+        // Record the current alarm sound values (url, name)
+        oldAlarmSoundUrl = alarm.sound
+        oldAlarmSoundName = alarmSound.subText
+    }
+
     head.backAction: Action {
         iconName: "back"
         onTriggered: {
+            // Restore alarm sound to old values (url, name) if the back button is pressed
+            alarm.sound = oldAlarmSoundUrl
+            alarmSound.subText = oldAlarmSoundName
+            pop()
+        }
+    }
+
+    head.actions: Action {
+        id: saveAction
+        objectName: "saveAction"
+        iconName: "tick"
+        enabled: oldAlarmSoundUrl !== alarm.sound
+        onTriggered: {
             pop()
         }
     }

=== modified file 'debian/changelog'
--- debian/changelog	2015-08-12 20:45:23 +0000
+++ debian/changelog	2015-08-12 21:01:35 +0000
@@ -6,6 +6,7 @@
   
   [Nekhelesh Ramananthan]
   * Increase the height of times in the alarm screen (LP: #1365428)
+  * Fixed the confusing behavior of the confirmation button (LP: #1408015)
   * Added README.mergeproposal checklist to help with the review process.
   * Fix alarm interval information being inconsistent (LP: #1466000)
 

=== modified file 'tests/unit/tst_alarm.qml'
--- tests/unit/tst_alarm.qml	2015-07-15 22:31:52 +0000
+++ tests/unit/tst_alarm.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-2015 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *

=== modified file 'tests/unit/tst_alarmLabel.qml'
--- tests/unit/tst_alarmLabel.qml	2015-06-04 21:46:37 +0000
+++ tests/unit/tst_alarmLabel.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-2015 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *
@@ -44,12 +44,14 @@
         property var header
         property var alarmLabel
         property var backButton
+        property var saveButton
 
         function initTestCase() {
             alarmLabelPage.visible = true
             header = findChild(mainView, "MainView_Header")
             alarmLabel = findChild(alarmLabelPage, "labelEntry")
             backButton = findChild(header, "customBackButton")
+            saveButton = findChild(header, "saveAction_header_button")
         }
 
         /*
@@ -60,29 +62,63 @@
             compare(alarmLabel.focus, true, "Alarm Label does not have focus by default")
         }
 
-        function test_backButtonEnabled_data() {
+        function test_saveButtonEnabled_data() {
             return [
-                        {tag: "EmptyAlarmLabel",       string: "",           enableStatus: false},
-                        {tag: "BlankSpacesAlarmLabel", string: "   ",        enableStatus: false},
-                        {tag: "FilledAlarmLabel",      string: "Test Label", enableStatus: true}
+                        {tag: "SameAlarmLabel",        string: "Alarm",      enableStatus: false, error: "Save button is enabled despite no alarm name change!"},
+                        {tag: "EmptyAlarmLabel",       string: "",           enableStatus: false, error: "Save button is enabled despite alarm name being empty!" },
+                        {tag: "BlankSpacesAlarmLabel", string: "   ",        enableStatus: false, error: "Save button is enabled despite alarm name being empty!" },
+                        {tag: "FilledAlarmLabel",      string: "Test Label", enableStatus: true,  error: "Save button is disabled despite alarm name being different!" }
                     ]
         }
 
         /*
-         Test to check if the back header button is enabled/disabled correctly
+         Test to check if the save header button is enabled/disabled correctly
          for different alarm label scenarios.
         */
-        function test_backButtonEnabled(data) {
+        function test_saveButtonEnabled(data) {
             compare(alarmLabel.text, "Alarm", "Default alarm label is not Alarm")
-            compare(backButton.enabled, true, "Back header button is not enabled by default")
+            compare(saveButton.enabled, false, "save header button is not disabled despite no alarm name change")
 
             clearTextField(alarmLabel)
             typeString(data.string)
 
             compare(alarmLabel.text, data.string, "Alarm label is not what was type in the textfield")
-            compare(backButton.enabled, data.enableStatus, "Back Button enable status is not as expected")
-
-            alarmLabel.text = _alarm.message
+            compare(saveButton.enabled, data.enableStatus, data.error)
+
+            alarmLabel.text = _alarm.message
+        }
+
+        /*
+         Test to check if the back button correctly restores the alarm name to
+         the old value when the back button is pressed
+        */
+
+        function test_backButtonRestoresValues() {
+            compare(alarmLabel.text, "Alarm", "Default alarm label is not Alarm")
+
+            clearTextField(alarmLabel)
+            typeString("New Alarm Label")
+            mouseClick(backButton, centerOf(backButton).x, centerOf(backButton).y)
+
+            compare(_alarm.message, "Alarm", "Alarm name is restored to the old value")
+
+            alarmLabel.text = _alarm.message
+        }
+
+        /*
+         Test to check if the save button correctly saves the new alarm name to
+         the alarm object when the save button is pressed
+        */
+        function test_saveButtonSavesNewValues() {
+            compare(alarmLabel.text, "Alarm", "Default alarm label is not Alarm")
+            compare(_alarm.message, "Alarm", "Default alarm message is not Alarm")
+
+            clearTextField(alarmLabel)
+            typeString("New Alarm Label")
+            pressHeaderButton(header, "saveAction")
+
+            compare(_alarm.message, "New Alarm Label", "Alarm message has not changed despite pressing the save button")
+            _alarm.reset()
         }
 
         /*

=== modified file 'tests/unit/tst_alarmRepeat.qml'
--- tests/unit/tst_alarmRepeat.qml	2015-06-04 21:46:37 +0000
+++ tests/unit/tst_alarmRepeat.qml	2015-08-12 21:01:35 +0000
@@ -16,7 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import QtQuick 2.0
+import QtQuick 2.4
 import QtTest 1.0
 import Ubuntu.Test 1.0
 import Ubuntu.Components 1.2
@@ -59,6 +59,7 @@
 
         property var header
         property var backButton
+        property var saveButton
         property var repeater
         property var today: Qt.locale().standaloneDayName(
                                 new Date().getDay(), Locale.LongFormat)
@@ -69,6 +70,7 @@
             spy.target = alarmRepeatPageLoader.item.Component
             header = findChild(mainView, "MainView_Header")
             backButton = findChild(header, "customBackButton")
+            saveButton = findChild(header, "saveAction_header_button")
             repeater = findChild(alarmRepeatPageLoader.item, "alarmDays")
         }
 
@@ -106,6 +108,21 @@
         }
 
         /*
+         Test to check if the save button is disabled when no changes have been made
+         and enabled when changes are made.
+        */
+        function test_saveButtonIsDisabledOnNoChanges() {
+            waitForRendering(alarmRepeatPageLoader.item);
+
+            tryCompare(_alarm, "daysOfWeek", 0, 3000, "Alarm days of weeks is not 0 by default")
+            compare(saveButton.enabled, false, "save header button is not disabled despite no alarm frequency change")
+
+            var randomDaySwitch = findChild(alarmRepeatPageLoader.item, "daySwitch"+3)
+            mouseClick(randomDaySwitch, centerOf(randomDaySwitch).x, centerOf(randomDaySwitch).y)
+            compare(saveButton.enabled, true, "save header button is not disabled despite no alarm frequency change")
+        }
+
+        /*
          Test to check if the alarm types are being correctly changed when
          toggling some of the swtiches. So if a switch is toggle, the alarm
          should become a repeating alarm. if no switches are enabled then

=== modified file 'tests/unit/tst_alarmSound.qml'
--- tests/unit/tst_alarmSound.qml	2015-06-04 21:46:37 +0000
+++ tests/unit/tst_alarmSound.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-2015 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *
@@ -31,6 +31,7 @@
 
     Alarm {
         id: _alarm
+        sound: "file:///usr/share/sounds/ubuntu/ringtones/Bliss.ogg"
     }
 
     FolderListModel {
@@ -55,14 +56,17 @@
         when: windowShown
 
         property var repeater
+        property var saveButton
 
         function initTestCase() {
             alarmSoundPage.visible = true
             repeater = findChild(alarmSoundPage, "alarmSounds")
+            saveButton = findChild(header, "saveAction_header_button")
         }
 
         function cleanup() {
             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 alarmSoundLabel = findChild(alarmSoundPage, "soundName"+i)
@@ -99,20 +103,19 @@
          Test to check if only one alarm sound is checked at all times
         */
         function test_onlyOneAlarmSoundIsSelected() {
-
             // Click on some random alarm sounds
             var secondSwitch = findChild(alarmSoundPage, "soundStatus"+2)
             mouseClick(secondSwitch, centerOf(secondSwitch).x, centerOf(secondSwitch).y)
 
-            var fourthSwitch = findChild(alarmSoundPage, "soundStatus"+2)
-            mouseClick(fourthSwitch, centerOf(fourthSwitch.width).x, centerOf(fourthSwitch.height).y)
+            var fourthSwitch = findChild(alarmSoundPage, "soundStatus"+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 alarmSoundLabel = findChild(alarmSoundPage, "soundName"+i)
 
-                if(i !== 2) {
+                if(i !== 4) {
                     compare(alarmSoundSwitch.checked, false, "More than one alarm sound selected")
                 }
             }
@@ -134,5 +137,23 @@
                 }
             }
         }
+
+        /*
+         Test to check if the save button is disabled when no changes have been made
+         and enabled when changes are made.
+        */
+        function test_saveButtonIsDisabledOnNoChanges() {
+            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)
+            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")
+        }
     }
 }

=== modified file 'tests/unit/tst_alarmUtils.qml'
--- tests/unit/tst_alarmUtils.qml	2015-08-05 13:13:05 +0000
+++ tests/unit/tst_alarmUtils.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-2015 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *

=== modified file 'tests/unit/tst_worldClock.qml'
--- tests/unit/tst_worldClock.qml	2015-07-15 22:31:52 +0000
+++ tests/unit/tst_worldClock.qml	2015-08-12 21:01:35 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Canonical Ltd
+ * Copyright (C) 2014-2015 Canonical Ltd
  *
  * This file is part of Ubuntu Clock App
  *