870726 - [Camera] Confirmation of successful Image pick for email attachments and wallpaper

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect, P2)

Description

[Leo]

1. Title : Confirmation of successful Image pick for email attachments and wallpaper
2. Precondition : email app should be configured and camera app should be working.
3. Tester's Action : (1) Settings->Display->Wallpaper->camera->Take picture
                     (2) Email App->compose->attach->camera->Take picture

4. Detailed Symptom (ENG.) : There is no confirmation to user whether image is captured successfully or not. In the current scenario if the pictue is captured and if it is successful, camera app will exit and attachment will be saved in email composer. But after Image is captured the control will be still in camera app for few seconds, this is very confusing to user.
5. Expected : Confirmation should be given to user for successful Image pick for email attachments and wallpaper
6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 3c48f7af4ecc8d32f62a44f0a30d4f18895bc33e
9.Personal email id: [e-mail]

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

I've observed this on other platforms as well. Is there anything we can do about it?

Comment 2

[Daniel Coloma:dcoloma]

Don't think we should hold the release because of this one, I recommend we mark this as tef-.

Comment 3

[Dale Harvey (:daleharvey)]

Yup, for some reason I thought this was implemented, definitely doable and should really be done, it is new feature work though

Comment 4

[Leo]

After taking picture, it still in preview mode for about 5 sec, and if I keep touching the camera button, it sounds taking picture but it shows only first image

Comment 5

[Alex Keybl [:akeybl]]

This is an enhancement, so it needs to become a new product requirement for us to block.

Comment 6

[Leo]

Attachment: Video Attachment

Hi Dale,

I have added message window at the bottom of the screen when user clicks camera capture button for picking photos for wallpaper and email picture attachments.

It has two scenarios:
1. Just after user presses capture button: It shows message "Image Saving" (message can be changed) and avoid user to cancel pick activity while taking the picture.
As per our analysis, if user clicks pick cancel button just after pressing capture button, there are some unhandling ipc messages because of which camera is getting crashed in Gecko Layer.
2. Pick activity success (takePictureSuccess): It shows message "Image Saved" (message can be changed) - confirmation for successfull Image pick.

Comment 7

[Leo]

Please let me know if this scenario Is Okay.

Comment 8

[Dale Harvey (:daleharvey)]

Sorry I misunderstood the intent of this bug

I dont believe we should have an 'image saved' confirmation, I dont really understand what 'image saved' means when I am picking my wallpaper, the confirmation is it returning to the settings apps / homescreen we the image we have just taken displayed, I am seeing that happen in between 1 and 2 seconds on my inari.

In the video it was certainly slower (almost 5 seconds), but I think instead of adding more UI we should speed that up as fast as possible. Booting up my leo to test now, will needinfo UX on this too.

I originally misread this bug to mean that we should have a 'confirm pick' implemented in the camera so people could check the photo and be happy with it before it went and changed their wallpaper

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 749688 [details]
Video Attachment

Sorry I meant to write the above as a comment here

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

Being able to hit the 'X' button after taking a picture may be the cause of the crash in bug 867025.

Comment 11

[Jim Porter (:squib)]

