Merge lp:~ken-vandine/ubuntu-system-settings/lp1542098 into lp:ubuntu-system-settings

[Ken VanDine]

Dropped the spinner in the connected list, i don't think it's ever visible but it shows a vertical divider

Dropped the spinner in the connected list, i don't think it's ever visible but it shows a vertical divider

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1593
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2603/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6360
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/377
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/786
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/4879
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6371
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6371/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27350

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2603/rebuild

review: Needs Fixing (continuous-integration)

[Matthew Paul Thomas (mpt)]

Did you mean “it’s ever visible”? It's supposed to be visible when a device is pairing automatically, so removing it altogether isn’t right. <https://wiki.ubuntu.com/Bluetooth#Listing_devices>

Anyway, we recorded this problem in the Bluetooth panel just because that was a panel we happened to go into while pairing a keyboard with the test tablet. If we’d gone into the Wi-Fi panel instead, we would have recorded the problem there. The dividers are wrong everywhere they appear, so amputating one callsite won’t fix the problem.

Please remove the dividers in the toolkit instead.

review: Disapprove (design)

[Ken VanDine (ken-vandine)]

> Did you mean “it’s ever visible”? It's supposed to be visible when a device is
> pairing automatically, so removing it altogether isn’t right.
> <https://wiki.ubuntu.com/Bluetooth#Listing_devices>
>
> Anyway, we recorded this problem in the Bluetooth panel just because that was
> a panel we happened to go into while pairing a keyboard with the test tablet.
> If we’d gone into the Wi-Fi panel instead, we would have recorded the problem
> there. The dividers are wrong everywhere they appear, so amputating one
> callsite won’t fix the problem.
>
> Please remove the dividers in the toolkit instead.

AFAICT, that spinner isn't displayed because the device doesn't get put into the connected device model until it's already connected.

[Matthew Paul Thomas (mpt)]

So you are seeing bug bug 1488885. But we did not see it on Tuesday, and I am not seeing it today: the spinner appears in the list item when pairing it manually, which is what’s supposed to happen. <https://wiki.ubuntu.com/Bluetooth#Pairing_manually> If the spinner was never displayed, the divider would never be displayed either, so there would be no point in making this change anyway!

This is a bit muddled by a connecting device moving to the “Connected devices:” list prematurely, which I’ve just reported as bug 1542368. But whether that bug is fixed or not, removing the spinner would be confusing. As long as it’s unfixed, the lack of spinner would give the impression that a device has paired instantly when it has not. And when it is fixed, the lack of spinner would give the impression that choosing “Connect” had no effect.

[lp:~ken-vandine/ubuntu-system-settings/lp1542098]

1594. By Ken VanDine on 2016-02-05

Added back the ActivityIndicator but commented out with a comment explaining we want this when we switch to the new ListItem

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1594
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2607/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6373
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/381
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/790
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/4888
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6384
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6384/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27371

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2607/rebuild

review: Needs Fixing (continuous-integration)

[Matthew Paul Thomas (mpt)]

Please, no, we don’t want the spinner just “when we switch to the new list item”. The spinner already exists, and removing it would make the UI worse in the way I described. That new problem would be much worse than the visual glitch of the divider.

Bug 1542098 is one of a grab-bag of problems, varying hugely in severity, that we happened to notice in the parts of the UI that we happened to look at in two hours. They are not the worst problems on the device. (If Bill got that impression, please ask him to call Ben who will be happy to clarify this.) And certainly this one is not so critical that it is worth fixing by making the UI more confusing.

Unmerged revisions

1594. By Ken VanDine on 2016-02-05

Added back the ActivityIndicator but commented out with a comment explaining we want this when we switch to the new ListItem

1593. By Ken VanDine on 2016-02-05

Dropped the spinner in the connected list, i don't think it's every visible but it shows a vertical divider

=== modified file 'plugins/bluetooth/PageComponent.qml'
--- plugins/bluetooth/PageComponent.qml	2016-01-28 14:22:51 +0000
+++ plugins/bluetooth/PageComponent.qml	2016-02-05 18:03:31 +0000
@@ -303,10 +303,13 @@
                         iconSource: iconPath
                         iconFrame: false
                         text: getDisplayName(type, displayName)
-                        control: ActivityIndicator {
-                            visible: connection == Device.Connecting
-                            running: visible
-                        }
+                        /* Dropped to work around #1542098, we should 
+                         * add this back when we switch to the new listitem
+                         * control: ActivityIndicator {
+                         *     visible: connection == Device.Connecting
+                         *     running: visible
+                         * }
+                         */
                         onClicked: {
                             backend.setSelectedDevice(addressName);
                             pageStack.push(Qt.resolvedUrl("DevicePage.qml"), {backend: backend, root: root});