Merge lp:~renatofilho/address-book-app/fix-1377334 into lp:~phablet-team/address-book-app/staging
- fix-1377334
- Merge into staging
Status: | Rejected |
---|---|
Rejected by: | Renato Araujo Oliveira Filho |
Proposed branch: | lp:~renatofilho/address-book-app/fix-1377334 |
Merge into: | lp:~phablet-team/address-book-app/staging |
Diff against target: |
380 lines (+142/-128) 4 files modified
src/imports/ContactEdit/AvatarImport.qml (+96/-91) src/imports/ContactEdit/ContactDetailAvatarEditor.qml (+14/-17) src/imports/ContactList/ContactExporter.qml (+28/-2) src/imports/ContactList/ContactListPage.qml (+4/-18) |
To merge this branch: | bzr merge lp:~renatofilho/address-book-app/fix-1377334 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ken VanDine | Approve | ||
PS Jenkins bot | continuous-integration | Needs Fixing | |
Review via email: mp+237480@code.launchpad.net |
Commit message
Implement AvatarImport as a Page.
Does not use dialog on content hub component, to avoid application to get stuck.
Handle operation aborted in pick mode.
Description of the change
Ken VanDine (ken-vandine) wrote : | # |
Ken VanDine (ken-vandine) wrote : | # |
I haven't run it yet, I'll wait for a CI build for that. But from the code review it looks good, see a couple inline comments.
Also, unrelated to this MP, I see there is a copyImage function to copy the file. There is now an API for that in content-hub, ContentItem.move(). Now that content-hub provides an API for that, you could consider switching to that.
- 315. By Renato Araujo Oliveira Filho
-
Fixed contact export.
Renato Araujo Oliveira Filho (renatofilho) wrote : | # |
> I haven't run it yet, I'll wait for a CI build for that. But from the code
> review it looks good, see a couple inline comments.
>
> Also, unrelated to this MP, I see there is a copyImage function to copy the
> file. There is now an API for that in content-hub, ContentItem.move(). Now
> that content-hub provides an API for that, you could consider switching to
> that.
The copyImage not only copy the image it resize it to a small size to be used as avatar.
Renato Araujo Oliveira Filho (renatofilho) : | # |
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:314
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:315
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Ken VanDine (ken-vandine) wrote : | # |
Works well on my mako, I tested the full path including adding the avatar from gallery as well as aborting the transfer. All worked well.
Unmerged revisions
- 315. By Renato Araujo Oliveira Filho
-
Fixed contact export.
- 314. By Renato Araujo Oliveira Filho
-
Implement AvatarImport as a Page.
Does not use dialogs on content hub component, to avoid application to get stuck.
Handle operation aborted in pick mode.
Preview Diff
1 | === modified file 'src/imports/ContactEdit/AvatarImport.qml' | |||
2 | --- src/imports/ContactEdit/AvatarImport.qml 2014-08-13 20:52:57 +0000 | |||
3 | +++ src/imports/ContactEdit/AvatarImport.qml 2014-10-07 20:52:25 +0000 | |||
4 | @@ -18,103 +18,108 @@ | |||
5 | 18 | 18 | ||
6 | 19 | import Ubuntu.Components 1.1 | 19 | import Ubuntu.Components 1.1 |
7 | 20 | import Ubuntu.Components.Popups 1.0 | 20 | import Ubuntu.Components.Popups 1.0 |
9 | 21 | import Ubuntu.Content 0.1 | 21 | import Ubuntu.Content 1.1 |
10 | 22 | 22 | ||
12 | 23 | Item { | 23 | Page { |
13 | 24 | id: root | 24 | id: root |
14 | 25 | 25 | ||
16 | 26 | property var importDialog: null | 26 | property var avatarListener: null |
17 | 27 | property alias activeTransfer: signalConnections.target | ||
18 | 28 | property alias isTransferRunning: activityIndicator.running | ||
19 | 27 | 29 | ||
20 | 28 | signal avatarReceived(string avatarUrl) | 30 | signal avatarReceived(string avatarUrl) |
21 | 29 | 31 | ||
110 | 30 | function requestNewAvatar() | 32 | title: i18n.tr("Import avatar from...") |
111 | 31 | { | 33 | |
112 | 32 | if (!root.importDialog) { | 34 | Item { |
113 | 33 | root.importDialog = PopupUtils.open(contentHubDialog, null) | 35 | id: transferHint |
114 | 34 | } else { | 36 | |
115 | 35 | console.warn("Import dialog already running") | 37 | anchors.fill: parent |
116 | 36 | } | 38 | visible: root.isTransferRunning |
117 | 37 | } | 39 | |
118 | 38 | 40 | ActivityIndicator { | |
119 | 39 | Component { | 41 | id: activityIndicator |
120 | 40 | id: contentHubDialog | 42 | |
121 | 41 | 43 | anchors { | |
122 | 42 | PopupBase { | 44 | horizontalCenter: parent.horizontalCenter |
123 | 43 | id: dialogue | 45 | verticalCenter: parent.verticalCenter |
124 | 44 | 46 | verticalCenterOffset: units.gu(-4) | |
125 | 45 | property alias activeTransfer: signalConnections.target | 47 | } |
126 | 46 | 48 | running: root.activeTransfer | |
127 | 47 | parent: QuickUtils.rootItem(this) | 49 | } |
128 | 48 | focus: true | 50 | |
129 | 49 | 51 | Label { | |
130 | 50 | Rectangle { | 52 | anchors { |
131 | 51 | anchors.fill: parent | 53 | top: activityIndicator.bottom |
132 | 52 | 54 | topMargin: units.gu(2) | |
133 | 53 | ContentTransferHint { | 55 | horizontalCenter: parent.horizontalCenter |
134 | 54 | anchors.fill: parent | 56 | } |
135 | 55 | activeTransfer: dialogue.activeTransfer | 57 | // TRANSLATORS: This message appear while waiting for a external application content, |
136 | 56 | } | 58 | // %1 refers to the the name of source application |
137 | 57 | 59 | text: i18n.tr("Waiting for content from %1...").arg(peerPicker.peer.name) | |
138 | 58 | ContentPeerPicker { | 60 | } |
139 | 59 | id: peerPicker | 61 | |
140 | 60 | 62 | Button { | |
141 | 61 | anchors.fill: parent | 63 | id: cancelButton |
142 | 62 | visible: dialogue.done | 64 | |
143 | 63 | contentType: ContentType.Pictures | 65 | enabled: root.activeTransfer && (root.activeTransfer.state === ContentTransfer.InProgress) |
144 | 64 | handler: ContentHandler.Source | 66 | anchors { |
145 | 65 | 67 | bottom: parent.bottom | |
146 | 66 | onPeerSelected: { | 68 | left: parent.left |
147 | 67 | peer.selectionType = ContentTransfer.Single | 69 | right: parent.right |
148 | 68 | dialogue.activeTransfer = peer.request() | 70 | margins: units.gu(2) |
149 | 69 | } | 71 | } |
150 | 70 | 72 | text: i18n.tr("Cancel") | |
151 | 71 | onCancelPressed: { | 73 | onClicked: root.activeTransfer.state = ContentTransfer.Aborted |
152 | 72 | PopupUtils.close(root.importDialog) | 74 | } |
153 | 73 | } | 75 | } |
154 | 74 | } | 76 | |
155 | 75 | } | 77 | ContentPeerPicker { |
156 | 76 | 78 | id: peerPicker | |
157 | 77 | Connections { | 79 | |
158 | 78 | id: signalConnections | 80 | anchors.fill: parent |
159 | 79 | 81 | contentType: ContentType.Pictures | |
160 | 80 | target: dialogue.activeTransfer | 82 | handler: ContentHandler.Source |
161 | 81 | onStateChanged: { | 83 | showTitle: false |
162 | 82 | var done = ((dialogue.activeTransfer.state === ContentTransfer.Charged) || | 84 | visible: !root.isTransferRunning |
163 | 83 | (dialogue.activeTransfer.state === ContentTransfer.Aborted)) | 85 | |
164 | 84 | 86 | onPeerSelected: { | |
165 | 85 | if (dialogue.activeTransfer.state === ContentTransfer.Charged) { | 87 | peer.selectionType = ContentTransfer.Single |
166 | 86 | dialogue.hide() | 88 | root.activeTransfer = peer.request() |
167 | 87 | if (dialogue.activeTransfer.items.length > 0) { | 89 | } |
168 | 88 | root.avatarReceived(dialogue.activeTransfer.items[0].url) | 90 | |
169 | 89 | } | 91 | onCancelPressed: pageStack.pop() |
170 | 90 | } | 92 | } |
171 | 91 | 93 | ||
172 | 92 | if (done) { | 94 | Connections { |
173 | 93 | acceptTimer.restart() | 95 | id: signalConnections |
174 | 94 | } | 96 | |
175 | 95 | } | 97 | target: null |
176 | 96 | } | 98 | onStateChanged: { |
177 | 97 | 99 | var done = ((root.activeTransfer.state === ContentTransfer.Charged) || | |
178 | 98 | // WORKAROUND: Work around for application becoming insensitive to touch events | 100 | (root.activeTransfer.state === ContentTransfer.Aborted)) |
179 | 99 | // if the dialog is dismissed while the application is inactive. | 101 | transferHint.enabled = !done |
180 | 100 | // Just listening for changes to Qt.application.active doesn't appear | 102 | if (root.activeTransfer.state === ContentTransfer.Charged) { |
181 | 101 | // to be enough to resolve this, so it seems that something else needs | 103 | if (( root.activeTransfer.items.length > 0) && root.avatarListener) { |
182 | 102 | // to be happening first. As such there's a potential for a race | 104 | root.avatarListener.avatarReceived( root.activeTransfer.items[0].url) |
183 | 103 | // condition here, although as yet no problem has been encountered. | 105 | } |
184 | 104 | Timer { | 106 | } |
185 | 105 | id: acceptTimer | 107 | |
186 | 106 | 108 | if (done) { | |
187 | 107 | interval: 100 | 109 | pageStack.pop() |
188 | 108 | repeat: true | 110 | } |
189 | 109 | running: false | 111 | } |
190 | 110 | onTriggered: { | 112 | } |
191 | 111 | if(Qt.application.active) { | 113 | |
192 | 112 | PopupUtils.close(root.importDialog) | 114 | Component.onDestruction: { |
193 | 113 | } | 115 | if (root.avatarListener) { |
194 | 114 | } | 116 | root.avatarListener.busy = false |
195 | 115 | } | 117 | } |
196 | 116 | 118 | } | |
197 | 117 | Component.onDestruction: root.importDialog = null | 119 | |
198 | 120 | Component.onCompleted: { | ||
199 | 121 | if (root.avatarListener) { | ||
200 | 122 | root.avatarListener.busy = true | ||
201 | 118 | } | 123 | } |
202 | 119 | } | 124 | } |
203 | 120 | } | 125 | } |
204 | 121 | 126 | ||
205 | === modified file 'src/imports/ContactEdit/ContactDetailAvatarEditor.qml' | |||
206 | --- src/imports/ContactEdit/ContactDetailAvatarEditor.qml 2014-09-29 13:20:08 +0000 | |||
207 | +++ src/imports/ContactEdit/ContactDetailAvatarEditor.qml 2014-10-07 20:52:25 +0000 | |||
208 | @@ -23,7 +23,7 @@ | |||
209 | 23 | ContactDetailBase { | 23 | ContactDetailBase { |
210 | 24 | id: root | 24 | id: root |
211 | 25 | 25 | ||
213 | 26 | readonly property alias busy: activityIndicator.running | 26 | property alias busy: activityIndicator.running |
214 | 27 | readonly property string defaultAvatar: "image://theme/add" | 27 | readonly property string defaultAvatar: "image://theme/add" |
215 | 28 | 28 | ||
216 | 29 | function isEmpty() { | 29 | function isEmpty() { |
217 | @@ -61,6 +61,16 @@ | |||
218 | 61 | return avatarUrl | 61 | return avatarUrl |
219 | 62 | } | 62 | } |
220 | 63 | 63 | ||
221 | 64 | function avatarReceived(avatarUrl) | ||
222 | 65 | { | ||
223 | 66 | // remove the previous image, this is nessary to make sure that the new image | ||
224 | 67 | // get updated otherwise if the new image has the same name the image will not | ||
225 | 68 | // be updated | ||
226 | 69 | avatarImage.source = "" | ||
227 | 70 | // Update with the new value | ||
228 | 71 | avatarImage.source = application.copyImage(root.contact, avatarUrl); | ||
229 | 72 | } | ||
230 | 73 | |||
231 | 64 | detail: contact ? contact.detail(ContactDetail.Avatar) : null | 74 | detail: contact ? contact.detail(ContactDetail.Avatar) : null |
232 | 65 | implicitHeight: units.gu(8) | 75 | implicitHeight: units.gu(8) |
233 | 66 | implicitWidth: units.gu(8) | 76 | implicitWidth: units.gu(8) |
234 | @@ -81,7 +91,7 @@ | |||
235 | 81 | anchors.centerIn: visible ? avatar : undefined | 91 | anchors.centerIn: visible ? avatar : undefined |
236 | 82 | height: units.gu(3) | 92 | height: units.gu(3) |
237 | 83 | width: units.gu(3) | 93 | width: units.gu(3) |
239 | 84 | visible: source == defaultAvatar | 94 | visible: source == defaultAvatar && !root.busy |
240 | 85 | 95 | ||
241 | 86 | // When updating the avatar using the content picker the temporary file returned | 96 | // When updating the avatar using the content picker the temporary file returned |
242 | 87 | // can contain the same name as the previous one and if the cache is enabled this | 97 | // can contain the same name as the previous one and if the cache is enabled this |
243 | @@ -94,29 +104,16 @@ | |||
244 | 94 | id: activityIndicator | 104 | id: activityIndicator |
245 | 95 | 105 | ||
246 | 96 | anchors.centerIn: avatar | 106 | anchors.centerIn: avatar |
247 | 97 | running: (avatarImport.importDialog != null) | ||
248 | 98 | visible: running | 107 | visible: running |
249 | 99 | } | 108 | } |
250 | 100 | 109 | ||
251 | 101 | AvatarImport { | ||
252 | 102 | id: avatarImport | ||
253 | 103 | |||
254 | 104 | onAvatarReceived: { | ||
255 | 105 | // remove the previous image, this is nessary to make sure that the new image | ||
256 | 106 | // get updated otherwise if the new image has the same name the image will not | ||
257 | 107 | // be updated | ||
258 | 108 | avatarImage.source = "" | ||
259 | 109 | // Update with the new value | ||
260 | 110 | avatarImage.source = application.copyImage(root.contact, avatarUrl); | ||
261 | 111 | } | ||
262 | 112 | } | ||
263 | 113 | |||
264 | 114 | MouseArea { | 110 | MouseArea { |
265 | 115 | anchors.fill: parent | 111 | anchors.fill: parent |
266 | 116 | onClicked: { | 112 | onClicked: { |
267 | 117 | // make sure the OSK disappear | 113 | // make sure the OSK disappear |
268 | 118 | root.forceActiveFocus() | 114 | root.forceActiveFocus() |
270 | 119 | avatarImport.requestNewAvatar() | 115 | root.busy = true |
271 | 116 | pageStack.push(Qt.resolvedUrl("AvatarImport.qml"), {avatarListener: root}) | ||
272 | 120 | } | 117 | } |
273 | 121 | } | 118 | } |
274 | 122 | } | 119 | } |
275 | 123 | 120 | ||
276 | === modified file 'src/imports/ContactList/ContactExporter.qml' | |||
277 | --- src/imports/ContactList/ContactExporter.qml 2014-09-30 21:17:25 +0000 | |||
278 | +++ src/imports/ContactList/ContactExporter.qml 2014-10-07 20:52:25 +0000 | |||
279 | @@ -16,15 +16,17 @@ | |||
280 | 16 | 16 | ||
281 | 17 | import QtQuick 2.2 | 17 | import QtQuick 2.2 |
282 | 18 | import QtContacts 5.0 | 18 | import QtContacts 5.0 |
283 | 19 | import Ubuntu.Content 1.1 | ||
284 | 19 | 20 | ||
285 | 20 | Item { | 21 | Item { |
286 | 21 | id: root | 22 | id: root |
287 | 22 | 23 | ||
288 | 23 | property var contactModel | 24 | property var contactModel |
289 | 24 | property var outputFile | 25 | property var outputFile |
290 | 26 | property var activeTransfer: null | ||
291 | 25 | 27 | ||
292 | 26 | signal contactExported(int error, string url) | ||
293 | 27 | signal contactsFetched(var contacts) | 28 | signal contactsFetched(var contacts) |
294 | 29 | signal done() | ||
295 | 28 | 30 | ||
296 | 29 | function start(contacts) { | 31 | function start(contacts) { |
297 | 30 | if (!contactModel) { | 32 | if (!contactModel) { |
298 | @@ -59,7 +61,20 @@ | |||
299 | 59 | 61 | ||
300 | 60 | onExportCompleted: { | 62 | onExportCompleted: { |
301 | 61 | priv.currentQueryId = -1 | 63 | priv.currentQueryId = -1 |
303 | 62 | root.contactExported(error, root.outputFile) | 64 | |
304 | 65 | // send contacts back to source app (pick mode) | ||
305 | 66 | if (error === ContactModel.ExportNoError) { | ||
306 | 67 | var obj = Qt.createQmlObject("import Ubuntu.Content 1.1; ContentItem { url: '" + url + "' }", root) | ||
307 | 68 | if (root.activeTransfer) { | ||
308 | 69 | root.activeTransfer.items = [obj] | ||
309 | 70 | root.activeTransfer.state = ContentTransfer.Charged | ||
310 | 71 | } else { | ||
311 | 72 | console.error("No active transfer") | ||
312 | 73 | } | ||
313 | 74 | } else { | ||
314 | 75 | console.error("Fail to export contacts:" + error) | ||
315 | 76 | } | ||
316 | 77 | root.done() | ||
317 | 63 | } | 78 | } |
318 | 64 | 79 | ||
319 | 65 | onContactsFetched: { | 80 | onContactsFetched: { |
320 | @@ -74,5 +89,16 @@ | |||
321 | 74 | } | 89 | } |
322 | 75 | } | 90 | } |
323 | 76 | } | 91 | } |
324 | 92 | |||
325 | 93 | Connections { | ||
326 | 94 | target: root.activeTransfer | ||
327 | 95 | |||
328 | 96 | onStateChanged: { | ||
329 | 97 | if (root.activeTransfer.state === ContentTransfer.Aborted) { | ||
330 | 98 | root.activeTransfer = null | ||
331 | 99 | root.done() | ||
332 | 100 | } | ||
333 | 101 | } | ||
334 | 102 | } | ||
335 | 77 | } | 103 | } |
336 | 78 | } | 104 | } |
337 | 79 | 105 | ||
338 | === modified file 'src/imports/ContactList/ContactListPage.qml' | |||
339 | --- src/imports/ContactList/ContactListPage.qml 2014-10-03 21:45:23 +0000 | |||
340 | +++ src/imports/ContactList/ContactListPage.qml 2014-10-07 20:52:25 +0000 | |||
341 | @@ -21,7 +21,7 @@ | |||
342 | 21 | import Ubuntu.Components.ListItems 1.0 as ListItem | 21 | import Ubuntu.Components.ListItems 1.0 as ListItem |
343 | 22 | import Ubuntu.Components.Popups 1.0 as Popups | 22 | import Ubuntu.Components.Popups 1.0 as Popups |
344 | 23 | import Ubuntu.Contacts 0.1 as ContactsUI | 23 | import Ubuntu.Contacts 0.1 as ContactsUI |
346 | 24 | import Ubuntu.Content 0.1 as ContentHub | 24 | import Ubuntu.Content 1.1 as ContentHub |
347 | 25 | 25 | ||
348 | 26 | import "../Common" | 26 | import "../Common" |
349 | 27 | 27 | ||
350 | @@ -622,27 +622,13 @@ | |||
351 | 622 | ContactExporter { | 622 | ContactExporter { |
352 | 623 | id: contactExporter | 623 | id: contactExporter |
353 | 624 | 624 | ||
354 | 625 | property var activeTransfer: null | ||
355 | 626 | |||
356 | 627 | contactModel: contactList.listModel | 625 | contactModel: contactList.listModel |
357 | 628 | outputFile: mainPage.pickMode ? "file:///tmp/address_book_app_export.vcf" : "" | 626 | outputFile: mainPage.pickMode ? "file:///tmp/address_book_app_export.vcf" : "" |
372 | 629 | onContactExported: { | 627 | |
373 | 630 | // send contacts back to source app (pick mode) | 628 | onDone: { |
360 | 631 | if (error === ContactModel.ExportNoError) { | ||
361 | 632 | var obj = Qt.createQmlObject("import Ubuntu.Content 0.1; ContentItem { url: '" + url + "' }", contactExporter) | ||
362 | 633 | if (activeTransfer) { | ||
363 | 634 | activeTransfer.items = [obj] | ||
364 | 635 | activeTransfer.state = ContentHub.ContentTransfer.Charged | ||
365 | 636 | } else { | ||
366 | 637 | console.error("No active transfer") | ||
367 | 638 | } | ||
368 | 639 | } else { | ||
369 | 640 | console.error("Fail to export contacts:" + error) | ||
370 | 641 | } | ||
371 | 642 | activeTransfer = null | ||
374 | 643 | mainPage.pickMode = false | 629 | mainPage.pickMode = false |
375 | 644 | mainPage.state = "default" | 630 | mainPage.state = "default" |
377 | 645 | application.returnVcard(url) | 631 | application.returnVcard(contactExporter.outputFile) |
378 | 646 | } | 632 | } |
379 | 647 | 633 | ||
380 | 648 | onContactsFetched: { | 634 | onContactsFetched: { |
I haven't done a full review yet, but I would suggest perhaps it's worth talking about adding the title to ContentTransferHint rather than reimplementing it here. I worry about needing to maintain a copy of that code, and it seems like a nice feature for ContentTransfer Hint.