Code review comment for lp:~macslow/unity/phablet-notification-renderer

Tests
========================

627 + wait(0)

Looks unnecessary?

========

643 + interactiveAreaSpy.clear()
644 + clickThroughSpy.clear()

651 + clickThroughSpy.clear()

You should do that in a « function cleanup() » instead - it's called after every test case.

========

646 + interactiveAreaSpy.wait()
647 + verify(interactiveAreaSpy.count == 1)

653 + clickThroughSpy.wait()
654 + verify(clickThroughSpy.count == 1)

SignalSpy.wait() already verifies the count is 1.

========

648 + verify(clickThroughSpy.count == 0)

Should use compare().

========

649 + }
650 + else {

Please keep in one line.

========

638 + compare(icon.visible, data.iconVisible, "check for icon of notification" + (mockModel.count - 1))
639 + compare(interactiveArea.visible, data.interactiveAreaVisible, "check for interactive notification" + (mockModel.count - 1))

657 + compare(secondaryIcon.visible, data.secondaryIconVisible, "check for secondary icon" + (mockModel.count - 1))
658 + compare(summaryLabel.visible, data.summaryVisible, "check for summary-label" + (mockModel.count - 1))
659 + compare(bodyLabel.visible, data.bodyVisible, "check for body-label" + (mockModel.count - 1))
660 + compare(buttonRow.visible, data.buttonRowVisible, "check for buttons" + (mockModel.count - 1))

The messages could be improved, and also "notification1", "notification2" are not really useful - the tag: from _data() shows which test is being run.

========

662 + if(data.buttonRowVisible) {
663 + var buttonCancel = findChild(buttonRow, "button0")
664 + var buttonAccept = findChild(buttonRow, "button1")
665 +
666 + buttonSpy.target = buttonCancel
667 + buttonSpy.clear()
668 + mouseClick(buttonCancel, buttonCancel.width / 2, buttonCancel.height / 2)
669 + buttonSpy.wait()
670 + verify(buttonSpy.count == 1)
671 +
672 + buttonSpy.target = buttonAccept
673 + buttonSpy.clear()
674 + mouseClick(buttonAccept, buttonAccept.width / 2, buttonAccept.height / 2)
675 + buttonSpy.wait()
676 + verify(buttonSpy.count == 1)
677 + }

This only tests that Button.clicked signal is working. You should instead create an actionInvoked(string) signal on Notification and spy on this signal, making sure that SignalSpy.signalArguments are as expected.

Another run through the code itself
===================================

 46 + width: parent.width
 47 + height: childrenRect.height

Again, top-level components in a file should not have width or height defined. Use implicitHeight and get rid of width altogether - it's determined by the delegate definition in Notifications.qml.

========

 57 + visible: type == "Notifications.Type.Interactive"

MouseAreas are not graphical, use "enabled" instead.

========

 58 + onClicked: print("clicked " + actions.get(0).id)

Forward through actionInvoked(string) signal mentioned above.

==================================

Going into a meeting, more in a bit.