ubuntu-touch-coreapps-reviewers team mailing list archive

[Zsombor Egri <zsombor.egri@xxxxxxxxxxxxx>]

Small comments inline, you can think of applying that right away or in a new MR.

Diff comments:

> === modified file 'app/alarm/AlarmList.qml'
> --- app/alarm/AlarmList.qml	2014-10-11 18:56:01 +0000
> +++ app/alarm/AlarmList.qml	2015-03-06 17:13:42 +0000
> @@ -129,7 +129,7 @@
>  
>              else {
>                  pageStack.push(Qt.resolvedUrl("EditAlarmPage.qml"),
> -                               {isNewAlarm: false, alarmIndex: index, alarmModel: alarmModel})
> +                               {isNewAlarm: false, tempAlarm: model})
>              }

I suggest you to use one alarm property in the EditAlarmPage. You can set it as default to an Alarm{} you use when the alarm is a new one, or override with the one from the list when you edit one. It'll make the application page lot simpler, no need for separate updateAlarm() or saneNewAlarm() functions anymore, the same alarm.save() would save or update the alarm.

EditAlarmPage.qml:

Page {
    // [...]
    title: "i18n.tr("New Alarm")
    property Alarm alarm: newAlarm
    Alarm { id: newAlarm }
}

Then when you push it:

PageStack.push(Qt.resolvedUrl("EditAlarmPage.qml", {alarm: model, title: "it8n.tr("Edit Alarm")})

>          }
>  
> 
> === modified file 'app/alarm/EditAlarmPage.qml'
> --- app/alarm/EditAlarmPage.qml	2015-01-22 00:11:57 +0000
> +++ app/alarm/EditAlarmPage.qml	2015-03-06 17:13:42 +0000
> @@ -31,15 +31,9 @@
>      // Property to determine if this is a new/saved alarm
>      property bool isNewAlarm: true
>  
> -    // Property to store the index of the alarm to be edited
> -    property int alarmIndex
> -
>      // Temporary alarm used to read saved alarm and modify them
>      property var tempAlarm
>  
> -    // Property to store the alarm model
> -    property var alarmModel
> -
>      title: isNewAlarm ? i18n.tr("New alarm") : i18n.tr("Edit alarm")
>      visible: false
>  
> @@ -82,8 +76,6 @@
>  
>      // Function to read a saved alarm
>      function readAlarm() {
> -        tempAlarm = alarmModel.get(alarmIndex)
> -
>          _alarm.message = tempAlarm.message
>          _alarm.type = tempAlarm.type
>          _alarm.daysOfWeek = tempAlarm.daysOfWeek
> @@ -94,7 +86,6 @@
>  
>      // Function to delete a saved alarm
>      function deleteAlarm() {
> -        tempAlarm = alarmModel.get(alarmIndex)
>          tempAlarm.cancel()
>  
>          if(validateAlarm(tempAlarm)) {
> @@ -104,8 +95,6 @@
>  
>      // Function to update a saved alarm
>      function updateAlarm() {
> -        tempAlarm = alarmModel.get(alarmIndex)
> -
>          var alarmTime = new Date()
>          alarmTime.setHours(_timePicker.hours, _timePicker.minutes, 0)
>  
> 
> === modified file 'debian/changelog'
> --- debian/changelog	2015-03-06 11:33:03 +0000
> +++ debian/changelog	2015-03-06 17:13:42 +0000
> @@ -10,6 +10,7 @@
>    * Fixed predefined cities and countries not being translatable in the timezone
>      selection dialog (LP: #1354466)
>    * Fixed empty state description not wrapping (LP: #1428165)
> +  * Fixed edit alarm crash issue in vivid (thanks to Zsombor)
>  
>    [Brendan Donegan]
>    * Fixed AP failure by waiting for the bottom edge tip visible property to be true
> 
> === modified file 'po/com.ubuntu.clock.pot'
> --- po/com.ubuntu.clock.pot	2015-03-06 10:54:21 +0000
> +++ po/com.ubuntu.clock.pot	2015-03-06 17:13:42 +0000
> @@ -8,7 +8,7 @@
>  msgstr ""
>  "Project-Id-Version: \n"
>  "Report-Msgid-Bugs-To: \n"
> -"POT-Creation-Date: 2015-03-06 11:52+0100\n"
> +"POT-Creation-Date: 2015-03-06 17:09+0000\n"
>  "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
>  "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
>  "Language-Team: LANGUAGE <LL@xxxxxx>\n"
> @@ -18,7 +18,7 @@
>  "Content-Transfer-Encoding: 8bit\n"
>  
>  #: ../app/alarm/AlarmLabel.qml:30 ../app/alarm/AlarmLabel.qml:56
> -#: ../app/alarm/EditAlarmPage.qml:302
> +#: ../app/alarm/EditAlarmPage.qml:291
>  msgid "Label"
>  msgstr ""
>  
> @@ -31,8 +31,8 @@
>  msgid "Alarms"
>  msgstr ""
>  
> -#: ../app/alarm/AlarmPage.qml:43 ../app/alarm/EditAlarmPage.qml:50
> -#: ../app/alarm/EditAlarmPage.qml:177
> +#: ../app/alarm/AlarmPage.qml:43 ../app/alarm/EditAlarmPage.qml:44
> +#: ../app/alarm/EditAlarmPage.qml:166
>  msgid "Alarm"
>  msgstr ""
>  
> @@ -56,7 +56,7 @@
>  msgid "Tap the + icon to add an alarm"
>  msgstr ""
>  
> -#: ../app/alarm/AlarmRepeat.qml:34 ../app/alarm/EditAlarmPage.qml:292
> +#: ../app/alarm/AlarmRepeat.qml:34 ../app/alarm/EditAlarmPage.qml:281
>  msgid "Repeat"
>  msgstr ""
>  
> @@ -94,7 +94,7 @@
>  msgid "Change time and date"
>  msgstr ""
>  
> -#: ../app/alarm/AlarmSound.qml:28 ../app/alarm/EditAlarmPage.qml:315
> +#: ../app/alarm/AlarmSound.qml:28 ../app/alarm/EditAlarmPage.qml:304
>  msgid "Sound"
>  msgstr ""
>  
> @@ -142,15 +142,15 @@
>  msgid "in %1m"
>  msgstr ""
>  
> -#: ../app/alarm/EditAlarmPage.qml:43
> +#: ../app/alarm/EditAlarmPage.qml:37
>  msgid "New alarm"
>  msgstr ""
>  
> -#: ../app/alarm/EditAlarmPage.qml:43
> +#: ../app/alarm/EditAlarmPage.qml:37
>  msgid "Edit alarm"
>  msgstr ""
>  
> -#: ../app/alarm/EditAlarmPage.qml:336
> +#: ../app/alarm/EditAlarmPage.qml:325
>  msgid "Delete alarm"
>  msgstr ""
>  
> 


-- 
https://code.launchpad.net/~nik90/ubuntu-clock-app/fix-alarm-crash/+merge/252150
Your team Ubuntu Clock Developers is requested to review the proposed merge of lp:~nik90/ubuntu-clock-app/fix-alarm-crash into lp:ubuntu-clock-app.