Closed Bug 976852 Opened 7 years ago Closed 7 years ago

[Camera] Indicator Icons On camera Preview for Important camera settings

Categories

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

defect
Not set
normal

Tracking

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

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: singhashish1887, Assigned: singhashish1887)

References

Details

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140224030203

Steps to reproduce:

1) Go to Camera Application -> Click on Setting 
2) change HDR or Timer etc settings .



Actual results:

There is no indicator icon for selected settings.



Expected results:

There should be Some indicator Icons for important settings.
Summary: [Camera] Indicator Icons for set Camera Parameters → [Camera] Indicator Icons On camera Preview for Important camera settings
Attached file Pull Request (camera-dev) (obsolete) —
Attachment #8382586 - Flags: review?(wilsonpage)
Landed on dmarcos/camera-dev
Comment on attachment 8382586 [details] [review]
Pull Request (camera-dev)

Simplified this patch and landed it on dmarcos/camera-dev
Attachment #8382586 - Flags: review?(wilsonpage)
Wilson can you please provide the pull request for your patch
Wilson, please create PR for camera branch so David can review
Flags: needinfo?(wilsonpage)
Blocks: 966764
Attached file pull-request (camera-new-features) (obsolete) —
Attachment #8385840 - Flags: review?(dflanagan)
Flags: needinfo?(wilsonpage)
Comment on attachment 8385840 [details] [review]
pull-request (camera-new-features)

The patch includes a couple of files that do not belong (including one 1700 line log file).

There are a few code changes in the patch that are unrelated to the indicators bug and should not be in the same PR because it will make backing things out impossible.

There is a bug in View.prototype.toggle.

I'd like you to include settings in config/app.js to that indicators can be turned on and off individually instead of as a group.  You'll need this for the geolocation indicator, so might as well add it now.

We'll also need to see screenshots showing the indicators in all four orientations for ux-review
Attachment #8385840 - Flags: review?(dflanagan) → review-
Flags: needinfo?(singhashish1887)
Diego has updated the patch .Diego can you please help us to re upload  the patches .
Flags: needinfo?(singhashish1887) → needinfo?(dmarcos)
Attached file Pull Request (camera-dev) (obsolete) —
It is updated code of Indicator and low battery .
Attachment #8388045 - Flags: review?(wilsonpage)
Attachment #8388045 - Flags: review?(dmarcos)
Comment on attachment 8388045 [details] [review]
Pull Request (camera-dev)

Taking the review from Diego, at his request.

I've left lots of comments on this commit: https://github.com/singhashish1887/gaia/commit/a28adade6ff7ce7d371f0eba4ba5d90c1d484f8c  Wilson has also left helpful comments on that commit.

Unfortunately, that commit appears to be only part of your patch, and I'm not sure which of the other 11 commits in the PR are related to indicators so I have not been able to do a full review.  In particular, I don't know anything about the notification code.

As you'll see in my comments, there are a number of minor changes I'd like to you to make to improve the code quality and readability.

But two particular causes for the r- are:

1) you can't have app management permission for the geolocation indicator. (And I think that is the wrong way to handle it, anyway).

2) For the low battery status indicators, you've hardcoded the levels 15, 10, and 6 into the app in various places. Different devices will have different battery life, though, and these values need to be configurable at build time, not hard-coded into the app.

There are a lot of things going on in this patch. Consider breaking it up into four pieces each with its own bug:

1) The general indicator framework, which seems to be in good shape
2) The notifications code.
3) The geolocation indicator
4) The low battery indicator and notifications.

At this point, it may be simplest to create your patches against the camera-new-features branch rather than against camera-dev, and request review directly from me.

4)
Attachment #8388045 - Flags: review?(dmarcos) → review-
Flags: needinfo?(singhashish1887)
Thanks for you comments David.
We will break it in four patches and upload in camera-new-features.

