Merge lp://qastaging/~renatofilho/address-book-app/fix-1377334 into lp://qastaging/~phablet-team/address-book-app/staging

Proposed by Renato Araujo Oliveira Filho
Status: Rejected
Rejected by: Renato Araujo Oliveira Filho
Proposed branch: lp://qastaging/~renatofilho/address-book-app/fix-1377334
Merge into: lp://qastaging/~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://qastaging/~renatofilho/address-book-app/fix-1377334
Reviewer Review Type Date Requested Status
Ken VanDine Approve
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+237480@code.qastaging.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.

To post a comment you must log in.
Revision history for this message
Ken VanDine (ken-vandine) wrote :

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 ContentTransferHint.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
Renato Araujo Oliveira Filho (renatofilho) :
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
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.

review: Approve

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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches