971090 - [Camera] 1.4 Visual Design - Thumbnail/Preview Gallery

Status: VERIFIED FIXED

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

Description

[Amy Lee [:amylee] UX]

When user selects preview thumbnail, user is able to swipe through images. 

Will attach visual specs

Comment 1

[Amy Lee [:amylee] UX]

Attachment: Image_Preview_Spec.jpg

Visual spec for Image Preview

Comment 2

[Amy Lee [:amylee] UX]

Attachment: Camera_Back_Icon.svg

Camera back icon

Comment 3

[Justin D'Arcangelo [:justindarc]]

Attachment: JS Bin demo of working carousel with animation

Here is a VERY simple (~100 lines) implementation on JS Bin of a full-screen photo carousel. It uses touch events exclusively, so you must run on device to see it working. Just swipe around in the demo to switch between the images. The previous Filmstrip implementation also has the ability to swipe between images, but there is no animation going between the images.

Also, the included CSS in the JS Bin will center and fit the images to the screen while preserving their aspect ratio.

Comment 4

[Hema Koka [:hema]]

Please update with eta on pull request to camera branch

Comment 5

[hyuna.cho]

Please upload svg images for share/delete.

Comment 6

[hyuna.cho]

Attachment: WIP-preview gallery PR

Comment 7

[Amy Lee [:amylee] UX]

Attachment: General_Toolbar_Delete.svg

Comment 8

[Amy Lee [:amylee] UX]

Attachment: General_toolbar_share.svg

Hi, 

The icons have been uploaded.

Comment 9

[hyuna.cho]

Comment on attachment 8386014 [details]
WIP-preview gallery PR

Plz review the preview gallery

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8386014 [details]
WIP-preview gallery PR

Since the patch description says this is a work in progress, I assume it is not ready for formal review. I'm clearing the review flag.

Overall the code looks pretty good, and I'm setting feedback+. I have not tried testing it. I've left a few comments on github.  

The patch includes a long file named style/icomoon.json which should not be there.

The patch includes a gallery.png image file, but no code that uses that image, which makes me think that the image is not needed.

Next steps: once the new controls-2 view has landed, this patch can probably land. Coordinate with Diego and as soon as controls2 lands, try to make a pull request against camera-new-features.

Also: attach screenshots illustrating the gallery/share/delete buttons and the image count text for ux review.

Comment 11

[hyuna.cho]

I'm modifying some code regarding with comments.
But there is some questions, so I added comments in github. Please check it.

I will create a pull request for preview in camera-new-features after landing Diego patch.

I will remove a gallery.png. But how about icomoon.json?
If anyone want to add new icons, I know they need this icomoon.json to generate.

Comment 12

[hyuna.cho]

Attachment: preview.png

Comment 13

[hyuna.cho]

Attachment: preview_landscape.png

Comment 14

[Wilson Page [:wilsonpage]]

Comment on attachment 8386014 [details]
WIP-preview gallery PR

r-

- Camera app crashes on attempting to pinch zoom image
- Image post-process work should be managed centrally in CameraController, emitting 'newmedia' when done.
- Old filmstrip has not been removed
- Patches not being linted before review.
- Smaller 'nits' in Github comments.

Overall: I'm super happy with the code. It's well structured and integrates nicely into the existing architecture. You clearly know what you're doing :)

Comment 15

[Amy Lee [:amylee] UX]

Flagging Rob for UX review

Comment 16

[Amy Lee [:amylee] UX]

Attachment: Image_Preview_VSD_Review.png

Hi Hyuna, 

Here is my feedback. I've also attached a visual spec for reference. Let me know if you have any questions. Thanks!

------------------------------------------------

Camera Back Icon

Position: Should be same as camera
front/back toggle switch

Circle should be 50x50px (looks too small)

Icon should be 3x3 rems based on SVG bounding box (looks too big).

Circle Opacity: Should be 12% (looks too opaque)

Highlight State: Please remove fade in transition. Circle should turn blue right away on press (same as flash, toggle, and settings button

Menu Fade in/Fade Out

Please have toolbar, text, and camera back button fade out after 3 seconds or when the user taps the screen. These should appear again when the user taps the screen. 

Image Swipe Transition

When user swipes to next image, the image should slide to the next image based on the direction of the swipe. Right now it jumps to the next image.

Swiping gesture doesn’t feel responsive.  Several attempts at swiping was required to view next image.


Confirmation Text

Font: Should be Fira Sans Medium (currently Regular)

Positon: Distance between image number text and toolbar should be a minimum of 38px (should be in the same position for all confirmation text).

Comment 17

[Amy Lee [:amylee] UX]

(In reply to hyuna.cho from comment #13)
> Created attachment 8389686 [details]
> preview_landscape.png

Provided feedback in comment 16

Comment 18

[Wilson Page [:wilsonpage]]

Comment on attachment 8386014 [details]
WIP-preview gallery PR

r-

- Improper use of events to call API and some events like `previewGalleryOpen` being called when they shouldn't be.
- Broadcast module should be completely removed and replaced with `app` events if need be.
- Unspecced PreviewGallery opening transition artifact of old filmstrip. SHould be changed to quick fade-in-out.
- Some questionable unit-tests assertions around `calledWith`
- Some unused CSS and confusion around flexbox.
- CSS state overrides should sit directly below original declarations to make sure all selectors for each element are in one place.

OVERALL: We're almost there! Really awesome changes in response to last review!! :)

Comment 19

[hyuna.cho]

Attachment: preview-update.png

Please check the below:
# Camera back icon
I changed the camera back icon with the latest SVG icon.
Circle: 50x50px
Circle opacity: 12%
Icon: 3x3rem

But it's too small.

# Confirmation text
It was 38px far from the toolbar

Comment 20

[Amy Lee [:amylee] UX]

Hi Hyuna, 

Can you give me the branch you are working from so I can flash my device to see the changes.

Thanks

Comment 21

[Amy Lee [:amylee] UX]

Hi Hyuna, 
 
Can you give me the branch you are working from so I can flash my device to see the changes.

Thanks

Comment 22

[David Flanagan [:djf]]

(In reply to Amy from comment #16)

> 
> Image Swipe Transition
> 
> When user swipes to next image, the image should slide to the next image
> based on the direction of the swipe. Right now it jumps to the next image.
> 
> Swiping gesture doesn’t feel responsive.  Several attempts at swiping was
> required to view next image.
> 

Getting this right will be a substantial amount of work, and I'd like to handle that in a followup bug.  I'm responsible for the current swiping behavior, and in the case where we had the filmstrip displayed I guess it was good enough.  Doing a nice smooth transition like gallery does will be a lot of work and will have performance implications because we'll have to have the preview image loaded in advance.

I agree that the current behavior is unacceptable, but let's land what we have now so we can get rid of the filmstrip and move ahead with the new visuals.

Comment 23

[Amy Lee [:amylee] UX]

Hi David, 

Thanks for responding. It would be great if we got the preview image transitions to the same level as the gallery at some point so a follow-up bug would be great for that.

Also, I wanted to ask about the overall scale of the graphics in the camera build. All the screen shots that I've seen, I've noticed that the graphics are roughly 10% smaller than the spec (i.e the flash, camera back, settings menu buttons, shutter controls). For example, it says 50px x 50px for the camera back icon in image preview but when I actually measure the screenshot, the button is closer to 42px x 42px. Do you know what is causing this? 

Thanks   

(In reply to David Flanagan [:djf] from comment #22)
> (In reply to Amy from comment #16)
> 
> > 
> > Image Swipe Transition
> > 
> > When user swipes to next image, the image should slide to the next image
> > based on the direction of the swipe. Right now it jumps to the next image.
> > 
> > Swiping gesture doesn’t feel responsive.  Several attempts at swiping was
> > required to view next image.
> > 
> 
> Getting this right will be a substantial amount of work, and I'd like to
> handle that in a followup bug.  I'm responsible for the current swiping
> behavior, and in the case where we had the filmstrip displayed I guess it
> was good enough.  Doing a nice smooth transition like gallery does will be a
> lot of work and will have performance implications because we'll have to
> have the preview image loaded in advance.
> 
> I agree that the current behavior is unacceptable, but let's land what we
> have now so we can get rid of the filmstrip and move ahead with the new
> visuals.

Comment 24

[David Flanagan [:djf]]

Amy,

I don't have any idea what could be causing the smaller size. But I've never worked with an icon font before. Let's ask Wilson.  Maybe it is just a matter of adjusting the font size.

Comment 25

[Amy Lee [:amylee] UX]

Just to clarify, I am referring to the css created assets such as the circles for the buttons/shutter button and also the background bar height of the preview image toolbar. In the CSS it says 50px x 50px but it's actually smaller when measured. It looks like all the assets on the screen are universally smaller. Thanks

(In reply to David Flanagan [:djf] from comment #24)
> Amy,
> 
> I don't have any idea what could be causing the smaller size. But I've never
> worked with an icon font before. Let's ask Wilson.  Maybe it is just a
> matter of adjusting the font size.

Comment 26

[hyuna.cho]

(In reply to Amy from comment #21)
> Hi Hyuna, 
>  
> Can you give me the branch you are working from so I can flash my device to
> see the changes.
> 
> Thanks

Hi Amy,

https://github.com/hyunacho/gaia/tree/bug971090-preview
It's the working branch of the preview based on camera-new-feature branch.
You can find your requirements except 'Menu Fade in/Fade Out' and 'Image Swipe Transition'

Comment 27

[hyuna.cho]

Attachment: PR-preview gallery

* It's the patch based on camera-new-features

* Camera back icon : The camera back icon with 3*3 rem is too small. So it's 4*4 rem

Comment 28

[Amy Lee [:amylee] UX]

Attachment: Edits.png

Hi Hyuna, 

Can you make the following changes: 

1. Camera back icon: Make this 4.5x4.5 rems

2. Speed up the fade in/out of menu items to 200ms (See attached)

3. Change the opacity of the preview toolbar background to 40% (See attached)

4. Can you move preview image so it is aligned to the top of the screen in portrait view? (I want to avoid having the camera back icon to get cut off by the black cropping)

6. Speed up the transition fade out when you hit the camera back icon and fades to viewfinder.

5. When the menu disappears and the user swipes to next image. The menu should remain invisible until the user taps the screen to bring back the menu.

6. Image count text: Can you change this to Fira Sans Medium?

Can you please also flag Rob MacDonald for future UI reviews? It's looking really good so far. Let me know if you have any questions. Thanks!

Comment 29

[Diego Marcos [:dmarcos]]

Can you please file individual bugs for each of the UX issues you find? Your suggestions are going to be lost in the comment of this bug.

Comment 30

[Justin D'Arcangelo [:justindarc]]

Follow-up bug for adding animations/transitions for the swipe gesture in `media_frame.js`:

https://bugzilla.mozilla.org/show_bug.cgi?id=984566

Comment 31

[Diego Marcos [:dmarcos]]

This is top priority right now. How far are we from landing this? Hyuna, Do you need any help?

Comment 32

[hyuna.cho]

(In reply to Diego Marcos [:dmarcos] from comment #31)
> This is top priority right now. How far are we from landing this? Hyuna, Do
> you need any help?

The remaining issues are amy's requirements.
4. Can you move preview image so it is aligned to the top of the screen in portrait view? (I want to avoid having the camera back icon to get cut off by the black cropping)
-> It's related with media_frame.js to fit the image. I start to check about this. Do you have any idea to fix this?

5. When the menu disappears and the user swipes to next image. The menu should remain invisible until the user taps the screen to bring back the menu.

Comment 33

[Justin D'Arcangelo [:justindarc]]

Hyuna,

Are there any remaining issues from code review that need addressed as well?

Comment 34

[Justin D'Arcangelo [:justindarc]]

Amy,

As for your one comment:

(In reply to Amy from comment #28)
> 4. Can you move preview image so it is aligned to the top of the screen in
> portrait view? (I want to avoid having the camera back icon to get cut off
> by the black cropping)

The component responsible for displaying the preview is also shared by the Gallery app and it centers the preview image. If we modify `media_frame.js` to top-align, this will also top-align the full-screen images in the Gallery app.

So, do we have any room to adjust/remove this from the UI spec? If not, is it acceptable to also top-align in the Gallery app? If not, I think our only option here would be to create a separate copy of `media_frame.js` for the Camera app that uses top-align, but then we lose our code reusability.

Comment 35

[hyuna.cho]

Comment on attachment 8392094 [details]
PR-preview gallery

Except the below item:
4. Can you move preview image so it is aligned to the top of the screen in portrait view? (I want to avoid having the camera back icon to get cut off by the black cropping)

Plz refer Justin comment.
https://bugzilla.mozilla.org/show_bug.cgi?id=971090#c34

Comment 36

[Amy Lee [:amylee] UX]

(In reply to hyuna.cho from comment #35)
> Comment on attachment 8392094 [details]
> PR-preview gallery
> 
> Except the below item:
> 4. Can you move preview image so it is aligned to the top of the screen in
> portrait view? (I want to avoid having the camera back icon to get cut off
> by the black cropping)
> 
> Plz refer Justin comment.
> https://bugzilla.mozilla.org/show_bug.cgi?id=971090#c34

Hi Hyuna, 

I don't want this to over complicate the code so let's keep the image center aligned. Can you please change the colour of the black area above and below the image to a dark grey (#333333). This will allow the camera back icon circle to still show up against the dark background. Thanks!

Comment 37

[Tiffanie Shakespeare [:tif]]

Attachment: delete photo confirmation not landscape

Comment 38

[Tiffanie Shakespeare [:tif]]

Attachment: share action menu not in landscape

Comment 39

[Tiffanie Shakespeare [:tif]]

Hey guys - I reviewed Hyunacho's' patch while Rob is on vacation. 

While in preview, I noticed a couple of issues:
- the full screen dialogues appear in portrait when the phone is oriented to landscape (attached photos)
- swiping up from the bottom exits preview and returns to the camera

Thanks!

Comment 40

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8392094 [details]
PR-preview gallery

See my comments in #39 - sorry guys I did it wrong! o_O

Comment 41

[Diego Marcos [:dmarcos]]

In my phon(In reply to Tiffanie Shakespeare from comment #38)
> Created attachment 8392997 [details]
> share action menu not in landscape

In my phone the delete/share dialogs rotate properly (see attachments). Anyways the rotation of those dialogs is a separate issue that should be addressed in a different bug.

Comment 42

[Diego Marcos [:dmarcos]]

Attachment: deleteLanscape.jpg

Comment 43

[Diego Marcos [:dmarcos]]

Attachment: shareLandscape.jpg

Comment 44

[David Flanagan [:djf]]

Diego: How in the world are you getting those dialogs in landscape? What hardware?  Since the app is in portrait, I'm shocked that the system activity menu is not in portrait!

Tiffanie: I'm pretty sure that the dialogs are wrong on the existing camera as well, so this is not a regression from the current camera.  Obviously it is wrong, but since it is deeply tied to the system app, it really isn't something we can address as part of this bug.

Comment 45

[Diego Marcos [:dmarcos]]

I had just flashed hyuna's branch. I don't know if she's changing the behaviour of the dialogs based on the feedback

Comment 46

[hyuna.cho]

(In reply to Diego Marcos [:dmarcos] from comment #45)
> I had just flashed hyuna's branch. I don't know if she's changing the
> behaviour of the dialogs based on the feedback

I didn't change about the dialog rotation.
In my nexus 4, delete/share dialog didn't rotate. 
But I know it can not handle in camera app. If need to fix, how can I handle in camera.

Comment 47

[Diego Marcos [:dmarcos]]

Hyuna. You don't need to handle this in this bug. It's a system app issue. 

I cannot explain why those dialogs rotate in my phone with your branch :)

Comment 48

[Tiffanie Shakespeare [:tif]]

Sorry guys I don't have a regular FFOS device to check it out the current camera behaviour. It seems weird that those dialogues would rotate in other apps (gallery, browser) but not in camera. Am I crazy?

Comment 49

[Justin D'Arcangelo [:justindarc]]

Tiffanie: The Camera app orientation is locked to keep certain UI elements (like capture button) "fixed" to their locations on the screen. As a result, the orientation events don't bubble up to the system-level dialogs.

Comment 50

[Diego Marcos [:dmarcos]]

The rotation of the dialog doesn't belong to the scope of this patch. We can create a separate bug to deal with it. Are there any other UX concerns left to get an r+ for this feature?

Comment 51

[Tiffanie Shakespeare [:tif]]

I haven't checked it out lately but did this get fixed? I didn't see any discussion on it in the comments.

- swiping up from the bottom exits preview and returns to the camera

Comment 52

[hyuna.cho]

(In reply to Tiffanie Shakespeare from comment #51)
> I haven't checked it out lately but did this get fixed? I didn't see any
> discussion on it in the comments.
> 
> - swiping up from the bottom exits preview and returns to the camera

I updated the patch about this. Please check it again.
- swiping up from the bottom exits preview and returns to the camera

Comment 53

[David Flanagan [:djf]]

Comment on attachment 8392094 [details]
PR-preview gallery

I didn't have time to do a complete review, but am giving r- because:

1) In secure mode, when the camera is closed, we have to forget all of the images that are in the gallery preview.  It is not enough to just close the preview. This is an important privacy feature.

2) I'm very nervous about the changes to shared/js/media/video_player.js since that is also used by Gallery. It would be much better if you didn't have to change that file.

3) I'd like you to check how this patch interacts with pick activities. You don't need to create thumbnails in that case, do you?

4) There is unrelated code in style/icons.css

Comment 54

[hyuna.cho]

(In reply to David Flanagan [:djf] from comment #53)
> Comment on attachment 8392094 [details]
> PR-preview gallery
> 
> I didn't have time to do a complete review, but am giving r- because:
> 
> 1) In secure mode, when the camera is closed, we have to forget all of the
> images that are in the gallery preview.  It is not enough to just close the
> preview. This is an important privacy feature.

I updated the comment in github. I had frame.clear also. Please check.

> 3) I'd like you to check how this patch interacts with pick activities. You
> don't need to create thumbnails in that case, do you?

I updated the comment in github. Please check.

Comment 55

[Tiffanie Shakespeare [:tif]]

Hyunacho: I updated the patch but am still seeing the issue (swiping up exits preview). I even tried doing a gaia reset and applied the patch again but still see the behaviour. 

Can you please check?

Comment 56

[hyuna.cho]

Attachment: 971090_link to path on github.html

Comment 57

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8393875 [details]
971090_link to path on github.html

Seems good hyunacho - the swipe issue is fixed!

I'll file a bug for the dialogues.

Thanks!

Comment 58

[David Flanagan [:djf]]

Comment on attachment 8393875 [details]
971090_link to path on github.html

r- because of the potential for unbounded memory growth from saving every thumbnail ever generated.  Also, there is related code that is going to need to be rebased on top of the patch for bug 949941.

Other bugs I've noticed while testing:

1) In the preview gallery, if I tap delete or share and then cancel, when I go back to the preview the buttons are gone, which seems surprising to me.  I suspect that you need to add a stopPropagation() to the handlers for those buttons so that the user's click doesn't also get interpreted as a command to hide the controls

2) If I tap on the preview to bring up the buttons and the item number, I would expect those to stay permanently instead of disappearing on their own.  Does the spec say that they should disappear even when I've tapped to make them visible again?

3) The patch includes these lines to allow the screen to sleep while the preview gallery is shown:

+  this.on('previewgallery:opened', lockscreen.enableTimeout);
+  this.on('previewgallery:closed', lockscreen.disableTimeout);

When I test on the nexus 4, however they don't work. While the preview is shown the screen never times out. It is possible that this is a bug in the system app and not in your patch, but please investigate.If you can verify that the release lock code in lock-screen.js is being called at the appropriate time, then we just need to file a follow up bug because something in the system is broken.  But it seems more likely that something is wrong in the camera app and we ought to fix it.

4) It is possible to get trapped in the preview with no way back to camera. STR:
   - take a picture
   - record a video
   - go to the preview gallery
   - start playing the video
   - while the video is playing, swipe to see the photo
   - tap to bring up the controls
   - notice that the controls do not appear. There is no way to get back
     to the camera!

If you go back to the video, play it again, then pause, you'll get  controls, but not otherwise. Swiping away a playing video needs to be treated as a video pause.

5) When I record a video in any orientation other than regular landscape, the thumbnail that appears in the circle is rotated the wrong way. I think you need to use getVideoRotation() to determine the correct orientation of the video when generating the thumbnail. See the old filmstrip code or see how it is done in Gallery.

Comment 59

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8393875 [details]
971090_link to path on github.html

The "Play" icon needs to be removed from the thumbnail for videos as reported in Bug 986102.

Comment 60

[hyuna.cho]

(In reply to David Flanagan [:djf] from comment #58)
> Comment on attachment 8393875 [details]
> 971090_link to path on github.html
> 
> r- because of the potential for unbounded memory growth from saving every
> thumbnail ever generated.  Also, there is related code that is going to need
> to be rebased on top of the patch for bug 949941.

bug 949941 didn't land into camera-new-features. 
So I created new PR that merged bug 949941 patch using cherry-pick in my patch.

> Other bugs I've noticed while testing:
> 
> 1) In the preview gallery, if I tap delete or share and then cancel, when I
> go back to the preview the buttons are gone, which seems surprising to me. 
> I suspect that you need to add a stopPropagation() to the handlers for those
> buttons so that the user's click doesn't also get interpreted as a command
> to hide the controls

I fixed it.

> 2) If I tap on the preview to bring up the buttons and the item number, I
> would expect those to stay permanently instead of disappearing on their own.
> Does the spec say that they should disappear even when I've tapped to make
> them visible again?

It's Amy's requirements and completed to review from Amy. 
Refer to Amy comments in Comment 16

> 3) The patch includes these lines to allow the screen to sleep while the
> preview gallery is shown:
> 
> +  this.on('previewgallery:opened', lockscreen.enableTimeout);
> +  this.on('previewgallery:closed', lockscreen.disableTimeout);
> 
> When I test on the nexus 4, however they don't work. While the preview is
> shown the screen never times out. It is possible that this is a bug in the
> system app and not in your patch, but please investigate.If you can verify
> that the release lock code in lock-screen.js is being called at the
> appropriate time, then we just need to file a follow up bug because
> something in the system is broken.  But it seems more likely that something
> is wrong in the camera app and we ought to fix it.

I checked the 'requestWakeLock('screen')' in lock-screen.js. 
The preview gallery locked/unlocked the screen lock but the screen doesn't go to sleep.
I can't find the reason in our code. I think it's better to file a bug and follow up.

> 4) It is possible to get trapped in the preview with no way back to camera.
> STR:
>    - take a picture
>    - record a video
>    - go to the preview gallery
>    - start playing the video
>    - while the video is playing, swipe to see the photo
>    - tap to bring up the controls
>    - notice that the controls do not appear. There is no way to get back
>      to the camera!
> 
> If you go back to the video, play it again, then pause, you'll get 
> controls, but not otherwise. Swiping away a playing video needs to be
> treated as a video pause.

I fixed it.

> 5) When I record a video in any orientation other than regular landscape,
> the thumbnail that appears in the circle is rotated the wrong way. I think
> you need to use getVideoRotation() to determine the correct orientation of
> the video when generating the thumbnail. See the old filmstrip code or see
> how it is done in Gallery.

When video recording started, the rotation is '0' in portrait mode.
But the rotate value changed to '90' after getting video meta data.
Already use getVideoRotation() in lib/get-video-meta-data.js and
I found the rotate value is changed to wrong value in MP4Parser() of shared/js/media/get_video_rotation.js
It need to check by the proper engineer.

Comment 61

[hyuna.cho]

Attachment: PR_971090+949941

Comment 62

[Amy Lee [:amylee] UX]

Hi Hyuna, 

Just a small thing I noticed. Is the fade in/fade out menu transition speed the same for when you tap to make the menu disappear and when it disappears automatically? The transition looks faster when it disappears on it's own. If they are different, can you match the speed to the slower one? 

Thanks

Comment 63

[David Flanagan [:djf]]

(In reply to hyuna.cho from comment #60)
> (In reply to David Flanagan [:djf] from comment #58)
> > Comment on attachment 8393875 [details]
> > 971090_link to path on github.html
> > 
> > r- because of the potential for unbounded memory growth from saving every
> > thumbnail ever generated.  Also, there is related code that is going to need
> > to be rebased on top of the patch for bug 949941.
> 
> bug 949941 didn't land into camera-new-features. 
> So I created new PR that merged bug 949941 patch using cherry-pick in my
> patch.

Did you modify the code to save only a single thumbnail?

> > 2) If I tap on the preview to bring up the buttons and the item number, I
> > would expect those to stay permanently instead of disappearing on their own.
> > Does the spec say that they should disappear even when I've tapped to make
> > them visible again?
> 
> It's Amy's requirements and completed to review from Amy. 
> Refer to Amy comments in Comment 16

Comment 16 does not actually say that. Setting needinfo for Amy to check.  Amy: In preview mode, if the user taps to make the controls visible, do you want them to stay visible until the next tap, or do you want them to fade out automatically.  I expect the former but currently we have the latter implemented and it feels wrong to me.

 
> > 3) The patch includes these lines to allow the screen to sleep while the
> > preview gallery is shown:
> > 
> > +  this.on('previewgallery:opened', lockscreen.enableTimeout);
> > +  this.on('previewgallery:closed', lockscreen.disableTimeout);
> > 
> > When I test on the nexus 4, however they don't work. While the preview is
> > shown the screen never times out. It is possible that this is a bug in the
> > system app and not in your patch, but please investigate.If you can verify
> > that the release lock code in lock-screen.js is being called at the
> > appropriate time, then we just need to file a follow up bug because
> > something in the system is broken.  But it seems more likely that something
> > is wrong in the camera app and we ought to fix it.
> 
> I checked the 'requestWakeLock('screen')' in lock-screen.js. 
> The preview gallery locked/unlocked the screen lock but the screen doesn't
> go to sleep.
> I can't find the reason in our code. I think it's better to file a bug and
> follow up.

Okay, please file a followup bug and note the number here.

> 
> > 5) When I record a video in any orientation other than regular landscape,
> > the thumbnail that appears in the circle is rotated the wrong way. I think
> > you need to use getVideoRotation() to determine the correct orientation of
> > the video when generating the thumbnail. See the old filmstrip code or see
> > how it is done in Gallery.
> 
> When video recording started, the rotation is '0' in portrait mode.
> But the rotate value changed to '90' after getting video meta data.
> Already use getVideoRotation() in lib/get-video-meta-data.js and
> I found the rotate value is changed to wrong value in MP4Parser() of
> shared/js/media/get_video_rotation.js
> It need to check by the proper engineer.

If I'm understanding you correctly, you have not fixed this bug, and as far as I know you have not filed a followup bug for it, so I'm not sure why you're requesting review again. Video thumbnails work correctly in the old filmstrip code, don't they? If so, then it doesn't seem like it should be hard to make the work right in this patch.  I'm the one who wrote get_video_rotation and MP4Parser, so if you have questions about those I might be able to help. Note that when you hold the phone in portrait orientation we say that the phone orientation is 0 in that case. But on most devices, the sensor is mounted at a 90 degree angle to the phone, so the video will have a 90 degree rotation recorded in its metadata. The phone orientation and the video rotation are two different values, and it is normal for them to be 90 degrees off from each other.

I'm willing to let this be fixed in a followup, but I'd prefer to just get it done in this bug, especially if the video thumbnails worked correctly in the old filmstrip code that you are replacing.

Comment 64

[David Flanagan [:djf]]

Comment on attachment 8394616 [details]
PR_971090+949941

I haven't looked at the latest code, but setting r- pending clarification from Amy about when controls should time out and because it sounds like the video thumbnail orientation issue is not fixed yet.

Comment 65

[David Flanagan [:djf]]

Here's a guess about the video rotation bug.  Somewhere I bet you're using the camera orientation as the rotation.  Then I bet there is code that does if (!rotation) { rotation = getVideoRotation(); }

When you pass in 0 (portait mode orientation for the phone), the !rotation branch runs and you correctly parse the rotation from the video.  But for all other orientations, you're passing in a number !== 0 and you just end up using that number and your video rotation is off by 90 degrees.

When creating a thumbnail for a photo, you pass in rotation and mirrored from the EXIF data.  When creating a thumbanil for a video, maybe you need to leave those undefined.

Comment 66

[Amy Lee [:amylee] UX]

(In reply to David Flanagan [:djf] from comment #63)
> (In reply to hyuna.cho from comment #60)
> > (In reply to David Flanagan [:djf] from comment #58)
> > > Comment on attachment 8393875 [details]
> > > 971090_link to path on github.html
> > > 
> > > r- because of the potential for unbounded memory growth from saving every
> > > thumbnail ever generated.  Also, there is related code that is going to need
> > > to be rebased on top of the patch for bug 949941.
> > 
> > bug 949941 didn't land into camera-new-features. 

Hi David, 

When the user taps to make the controls visible, it should fade out again if left idle. This is a common convention used in other devices (i.e android, kindle app). Also, we want to stay consistent with the behaviour pattern. When you open image preview, the menu fades out when left idle, so when the user taps to bring the menu back, it also follows the same pattern of fading out when left idle.


> > So I created new PR that merged bug 949941 patch using cherry-pick in my
> > patch.
> 
> Did you modify the code to save only a single thumbnail?
> 
> > > 2) If I tap on the preview to bring up the buttons and the item number, I
> > > would expect those to stay permanently instead of disappearing on their own.
> > > Does the spec say that they should disappear even when I've tapped to make
> > > them visible again?
> > 
> > It's Amy's requirements and completed to review from Amy. 
> > Refer to Amy comments in Comment 16
> 
> Comment 16 does not actually say that. Setting needinfo for Amy to check. 
> Amy: In preview mode, if the user taps to make the controls visible, do you
> want them to stay visible until the next tap, or do you want them to fade
> out automatically.  I expect the former but currently we have the latter
> implemented and it feels wrong to me.
> 
>  
> > > 3) The patch includes these lines to allow the screen to sleep while the
> > > preview gallery is shown:
> > > 
> > > +  this.on('previewgallery:opened', lockscreen.enableTimeout);
> > > +  this.on('previewgallery:closed', lockscreen.disableTimeout);
> > > 
> > > When I test on the nexus 4, however they don't work. While the preview is
> > > shown the screen never times out. It is possible that this is a bug in the
> > > system app and not in your patch, but please investigate.If you can verify
> > > that the release lock code in lock-screen.js is being called at the
> > > appropriate time, then we just need to file a follow up bug because
> > > something in the system is broken.  But it seems more likely that something
> > > is wrong in the camera app and we ought to fix it.
> > 
> > I checked the 'requestWakeLock('screen')' in lock-screen.js. 
> > The preview gallery locked/unlocked the screen lock but the screen doesn't
> > go to sleep.
> > I can't find the reason in our code. I think it's better to file a bug and
> > follow up.
> 
> Okay, please file a followup bug and note the number here.
> 
> > 
> > > 5) When I record a video in any orientation other than regular landscape,
> > > the thumbnail that appears in the circle is rotated the wrong way. I think
> > > you need to use getVideoRotation() to determine the correct orientation of
> > > the video when generating the thumbnail. See the old filmstrip code or see
> > > how it is done in Gallery.
> > 
> > When video recording started, the rotation is '0' in portrait mode.
> > But the rotate value changed to '90' after getting video meta data.
> > Already use getVideoRotation() in lib/get-video-meta-data.js and
> > I found the rotate value is changed to wrong value in MP4Parser() of
> > shared/js/media/get_video_rotation.js
> > It need to check by the proper engineer.
> 
> If I'm understanding you correctly, you have not fixed this bug, and as far
> as I know you have not filed a followup bug for it, so I'm not sure why
> you're requesting review again. Video thumbnails work correctly in the old
> filmstrip code, don't they? If so, then it doesn't seem like it should be
> hard to make the work right in this patch.  I'm the one who wrote
> get_video_rotation and MP4Parser, so if you have questions about those I
> might be able to help. Note that when you hold the phone in portrait
> orientation we say that the phone orientation is 0 in that case. But on most
> devices, the sensor is mounted at a 90 degree angle to the phone, so the
> video will have a 90 degree rotation recorded in its metadata. The phone
> orientation and the video rotation are two different values, and it is
> normal for them to be 90 degrees off from each other.
> 
> I'm willing to let this be fixed in a followup, but I'd prefer to just get
> it done in this bug, especially if the video thumbnails worked correctly in
> the old filmstrip code that you are replacing.

Comment 67

[Amy Lee [:amylee] UX]

Hi Hyuna, 

Can you please add a fade out/in transition for when the image preview button disappears and appears when you turn video recording on/off? Right now it just disappears abruptly with no transition. 

Thanks!

Comment 68

[Amy Lee [:amylee] UX]

See above. Thanks

Comment 69

[Wilson Page [:wilsonpage]]

Feedback:

- `PreviewGalleryController` is doing some very low level file-system work. IMO I think code like this should only be seen inside `Storage` class.
- `PreviewGalleryController` is touching `camera` and `controls`. I think this work should be moved to there corresponding controllers.
- Testing on the Nexus4 I noticed that that you can see images loading in the media frame (top to bottom, like images loading on a website). Looks a little unpolished. I suggest we should fade images in only once we know they are fully loaded.

Comment 70

[hyuna.cho]

(In reply to Amy from comment #62)
> Hi Hyuna, 
> 
> Just a small thing I noticed. Is the fade in/fade out menu transition speed
> the same for when you tap to make the menu disappear and when it disappears
> automatically? The transition looks faster when it disappears on it's own.
> If they are different, can you match the speed to the slower one? 
> 
> Thanks

The transition speed is same when the menu fade in/out. (Comment 28)

Comment 71

[hyuna.cho]

The screen timeout issue
Bug 987071 - [Camera] The screen doesn't going to sleep in the preview gallery

Comment 72

[David Flanagan [:djf]]

(In reply to Amy from comment #67)
> Hi Hyuna, 
> 
> Can you please add a fade out/in transition for when the image preview
> button disappears and appears when you turn video recording on/off? Right
> now it just disappears abruptly with no transition. 
> 
> Thanks!

Amy: would you file a followup bug for this?  I'd like to focus on just getting this main feature landed, and we're really close to the deadline, so I'd rather Hyuna was not distracted with polish issues like this.

Comment 73

[David Flanagan [:djf]]

(In reply to Wilson Page [:wilsonpage] from comment #69)
> Feedback:
> 
> - `PreviewGalleryController` is doing some very low level file-system work.
> IMO I think code like this should only be seen inside `Storage` class.

Hyuna has either already fixed that, or I don't understand what you're talking about.  Looks like all storage stuff is done via the storage object.

> - `PreviewGalleryController` is touching `camera` and `controls`. I think
> this work should be moved to there corresponding controllers.

Do you mean viewfinder and controls?  I see that the constructor initializes properties to hold references to both of those views, but they seem to be unused.  Hyuna should delete those unused properties.

> - Testing on the Nexus4 I noticed that that you can see images loading in
> the media frame (top to bottom, like images loading on a website). Looks a
> little unpolished. I suggest we should fade images in only once we know they
> are fully loaded.

This is because the Nexus 4 firmware cannot give us big enough thumbnails, so we're decoding the full-size image.  There is a build-time config option (at least there was in 1.3) to change the definition of "big enough" to avoid this problem on hardware like nexus 4.  In any case, this is out of scope for Hyuna's patch.  

Wilson: do you think that any of your issues should prevent this patch from landing?  At this late stage, I'd prefer to use followup bugs for anything that is not obvious breakage.

Comment 74

[Amy Lee [:amylee] UX]

(In reply to David Flanagan [:djf] from comment #72)
> (In reply to Amy from comment #67)
> > Hi Hyuna, 
> > 
> > Can you please add a fade out/in transition for when the image preview
> > button disappears and appears when you turn video recording on/off? Right
> > now it just disappears abruptly with no transition. 
> > 
> > Thanks!
> 
> Amy: would you file a followup bug for this?  I'd like to focus on just
> getting this main feature landed, and we're really close to the deadline, so
> I'd rather Hyuna was not distracted with polish issues like this.

Hi David, I've filed a new bug for this. See bug 987200. Thanks

Comment 75

[David Flanagan [:djf]]

Attachment: pull request with the thumbnail memory leak fixed

Justin,

This pull request has 4 commits that I'll squash before landing.

The first is Hyuna's latest commit.

The second is the merge commit where I resolved some merge conflicts

The fourth is a one line bug fix in storage.js that isn't really related to this patch, but I found it so I fixed it here.  

The third commit is the main one I'd like you to review.  It addresses the issue that Hyuna's patch was storing thumbnails forever which was a slow memory leak.  This changes the architecture so thumbnails are now created by the preview gallery controller, not by the camera controller.  A newthumbnail event is sent when the user takes a picture or video or when the user deletes the itme that currently has its thumbnail displayed and then closes the preview. Thumbnails re not generated until the preview closes.

Also, as part of this commit I removed the play arrow from video thumbnails. This is another patch that Hyuna had mostly done, but I'd sent it back for more chnages, so I went ahead and integrated it here.  (There is a bug filed for that, so if we land this, we should close that bug, too.)

I have not verified yet that this does not break any tests.  I'd appreciate it if you would check that for me as part of your review.  If you think this is close, please tell me what you want changed and give a conditional r+, so I can address the points you raise and get it ready for Diego to land in the morning.

Comment 76

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8396051 [details] [review]
pull request with the thumbnail memory leak fixed

Looks good! Tested on Nexus 4 with no visible problems. However, the unit tests were very broken. I created a branch on my fork and added an additional commit to fix the unit tests here:

https://github.com/justindarc/gaia/tree/bug971090-davidflanagan

The specific commit that fixes the unit tests can be found here:

https://github.com/justindarc/gaia/commit/9ae9b275beca6f17d2f31bf638b9e384d8260376

There were a few additional "nits" that I put in the comments on the pull request. Its nothing that would hold us back landing this on camera-new-features though. If you can, apply my unit test fixes to your branch and go ahead and land on camera-new-features. You can just get the diff patch by appending ".diff" to the end of the URL above and apply it to your branch:

https://github.com/justindarc/gaia/commit/9ae9b275beca6f17d2f31bf638b9e384d8260376.diff

Comment 77

[hyuna.cho]

I checked and tested with new patch.
I want to update the patch some code,
 1) change event for pick activities in confirm controller because the event name changed in camera controller
    - Should update. Now can't display the taken image/video in the pick activity
 2) remove unused codes
 3) some codes move from previewItem() to openPreview() to handle the secure mode in opening time.

https://github.com/hyunacho/gaia/commit/06bd371196c81125aa9974684a93ac9cc8d69016

Comment 78

[David Flanagan [:djf]]

Justin: thanks for reviewing and fixing the test.

Hyuna: thank you for finding and fixing the issue with the pick activity and the event name.

Later tonight I will integrate both your latest commits and get this landed to camera-new-features.  Hopefully that will mean that Diego will be able to land to master in the morning.

Comment 79

[David Flanagan [:djf]]

Landed on camera-new-features: https://github.com/mozilla-b2g/gaia/commit/a98694087619de6a9b2613fd4724d9a963091049

Comment 80

[Wilson Page [:wilsonpage]]

djf: Seems fair, glad this has landed :)

Comment 81

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530

Comment 82

[sarsenyev]

The bug is no longer reproduces,visual design - Thumbnail/Preview Gallery is changed in 1.4 and master builds

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140410000201
Gaia: 9b2da43dfee3792cd311ae55f0b06272313208f0
Gecko: 9d9ead7d6afa
Version: 30.0a2
Firmware Version: v1.2-device.cfg

1.5 Environmental Variables:
Device: Buri 1.5 MOZ
BuildID: 20140410040201
Gaia: 9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko: 690c810c8e3e
Version: 31.0a1
Firmware Version: v1.2-device.cfg