966839 - [Camera] Add frame grid to camera settings

Status: RESOLVED FIXED

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

Comment 1

[Youngjun Kim]

Ashish! Please check the user stories and update the current status.

Comment 2

[singhashish1887]

It can be implemented

Comment 3

[singhashish1887]

It can be implemented by drawing lines on View Finder

Comment 4

[singhashish1887]

Attachment: Pointer to Pull Request.html

Hi David,

We have uploaded frame grid work in progress code.This code does not include test code.
Pleased review the code.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8380121 [details]
Pointer to Pull Request.html

Adding Diego and Wilson for feedback

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8380121 [details]
Pointer to Pull Request.html

Clearing the review request, since this patch was not actually yet ready for review and landing.  I've left feedback on github.

My overall feeling is that that the code to display and position the grid is more complicated than needed.  I think you'll be able to simplify it quite a bit with only a little work.

I'd also like wilson's opinion on whether the grid should be created and destroyed when shown and hidden or if, for consistency with other views, it should just be created at startup and shown and hidden when used.  And the bigger question: should the grid actually be part of the viewfinder view, or part of the hud view?  (Or even a new view class of its own?)

Comment 7

[Wilson Page [:wilsonpage]]

Attachment: CSS_Pixel_Frame_Grid.png

Comment 8

[Wilson Page [:wilsonpage]]

Attachment: patch (camera-dev)

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8380460 [details]
patch (camera-dev)

Comments on github.

Overall, this seems much cleaner than the previous patch.

Note that there is still an issue with the file permissions. Someone running a Unix variant needs to run chmod 644 on them.

I'm completely confused by the updatePreview() method, but mostly by the parts that predate this patch, so I'd be happy with just having a followup bug files to look into that stuff.

Comment 10

[Wilson Page [:wilsonpage]]

Attachment: pull-request (camera-new-features)

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8385288 [details] [review]
pull-request (camera-new-features)

r- because:

1) the patch modifies a test file, but does not appear to actually test the grid

2) this PR combines an API refactoring to (methods added to js/vendor/view.js and removed from subclasses) with the feature work. Please put those in two separate pull requests.

3) The PR changes file modes from 644 to 755. As I've said before, I won't land patches that make files executable.

I have not yet tested the patch, and it has not gone through UX review yet, so more work may be required before landing, but I wanted to give this feedback right away.

Comment 12

[Hema Koka [:hema]]

3/4 update: LG to address david's comments and move this along.

Comment 13

[singhashish1887]

Wilson, please create PR for camera branch so David can review

Comment 14

[Diego Marcos [:dmarcos]]

PR is attached

https://github.com/mozilla-b2g/gaia/pull/16825

Comment 15

[Diego Marcos [:dmarcos]]

Attachment: pull-request (camera-new-features) review comments addressed

Comment 16

[Diego Marcos [:dmarcos]]

I created a new Bug 980094 for the refactor of view.js that blocks this bug.

Comment 17

[Diego Marcos [:dmarcos]]

https://github.com/mozilla-b2g/gaia/pull/16924

The new pull request addresses the reasons of the r-:

1. It changes all the file permissions to 644
2. It moves the view.js refactor to a different patch (Bug 980094)
3. There's a unit test for the grid.

Comment 18

[Diego Marcos [:dmarcos]]

I made a couple of tweaks to the grid that are different from the visual specs:

1. I dimmed down the grid so it is less distracting. I also added a border to the lines to increase contrast when the background is bright.
2. The grid is now hidden when opening the settings menu. With the settings open, the lines of the grid and the menu options were overlapping creating a confusing pattern (see attachment)

Comment 19

[Diego Marcos [:dmarcos]]

Attachment: settingsMenuLinesOverlapping.png

Image illustrating the overlap of the grid with the settings lines

Comment 20

[Diego Marcos [:dmarcos]]

Attachment: settingsMenuNoGrid.png

This how the settings menu looks like with the grid hidden

Comment 21

[Diego Marcos [:dmarcos]]

Rob, I flaged the bug for you to review. Please ping me if you need help to flash your phone. 

You have to fetch the branch from my repo with the following commands (same thing we did together the other day but on a different branch):

git fetch dmarcos
git checkout dmarcos/bug966839
make install-gaia APP=camera

Comment 22

[Rob MacDonald [:robmac]]

Adding Hung to this (In reply to Diego Marcos from comment #21)
> Rob, I flaged the bug for you to review. Please ping me if you need help to
> flash your phone. 

Adding Hung as well. Amy is on PTO until Wednesday and Hung has kindly agreed to step in for visual reviews while she is away.

Comment 23

[Rob MacDonald [:robmac]]

Comment on attachment 8386463 [details] [review]
pull-request (camera-new-features) review comments addressed

Focusing solely on the frame grid... I really like how the frame grid is hidden while the menu is open but minusing for two reasons. 

1 - The menu should automatically close when an option, including HDR on or off, is selected. (Should this be covered under a different bug?) 
2 - The grid lines do not match the visual design spec and, as a result, I'm flagging Hung from visual on this patch. 

If the menu closing is a separate bug and Hung is ok with the visuals, then I'm otherwise fine with this.

Thanks!

Thanks!

Comment 24

[Hung Nguyen]

Attachment: CSS_Pixels_Frame_Grid_V02.png

Updated visual spec for the camera grid.

Comment 25

[Hung Nguyen]

Comment on attachment 8386463 [details] [review]
pull-request (camera-new-features) review comments addressed

After reviewing the current visual specification with Peter, we've decided to make a minor update. 

In place of the opaque white lines, we thought best to use a bit of a transparency to reduce the harshness along with a faint black stroke for very bright situations. Its basically a polished version of what Diego had implemented. 

Please see attached updated specification for full details.

Comment 26

[Diego Marcos [:dmarcos]]

Flagging again for UX review after implemented requested changes in the grid. Please focus now only on the grid. You will have a chance to review the behavior of the settings menu in a different bug.

Flashing your phones:

git fetch dmarcos
git checkout dmarcos/bug966839
make install-gaia APP=camera

Comment 27

[Hung Nguyen]

Its looks great - thanks for Diego. I think we're good to go.

Comment 28

[David Flanagan [:djf]]

Comment on attachment 8386463 [details] [review]
pull-request (camera-new-features) review comments addressed

r- because I can barely understand the CSS and because it does not appear to match the visual spec.  The updated spec asks for 2px wide, 70% opaque white lines, and the CSS gives 1px wide 30% opaque white.  Also, this patch draws a translucent black line around the perimeter of the screen as part of the grid.

Comment 29

[David Flanagan [:djf]]

Comment on attachment 8386463 [details] [review]
pull-request (camera-new-features) review comments addressed

r+ with nits addressed, and if you check the merge on view/viewfinder.js, particularly the code involved in setting the preview size.

Comment 30

[Diego Marcos [:dmarcos]]

Landed on camera-new-features:

https://github.com/mozilla-b2g/gaia/commit/5d0758cf2bc4e25f6adc9392ab23b63dcca79a0e

Comment 31

[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