Merge lp:~jonas-drange/ubuntu-system-settings/about-dynamic-imei-fixes-1205294 into lp:ubuntu-system-settings

Proposed by Jonas G. Drange
Status: Merged
Approved by: Sebastien Bacher
Approved revision: 1211
Merged at revision: 1228
Proposed branch: lp:~jonas-drange/ubuntu-system-settings/about-dynamic-imei-fixes-1205294
Merge into: lp:ubuntu-system-settings
Diff against target: 64 lines (+19/-10)
2 files modified
plugins/about/PageComponent.qml (+19/-3)
tests/autopilot/ubuntu_system_settings/tests/test_about.py (+0/-7)
To merge this branch: bzr merge lp:~jonas-drange/ubuntu-system-settings/about-dynamic-imei-fixes-1205294
Reviewer Review Type Date Requested Status
Sebastien Bacher (community) Approve
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+241977@code.launchpad.net

Commit message

[about] properly display IMEI values for no devices ("None"), one device and multiple devices. Remove test now redundant.

Description of the change

Shows multiple IMEIs in a multivalue list according to spec [1].

I have removed the test that has become redundant by doing this.

[1] https://wiki.ubuntu.com/AboutThisDevice#phone

To post a comment you must log in.
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks, is the gsettings/simnames snippet part of that changeset or some other work? It doesn't seme required for emei display...

Otherwise looks fine, might be better to only have 2 items though? The first one could have "visible: length <= 1" and "value: length = 1 ? imei : "none""

The second point is not a blocker, mostly a matter of taste so feel free to keep your version

review: Needs Information
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

Thanks @seb128, that was indeed part of an older version where SIM names appeared before the relevant IMEI. After talking to mpt, this was changed to no mention of SIM names, which makes more sense.

Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks, small nitpick

> value: modemsSorted.length ? (imeiNumber || i18n.tr("None"))

the (imeiNumber || i18n.tr("None")) doesn't seem an obvious syntax to me, what would think of using
"value (modemsSorted.length && imeiNumber) ? imeiNumber : i18n.tr("None"))"
instead?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Sebastien Bacher (seb128) wrote :

Looks fine to me, but do you have any clue with those CI tests are failing?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Sebastien Bacher (seb128) wrote :

The tests failing is issues with the CI machines, the changes look good so let's approve it

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/about/PageComponent.qml'
--- plugins/about/PageComponent.qml 2014-10-31 14:53:32 +0000
+++ plugins/about/PageComponent.qml 2014-11-19 12:03:52 +0000
@@ -33,6 +33,7 @@
3333
34 title: i18n.tr("About this phone")34 title: i18n.tr("About this phone")
35 flickable: scrollWidget35 flickable: scrollWidget
36 property var modemsSorted: manager.modems.slice(0).sort()
3637
37 UbuntuStorageAboutPanel {38 UbuntuStorageAboutPanel {
38 id: backendInfos39 id: backendInfos
@@ -52,7 +53,7 @@
52 if (manager.modems.length === 1) {53 if (manager.modems.length === 1) {
53 phoneNumbers.setSource("PhoneNumber.qml", {path: manager.modems[0]})54 phoneNumbers.setSource("PhoneNumber.qml", {path: manager.modems[0]})
54 } else if (manager.modems.length > 1) {55 } else if (manager.modems.length > 1) {
55 phoneNumbers.setSource("PhoneNumbers.qml", {paths: manager.modems.slice(0).sort()})56 phoneNumbers.setSource("PhoneNumbers.qml", {paths: modemsSorted})
56 }57 }
57 }58 }
58 }59 }
@@ -117,8 +118,23 @@
117 property string imeiNumber118 property string imeiNumber
118 imeiNumber: deviceInfos.imei(0)119 imeiNumber: deviceInfos.imei(0)
119 text: "IMEI"120 text: "IMEI"
120 value: imeiNumber121 value: modemsSorted.length ? (imeiNumber || i18n.tr("None")) :
121 visible: imeiNumber122 i18n.tr("None")
123 visible: modemsSorted.length <= 1
124 }
125
126 ListItem.MultiValue {
127 text: "IMEI"
128 objectName: "imeiItems"
129 values: {
130 var imeis = [];
131 modemsSorted.forEach(function (path, i) {
132 var imei = deviceInfos.imei(i);
133 imei ? imeis.push(imei) : imeis.push(i18n.tr("None"));
134 });
135 return imeis;
136 }
137 visible: modemsSorted.length > 1
122 }138 }
123139
124 ListItem.SingleValue {140 ListItem.SingleValue {
125141
=== modified file 'tests/autopilot/ubuntu_system_settings/tests/test_about.py'
--- tests/autopilot/ubuntu_system_settings/tests/test_about.py 2014-09-03 14:48:02 +0000
+++ tests/autopilot/ubuntu_system_settings/tests/test_about.py 2014-11-19 12:03:52 +0000
@@ -132,13 +132,6 @@
132 displayed_imei = self.about_page.get_imei()132 displayed_imei = self.about_page.get_imei()
133 self.assertThat(displayed_imei, Equals(device_imei))133 self.assertThat(displayed_imei, Equals(device_imei))
134134
135 def test_device_without_imei_must_not_display_it(self):
136 device_imei = self._get_imei_from_dbus()
137 if device_imei:
138 self.skipTest('The device has imei.')
139 else:
140 self.assertFalse(self.about_page.is_imei_visible())
141
142 def test_phone_number(self):135 def test_phone_number(self):
143 number = self.system_settings.main_view.about_page.select_single(136 number = self.system_settings.main_view.about_page.select_single(
144 objectName="numberItem"137 objectName="numberItem"

Subscribers

People subscribed via source and target branches