Closed Bug 974119 Opened 7 years ago Closed 7 years ago

[Camera][Madai] 1.4 Viewfinder size and alignment

Categories

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

ARM
Gonk (Firefox OS)
enhancement
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S3 (14mar)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: dmarcos, Assigned: wilsonpage)

References

()

Details

Attachments

(2 files, 2 obsolete files)

The size and alignment of the viewfinder changes depending on the selected aspect ratio.
Blocks: 971444
QA Contact: wilsonpage
For Madai camera app, there is an option of 1:1 aspect ration for Preview size. But there is no image size setting that support 1:1 aspect ratio.
So, to show the preview in 1:1 aspect ratio as per the UX spec, the preview in viewfinder must be scaled to fit the 1:1 ratio.
Assignee: nobody → wilsonpage
jyothiprasad: The new implementation I'm working on will expose **all** pictureSizes that Gecko exposes. We Then aim to match the chosen pictureSize aspect ratio with a previewSize. If your device exposes a 1:1 pictureSize, it will be selectable after this patch has landed.
Attached file pull-request (camera-new-features) (obsolete) —
Attachment #8385237 - Flags: review?(dflanagan)
Comment on attachment 8385237 [details] [review]
pull-request (camera-new-features)

r- because:

- it appears to disable an entire file full of tests

- it does not include any tests of the preview size selection code or of the viewport sizing and positioning code

- the pickPreviewSize() algorithm is inadequate because it does not consider the size of the preview stream, only its aspect ratio

- the updatePreview() algorithm (which is the whole point of this bug) is too opaque to understand.

Also, given that there is still not any code that uses capabilities.pictureSizes, I don't believe that the patch will actually allow the user to select a picture size and see the preview displayed in the ways illustrated in the attached image.

Given the big overlap between this PR and the one for 974337, I suggest that you combine the two into a single patch on 974337 and leave this bug open
Attachment #8385237 - Flags: review?(dflanagan) → review-
Blocks: 966764
Depends on: 979234
Attachment #8385237 - Attachment is obsolete: true
Attachment #8386410 - Flags: review?(dflanagan)
Attachment #8386410 - Attachment is obsolete: true
Attachment #8386410 - Flags: review?(dflanagan)
Attachment #8386481 - Flags: review?(dflanagan)
Target Milestone: --- → 1.4 S3 (14mar)
Comment on attachment 8386481 [details] [review]
UNIFIED PATCH: pull-request (camera-new-features)

Landed in 980599. Clearing review request and marking resolved.
Attachment #8386481 - Flags: review?(dflanagan)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I don't see explicit ux+ -- Flagging ux for reviews (amy/tif: if there are minor spec deviations, please file bugs so we can fix them -- reopen this bug if there are major issues) -- this feature has landed on camera-new-features branch only
Flags: needinfo?(tshakespeare)
Flags: needinfo?(amlee)
Pulling the patch listed in the attachments - from the interaction side it seems fine. Viewfinder changes based on camera resolution chosen and looking at the photos in gallery, they seem to match up.

I'll let Amy comment on the visual side of things.
Flags: needinfo?(tshakespeare)
The patch attached to this bug never landed, but instead a unified patch was landed for:

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

The UX review should probably be for 980599 instead.
(In reply to Hema Koka [:hema] from comment #8)
> I don't see explicit ux+ -- Flagging ux for reviews (amy/tif: if there are
> minor spec deviations, please file bugs so we can fix them -- reopen this
> bug if there are major issues) -- this feature has landed on
> camera-new-features branch only
Flags: needinfo?(amlee)
blocking-b2g: --- → 1.4+
Flags: in-moztrap?(ychung)
New test case needs to be added. There is no existing test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Test case added to moztrap:

https://moztrap.mozilla.org/manage/case/14395/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.