Code review comment for lp:~macslow/unity8/snap-decisions-states

Revision history for this message
Michael Zanetti (mzanetti) wrote :

15 + var result = "undefined";

I think this should be "" instead of "undefined" otherwise leaving a state might not reset stuff correctly.

===

18 + if (notificationList.currentIndex == index) {
and following lines...

You shouldn't reach out of scope here (Can break if someone. Please either user ListView.view instead of notificationList or move the logic out into the containing ListView.

===
 small coding style nitpick

70 anchors.fill: parent
71 - anchors.margins: notification.margins
72 + anchors.leftMargin: notification.margins
73 + anchors.rightMargin: notification.margins

anchors { fill: parent; ... }

===

117 + Component.onCompleted: {
118 + if (fullscreen) {
119 + notificationList.spacing = 0
120 + } else {
121 + notificationList.spacing = units.gu(.5)
122 + }
123 + }

Why can't this be a binding in the ListView? Not sure why this needs to be called every time a new delegate is created.
===

Not sure if related to this branch (and if its a real issue after all), but if I trigger sd-example-incoming-call.py and then decline the incoming call the notification doesn't go away (it works fine if I accept the call)

===

I'm calling this in a script:

./sd-example-incoming-call-canceled.py &
./sd-example-incoming-call.py &
./sd-example-incoming-file.py &
./sd-example-morning-alarm.py &
./sd-example-much-body-text.py &
./sd-example-password-entry.py &
./sd-example-simunlock.py &
./sd-example-user-auth.py &
./sd-example-using-button-tint-hint.py &

=> unity crashes.

After that I added a sleep 1 in between the lines, it still causes issues and at some point an empty rectangle appears.

===

If the first notification is expanded, and a new one comes in at position 0, the new one shows up expanded and then quickly collapses. I think that might be related to this:

125 + ListView.onAdd: {
126 + if (notificationList.currentIndex < 1 && notificationList.count > 1) {
127 + notificationList.currentIndex = 1
128 + }

The problem here is that you only correct the currentIndex after everything is built up. If you make it a binding it should work better.

===

207 + wait(500);

Is that a leftover from testing?

===

tests look good. Maybe rename them to give it a more meaningful name? make testVisualQueue does not really tell its notifications.

review: Needs Fixing

« Back to merge proposal