Closed Bug 974337 Opened 7 years ago Closed 7 years ago

[Camera] Display all pictureSizes in settings

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: wilsonpage, Assigned: wilsonpage)

Details

Attachments

(1 file, 4 obsolete files)

We have decided to display all pictureSizes exposed to use through Gecko in the settings menu. Partners should be able to specify exact picture sizes should they wish to only display a sub-set for their particular device.
Attached file pull-request (wip) (obsolete) —
Attachment #8378958 - Attachment description: pull → pull-request (wip)
Attachment #8378958 - Flags: review?(dmarcos)
Please take a look at changes to previewSize selection in PR for bug 974264 to see if CameraUtils can be used to find a previewSize based on the selected pictureSize (instead of viewportSize as it currently is).
Flags: needinfo?(wilsonpage)
Assignee: nobody → wilsonpage
Flags: needinfo?(wilsonpage)
Attached file patch (camera-dev) (obsolete) —
Attachment #8380455 - Flags: feedback?(dflanagan)
Comment on attachment 8380455 [details]
patch (camera-dev)

Lots of comments on the commit in github.

I'm giving feedback- in this case because I realized that your settings are completely unlocalized, which means that the code on master is badly broken and should not have landed. Each setting in app.js should have an l10n id instead of a title, and you should use that id to localize in views/setting.js.

Other things I'm concerned about:

I didn't see any code in this patch that actually took the set of supported picture sizes and configured that as the list of options for the pictureSize setting.

It seems really awkward (and perhaps dangerous with persistence) that the pick activity modifies the the set of options for the picture and video size settings. If the pick activity specifies a size constraint, I think it is fine if you just force the size and disable that setting. The user does not need to be able to choose among a list of sizes. Those size settings are basicaly just for MMS.
Attachment #8380455 - Flags: feedback?(dflanagan) → feedback-
LOCALIZATION

Great point, I completely missed that! I've just created bug 976203 to track the implementation of this. I'm not concerned that the code in master is not localized as although the settings framework is powering application configuration, the settings menu is hidden by default.

PICK ACTIVITY

The settings options are whittled down to options that match the passed activity parameters. When we choose a suitable option inside the ActivityController it will be silently selected, meaning that it won't be persisted. If the user were to open the settings menu and change to an alternative 'allowed' picture size, then this *would* persist for the next session.

We have a couple of options:

1. Hide the settings menu in pick activity sessions to prevent the user from changing picture sizes.
2. Provide only one pictureSize in the options, so it cannot be changed and therefore persist until the next session.
Flags: needinfo?(dflanagan)
(In reply to Wilson Page [:wilsonpage] from comment #5)
> LOCALIZATION
> 
> Great point, I completely missed that! I've just created bug 976203 to track
> the implementation of this. I'm not concerned that the code in master is not
> localized as although the settings framework is powering application
> configuration, the settings menu is hidden by default.

Good point. That helps me relax.

> 
> PICK ACTIVITY
> 
> The settings options are whittled down to options that match the passed
> activity parameters. When we choose a suitable option inside the
> ActivityController it will be silently selected, meaning that it won't be
> persisted. 

Okay, I guess. But please comment, because that is a nuance of the code that completely escaped me. It resquires understanding that the setting is saved in response to an event rather than being done directly in the set method.

If the user were to open the settings menu and change to an
> alternative 'allowed' picture size, then this *would* persist for the next
> session.

I wonder what UX would say about that. My first reaction is that that would be wrong. But I'm not sure.

> We have a couple of options:
> 
> 1. Hide the settings menu in pick activity sessions to prevent the user from
> changing picture sizes.

Can you just hide or disable the one pictureSize entry in the settings menu?  The user might still want to access settings for other features (or future features) I suppose.

> 2. Provide only one pictureSize in the options, so it cannot be changed and
> therefore persist until the next session.

This seems like an solution to me.  Again, this is only used for MMS, so a general solution is not needed.
Flags: needinfo?(dflanagan)
Attached file pull-request (camera-new-features) (obsolete) —
Attachment #8378958 - Attachment is obsolete: true
Attachment #8380455 - Attachment is obsolete: true
Attachment #8378958 - Flags: review?(dmarcos)
Attachment #8385202 - Flags: review?(dflanagan)
As agreed feedback will be carried out as 'follow-up' bugs:

1. Localization - bug 976203
2. Preventing change of picture-size during 'pick' - bug 978727
(In reply to Wilson Page [:wilsonpage] from comment #8)
> As agreed feedback will be carried out as 'follow-up' bugs:
> 
> 1. Localization - bug 976203

I just gave a preemptive r- on that bug even though there is not a PR yet.

> 2. Preventing change of picture-size during 'pick' - bug 978727

There isn't anything in that bug yet. Let's get someone assigned. If the mozilla camera isn't going to offer any resolution choice in the menu, then perhaps this is an issue that LG needs to be aware of.
Comment on attachment 8385202 [details] [review]
pull-request (camera-new-features)

Sorry, but I can't land this.

I'm willing to defer major changes to followup bugs. But there are lots of minor changes requested from my previous review that have not been addressed. Clarifications, spelling corrections other relatively simple things like not confusing bytes and pixels. Though there is also at least one bug identified in my previous review that has not been addressed yet.

See my comments on github on this PR, but also please review my comments on the earlier commit here: https://github.com/dmarcos/gaia/commit/e1a0c11a8d3f62702fb199937f2f218170bb59c1

Also, with this patch applied, there is apparently nothing in the app that ever inspect camera.capabilities.pictureSizes to find out what sizes are supported.  I think that means that the camera will use the default size wnen invoked normally, but I also suspect that it will just fail when invoked for an activity.

I don't want to partial features. If there is a different bug that initializes the set of possible values for the pictureSizes setting, then please combine those two patches into a single PR so we don't land something that does not work. 

Finally, there are no tests in this PR. If you've got tests for this feature coming in a later commit, I'm fine with that, but just point me to where they are so I can take a look before landing.
Attachment #8385202 - Flags: review?(dflanagan) → review-
djf: I'm going to work on addressing your comments today so we can expedite moving some of these patches over onto camera-new-features
Attachment #8385202 - Attachment is obsolete: true
Attachment #8386408 - Flags: review?(dflanagan)
Attachment #8386408 - Attachment is obsolete: true
Attachment #8386408 - Flags: review?(dflanagan)
Attachment #8386480 - Flags: review?(dflanagan)
Target Milestone: --- → 1.4 S3 (14mar)
Comment on attachment 8386480 [details] [review]
UNIFIED PATCH: pull-request (camera-new-features)

Landed in 980599. Clearing review request
Attachment #8386480 - Flags: review?(dflanagan)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.