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

Status: RESOLVED FIXED

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

Description

[singhashish1887]

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.

Comment 1

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 2

[Wilson Page [:wilsonpage]]

Landed on dmarcos/camera-dev

Comment 3

[Wilson Page [:wilsonpage]]

Comment on attachment 8382586 [details] [review]
Pull Request (camera-dev)

Simplified this patch and landed it on dmarcos/camera-dev

Comment 4

[singhashish1887]

Wilson can you please provide the pull request for your patch

Comment 5

[Hema Koka [:hema]]

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

Comment 6

[Diego Marcos [:dmarcos]]

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

Comment 7

[David Flanagan [:djf]]

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

Comment 8

[singhashish1887]

Diego has updated the patch .Diego can you please help us to re upload  the patches .

Comment 9

[singhashish1887]

Attachment: Pull Request (camera-dev)

It is updated code of Indicator and low battery .

Comment 10

[David Flanagan [:djf]]

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)

Comment 11

[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.

Comment 12

[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.

Comment 13

[singhashish1887]

Attachment: Pull Request(Camera-new-feature)

Previous Commit Link:
https://github.com/singhashish1887/gaia/commit/a28adade6ff7ce7d371f0eba4ba5d90c1d484f8c

Comment 14

[David Flanagan [:djf]]

(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.

Comment 15

[David Flanagan [:djf]]

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.

Comment 16

[singhashish1887]

Attachment: Pull Request (camera-dev) UT Test

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.

Comment 17

[Wilson Page [:wilsonpage]]

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

Comment 18

[David Flanagan [:djf]]

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!

Comment 19

[Wilson Page [:wilsonpage]]

Landed on 'camera-new-features'

Comment 20

[Hema Koka [:hema]]

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

Comment 21

[Amy Lee [:amylee] UX]

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

Comment 22

[Amy Lee [:amylee] UX]

Also, a confirmation message should appear in the viewfinder when the user selects Self-Timer from the settings menu.

Comment 23

[Tiffanie Shakespeare [:tif]]

Plus one to Amy's comments - HDR indicator is not appearing, and confirmation text isn't appearing when toggling HDR and self-timer options.

Comment 24

[Tiffanie Shakespeare [:tif]]

Attachment: 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.

Comment 25

[Youngjun Kim]

(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.

Comment 26

[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