ubuntu-touch-coreapps-reviewers team mailing list archive

[Bartosz Kosiorek <gang65@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Some small coding style remarks.


Diff comments:

> === 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
> @@ -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)

Spaces around "*"

>  
>          Column {
>              id: _alarmSoundColumn
> 
> === 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
> @@ -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)

spaces around "+"

>                  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)

spaces around "+"

>                  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)

spaces around "+"

>                  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")
>                  }
>              }
>          }
> @@ -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)

spaces around "+"

> +
> +                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")
> +                }
> +            }
>          }
>      }
>  }


-- 
https://code.launchpad.net/~nik90/ubuntu-clock-app/replace-alarmsound-checkbox/+merge/269328
Your team Ubuntu Clock Developers is subscribed to branch lp:ubuntu-clock-app.