(In reply to Dale Harvey (:daleharvey) from comment #8)
> In the video it was certainly slower (almost 5 seconds), but I think instead
> of adding more UI we should speed that up as fast as possible. Booting up my
> leo to test now, will needinfo UX on this too.

Maybe we should have a "Working... [spinner]" popup while it's doing its thing? That seems sufficiently generic. Unless we get the process consistently under 1 second, it might confuse people if they don't get any feedback while saving.

I don't think we really need to show an "image saved"/"success" popup. You can infer success from the fact that the camera closed and it did whatever it is it was supposed to.

Comment 12

[Stephany Wilkes]

In regard to comment 8, is the UX need here a yes/no on this proposal? "In the video it was certainly slower (almost 5 seconds), but I think instead of adding more UI we should speed that up as fast as possible. Booting up my leo to test now, will needinfo UX on this too."

Please let us know if any additional UX is needed this. Initial call is: Yes, you're right, it should be as fast as possible.

Comment 13

[David Flanagan [:djf]]

Stephany,

I think the question most broadly is: what is the proper UX flow when the user picks a photo from the camera (for wallpaper or as an image attachment).

Currently the camera just takes a picture and returns to the invoking app. The user does not get a chance to review and approve the photo they just took.  It seems to me that that kind of review with the option of retaking might be what we want eventually, but it might also be a new feature that needs to be postponed for v1.2.

More narrowly, this specific bug has to do with the fact that it takes the camera a couple of seconds to save a new photo after taking it and during that time the user isn't given any feedback about what is happening. The attached video demonstrates a solution proposed by our partners. Your thoughts on the the "saving photo..." "photo saved" banners shown in the video would be helpful.

If we're not going to modify the app to allow the user to review and approve or discard the photo they just took, then of course we'll try to speed it up as much as possible so that we return to the invoking app quickly. I'm not sure how fast we can do this.  How fast is fast enough, and if we can't make it fast enough, what sort of feedback to do give the user?

Comment 14

[David Flanagan [:djf]]

I'm taking this for myself to get it off Dale's list, but will probably try to pass it off to Jim.

Comment 15

[Stephany Wilkes]

Thanks, David. Very helpful. Assigning to a member of our team so that we can tackle this on the pattern level.

Comment 16

[Stephany Wilkes]

Thanks, David. Very helpful. Assigning to a member of our team so that we can tackle this on the pattern level.

Comment 17

[David Flanagan [:djf]]

Stephany, I assume you didn't actually mean to assign this bug back to Dale and that that was just a mid-air collision thing, so I'm taking it again.

Comment 18

[Stephany Wilkes]

Definitely mid-air; I was only raising needinfo for Shane. Thanks!

Comment 19

[Jim Porter (:squib)]

Taking this over per comment 14.

Comment 20

[Stephany Wilkes]

What is the deadline/intended release for this? UX is focusing its efforts on leo+ and tef+ blockers, so knowing this will help us prioritize for this bug. Thanks!

Comment 21

[Jim Porter (:squib)]

(In reply to Stephany Wilkes from comment #20)
> What is the deadline/intended release for this?

This is QE2, which has a deadline of May 31st.

Comment 22

[David Flanagan [:djf]]

Let's follow-up on bug 867025 in this bug. Just a minor UX thing: when we disable the X button (in the pick activity) we should gray it out.  Or, reuse it to display the "busy" spinner suggested here.

Comment 23

[Hema Koka [:hema]]

Stephany/Shane, 

Do you know when UX can pick this up to provide input on the Ux flow for this case. Our planned due date for QE2 bugs is May 31st. Let us know.

Thanks
Hema

Comment 24

[Stephany Wilkes]

Assigning to Casey, though please note that leo+, tef+ and tracking+ bugs are still our top priority.

Comment 25

[Leo]

As discussed in San Diego work week marking this as leo+.

Comment 26

[Hema Koka [:hema]]

Casey,

This bug is marked leo+ and we are shooting to get this fixed by end of this week (May 31st). Please let us know your input, so Jim can land this fix in time.

thanks!
hema

Comment 27

[Casey Yee]

Hi Folks,
We should be following the flow as outlined in the Pick-View-Save pattern Pg. 8:
https://mozilla.box.com/s/ie6x8zgm8902dveyb5xf

1. Select Camera app as image source.
2. Take photo
3. Select confirm or re-take photo.
4. Select will confirm selection and change background image.

Given this flow, I'm not sure where the delay would fit into this flow but we can perhaps add some kind of indeterminate progress somewhere in between the steps if the user is left waiting for more than a few seconds.

Comment 28

[Casey Yee]

(In reply to David Flanagan [:djf] from comment #22)
> Let's follow-up on bug 867025 in this bug. Just a minor UX thing: when we
> disable the X button (in the pick activity) we should gray it out.  Or,
> reuse it to display the "busy" spinner suggested here.

David, the spinner makes sense here to me since you can't really do anything until the operation is complete.

Comment 29

[Casey Yee]

What has been proposed here is the ideal scenario where the user is given a chance to review the image before having it placed onto the homescreen.   Without this review step, I think that it will be quite annoying that as a user, I would have to go through the whole interaction again to re-take a picture.

If we can't get this in-place for LEO, I think that a banner (as already suggested earlier in the thread) would be the best compromise.   The wording may need to be different though.  How about: "Saved to Homescreen"?

Comment 30

[Jim Porter (:squib)]

(In reply to Casey Yee [:cyee] from comment #27)
> Hi Folks,
> We should be following the flow as outlined in the Pick-View-Save pattern
> Pg. 8:
> https://mozilla.box.com/s/ie6x8zgm8902dveyb5xf

What's the recommended way of getting at these? Just make an account? (It sounds like they're rolling out accounts to everyone, and I don't want to foul anything up there.)

Comment 31

[Jim Porter (:squib)]

WIP up at https://github.com/mozsquib/gaia/compare/camera-pick-ux

Dale: do you have any idea why the changes above wouldn't show the shared progress spinner? I just get the ugly default progress bar. I'm doing things the same way the email app does them (I think), so I'm not sure what the issue is.

Comment 32

[Jim Porter (:squib)]

Oh for crying out loud... it turns out the issue with the progress bar styling was because I put a newline in the <link rel="stylesheet"> tag in order to wrap to 80 characters.

Comment 33

[Dale Harvey (:daleharvey)]

hah sorry Jim I have been meaning to get to this but I kept getting caught in other bugs.

I still believe we shouldnt be adding progress UI here, the video was likely shot on a device with a debug build enabled, on a leo device testing right now I am seeing the camera return no longer than 3 seconds, usually in just around 2 at which point we have successful confirmation by the fact the wallpaper changes, I dont think this is enough time to confuse users into thinking nothing is happening. (and I believe we could likely make this faster)

Comment 34

[Jim Porter (:squib)]

Hm, I was just going with what comment 28 said. I agree that it's not really that confusing, since most of the time is spent during the actual picture-taking process. We could also go with comment 22 and gray out/hide the X button during the process, since you can't use that button once you've started taking a picture.

I still need to do the final confirmation dialog, but I don't have access to the UX specs...

Comment 35

[Wayne Chang [:wchang]]

UX proposal from comment 27 provided offline to :squib.

Thanks Casey.

Comment 36

[Jim Porter (:squib)]

When we show the confirmation dialog, if the user selects "Retake" to take a new picture, should we save the old one or just discard it? If we save the old one, then we can fold the saving process into the "waiting..." time, so that confirming the picture seems faster, but it also seems like that would be a waste of space.

We could also delete the picture after saving in this case, but that's a bit more complicated.

Comment 37

[Stephany Wilkes]

Jim: Do you still need UX on this? Let us know, or if Casey's comments have it covered. Thanks!

Comment 38

[Jim Porter (:squib)]

(In reply to Stephany Wilkes from comment #37)
> Jim: Do you still need UX on this? Let us know, or if Casey's comments have
> it covered. Thanks!

I should be pretty good. I could use some input on comments 33, 34, and 36, though.

Comment 39

[Jim Porter (:squib)]

I'm having some strange issues with saving pictures after clicking "retake". Sometimes it works, and sometimes it fails with a not-very-descriptive "error" Event. Does anyone have any ideas? I'll work on trying to diagnose this on my end too.

Comment 40

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #39)
> I'm having some strange issues with saving pictures after clicking "retake". ...

Nevermind, I'm just making silly mistakes. (I forgot to remove an event listener.)

Comment 41

[Jim Porter (:squib)]

Attachment: https://github.com/mozsquib/gaia/compare/camera-pick-ux

Ok, this should be working. I left the loading throbber in for now, since that's what Casey suggested. I'm fine with changing it, though (e.g. by simply graying out the X button instead).

Comment 42

[Dale Harvey (:daleharvey)]

Doing the review for this now, but could you point the attachment to a pull request (or make it a patch), Cheers

Comment 43

[Dale Harvey (:daleharvey)]

Right now I cant comment on individual lines of code, the only one I think need changed is we dont use comma first anywhere else that I remember.

We should definitely take the spinner out, its just animated stuff that doesnt server a purpose and distracts from the actual progress UI (focus success etc)

Apart from that I think this is good to go, its a huge improvement cheers, if you can do those and switch the attachment to a pr I can r+ and merge, this is is actually sitting in front of some of my work that needs to get finished today so nice to get it in quickly

Comment 44

[Jim Porter (:squib)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/10246

Oops, I thought I had made a PR already... here's one now.

Comment 45

[Dale Harvey (:daleharvey)]

Comment on attachment 759257 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10246

There are some nits on github that I think should be followed up before merging, but otherwise I am good, this is a really nice improvement.

Also before merging the commits should be squashed and have "Bug 870726 - [Camera] Confirmation of successful Image pick for email attachments and wallpaper" as the commit message

Cheers

Comment 46

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/pull/10246

Comment 47

[Jim Porter (:squib)]

For the UX folks: after discussing this with Dale, we agreed to set the X button for cancelling to look disabled while we're saving the picture. I just set the opacity to 0.5 to do this. It looks like other places use a separate disabled icon, but we can address that in a followup if need be.

Comment 48

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 3434257da95043dbfa930e1747325b4e34c02162
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 50

[Julien Wajsberg [:julienw]]

Please uplift along with the followup Bug 882107.

Comment 51

[Jim Porter (:squib)]

It looks like the issue is a conflict due to the addition of multi-resolution support on trunk. It should be easy to eliminate that dependency, but I'm not sure if we should actually be backporting the multi-resolution stuff to v1-train. Thoughts?

Comment 52

[Julien Wajsberg [:julienw]]

afaik we don't want it on v1-train, but these days I'm not so sure (with the *hd branches).

Comment 53

[Julien Wajsberg [:julienw]]

jim, do you feel like resolving the conflicts and uplifting this along wiht Bug 882107 ?

Comment 54

[Jim Porter (:squib)]

Sure, I can do that. Is there a particular process I should follow for that? I assume I check out v1-train and then cherry-pick the appropriate patches, but what do I do for merging it upstream? (Note that when I check out v1-train, I have to do `git checkout upstream/v1-train` for some reason... maybe I need to fix something there.)

Comment 55

[Jim Porter (:squib)]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/d0f8d8ea52f835bbb02997643d85d9f6cd5d8e5e

I think I did this right. If not, let me know!

Comment 56

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: d0f8d8ea52f835bbb02997643d85d9f6cd5d8e5e

Comment 57

[Parul Mathur]

Created test case in MozTrap https://moztrap.mozilla.org/manage/case/8790/

Executed test case and verified fixed on:
Device: Unagi phone
Build Identifier: 20130621070212
Update channel: unagi/1.1.0/beta
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a34f6d62cb05
Gaia: cca61224e6df8e9f7e450d77cb6a8cf91029be641371823447
Git commit info: 2013-06-21 14:04:07
OS version: 1.1.0.0-prerelease

On the Leo phone, this fix cannot be verified because of camera freeze and phone reboot caused by bug 885133.

Device: Leo phone
Build Identifier: 20130621070212
Update channel: leo/1.1/nightly
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a34f6d62cb05
Gaia: cca61224e6df8e9f7e450d77cb6a8cf91029be641371823447
Git commit info: 2013-06-21 14:04:07
OS version: 1.1.0.0-prerelease

Comment 58

[Leo]

Hello Dale,

In video pick activity from MMS, once the video is recorded successfully, we will get a popup for "retake" or "select". But the popup is black without any preview of video file
Is this intentional? or we missing something here?

Comment 59

[Leo]

registered a bug 896907.