Is there a way to get Geo location.Because we are using mozPermissionSettings API to read the Permission. but there no listener available to listen change events.That is why we have implemented such a way.
Flags: needinfo?(singhashish1887)
David is there a way to get Geo location.Because we are using mozPermissionSettings API to read the Permission. but there no listener available to listen change events.That is why we have implemented such a way.
Flags: needinfo?(dflanagan)
Attached file Pull Request(Camera-new-feature) (obsolete) —
Previous Commit Link:
https://github.com/singhashish1887/gaia/commit/a28adade6ff7ce7d371f0eba4ba5d90c1d484f8c
Attachment #8389111 - Flags: review?(wilsonpage)
Attachment #8389111 - Flags: review?(dflanagan)
Attachment #8388045 - Flags: review?(wilsonpage)
(In reply to singhashish1887 from comment #12)
> David is there a way to get Geo location.Because we are using
> mozPermissionSettings API to read the Permission. but there no listener
> available to listen change events.That is why we have implemented such a way.

see lib/geo-location.js

The camera driver does not do the geolocation for us. We do the geolocation in gaia and pass the position to the camera driver.  So we know when we're geotagging the photos and when we are not.
Flags: needinfo?(dflanagan)
Comment on attachment 8389111 [details] [review]
Pull Request(Camera-new-feature)

This pull request has 76 commits in it!  Please rebase and create a PR with just one commit.
Attachment #8389111 - Flags: review?(dflanagan) → review-
Attachment #8389111 - Flags: review- → review?(wilsonpage)
Attachment #8389111 - Attachment is obsolete: true
Attachment #8389111 - Flags: review?(wilsonpage)
Attached file Pull Request (camera-dev) UT Test (obsolete) —
I have done the changes in Geo-Location and created the new pull request .I came to know that geo-Location will not be land in mozilla mater it is MADAI specific.
If you find rest of the things fine in patch It can be merge with geolocation disabled.
Attachment #8390264 - Flags: review?(dflanagan)
Attachment #8390694 - Flags: review?(dflanagan)
Attachment #8382586 - Attachment is obsolete: true
Attachment #8385840 - Attachment is obsolete: true
Attachment #8388045 - Attachment is obsolete: true
Attachment #8390264 - Attachment is obsolete: true
Attachment #8390264 - Flags: review?(dflanagan)
Comment on attachment 8390694 [details] [review]
pull-request (camera-new-features)

A few nits on github, and a question about how you deal with the HDR indicator when the hardware does not support HDR, but other than that this looks fine.

Thanks, Wilson!
Attachment #8390694 - Flags: review?(dflanagan) → review+
Landed on 'camera-new-features'
Status: UNCONFIRMED → 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)
Major Issue:

The HDR indicator on/off doesn't appear at all in the viewfinder. Also, a confirmation message should appear in the viewfinder when the user selects HDR on/off from the settings menu. 

Also:

The indicators look like buttons in it's current ON state. Also, in the OFF state when it's semi-transparent, it looks like the feature might still be on. I've created a bug with revised VSD for the indicators. See Bug 985092
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(amlee)
Resolution: FIXED → ---
Also, a confirmation message should appear in the viewfinder when the user selects Self-Timer from the settings menu.
Plus one to Amy's comments - HDR indicator is not appearing, and confirmation text isn't appearing when toggling HDR and self-timer options.
Flags: needinfo?(tshakespeare)
Assignee: nobody → singhashish1887
Flags: needinfo?(dmarcos)
I noticed that on rotation, 180 (portrait upside down) the indicator self-timer icon remains on the left instead of moving to the right. screenshot is upside down.
(In reply to Tiffanie Shakespeare from comment #24)
> Created attachment 8393701 [details]
> 180 orientation (portrait upside down)
> 
> I noticed that on rotation, 180 (portrait upside down) the indicator
> self-timer icon remains on the left instead of moving to the right.
> screenshot is upside down.

Dear Amy and Tiffanie,

We filed new bug for Comment 24 about rotation issue. Let's follow-up new Bug-989831.
And HDR Icon and Text feedback issues for comment 21 are fixed on Mozilla Master. 
So I'm closing this issue.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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
Target Milestone: --- → 1.4 S5 (11apr)
blocking-b2g: --- → 1.4+
You need to log in before you can comment on or make changes to this bug.