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.
--
Mailing list: https://launchpad.net/~ubuntu-touch-coreapps-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~ubuntu-touch-coreapps-reviewers
More help : https://help.launchpad.net/ListHelp
