Add picture quality settings and variables#71
Conversation
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
Looks good, a few relatively minor comments.
Any reason not to do the non-numeric settings too, or was it just simpler to start with these?
For similar elsewhere, I've also delivered feedbacks to match if it's at a specific value.
| color: NaN, | ||
| brightness: NaN, | ||
| contrast: NaN, | ||
| sharpness: NaN, |
There was a problem hiding this comment.
Should we make volumeLevel NaN too if this is the best bet? Or undefined?
There was a problem hiding this comment.
Since 0 has meaning, I set it to NaN as the default value, but perhaps it should be undefined instead.
There was a problem hiding this comment.
undefined feels a better fit to me. Can you update volume too to match.
src/presets.js
Outdated
| const PICTURE_PRESETS = [ | ||
| { id: 'color', label: 'Color', default: 50 }, | ||
| { id: 'brightness', label: 'Brightness', default: 40 }, | ||
| { id: 'contrast', label: 'Contrast', default: 90 }, | ||
| { id: 'sharpness', label: 'Sharpness', default: 50 }, | ||
| ] |
There was a problem hiding this comment.
I don't think this data should be duplicated here and in actions, it should just be set once somewhere
There was a problem hiding this comment.
I have changed it to use the data within actions.js.
| PICTURE_SETTINGS.forEach((setting) => { | ||
| actions['set_' + setting.id] = { | ||
| name: setting.label, | ||
| options: [ |
There was a problem hiding this comment.
You may as well add the step setting they provide and populate the option with that too.
There was a problem hiding this comment.
I was sort of meaning pull it out of their config via the API, but maybe that's more effort than it's worth.
| name: setting.label, | ||
| options: [ | ||
| { | ||
| type: 'number', |
There was a problem hiding this comment.
I don't know the right answer here, but whilst number is technically correct, it sadly currently excludes variable support, meaning people couldn't e.g. use this to set the value via a rotary encoder.
There was a problem hiding this comment.
What do you think we should do?
There was a problem hiding this comment.
Honestly I don't know, I think our three options are:
- Stay with number, nicer UI with steps and min/max
- Switch to text, can enable variables, but probably need external value validation
- Do both with some fancy switch and isVisible toggling of two different fields
Maybe 1 makes sense just to start going for now. I think Companion centrally may improve the behaviour around that soon (it's just a shame they haven't done so already...).
…e duplicate values.
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
Sorry I really suck at reviewing at the moment it seems.
Can you add some feedbacks too (using your new internal values).
| const PICTURE_SETTINGS = [ | ||
| { id: 'color', label: 'Color', default: 50 }, | ||
| { id: 'brightness', label: 'Brightness', default: 40 }, | ||
| { id: 'contrast', label: 'Contrast', default: 90 }, | ||
| { id: 'sharpness', label: 'Sharpness', default: 50 }, | ||
| ] | ||
|
|
There was a problem hiding this comment.
Given we're now using the ones from actions:
| const PICTURE_SETTINGS = [ | |
| { id: 'color', label: 'Color', default: 50 }, | |
| { id: 'brightness', label: 'Brightness', default: 40 }, | |
| { id: 'contrast', label: 'Contrast', default: 90 }, | |
| { id: 'sharpness', label: 'Sharpness', default: 50 }, | |
| ] |
| down: [ | ||
| { | ||
| actionId: 'set_' + setting.id, | ||
| options: { value: setting.default }, |
There was a problem hiding this comment.
I normally provide say 5 presets from 0-100 with those values in the option and on the button (plus the feedback to show they're selected). Personally for me this preset is a bit pointless on it's own (and hard to test as an end user, as nothing will change).
| color: NaN, | ||
| brightness: NaN, | ||
| contrast: NaN, | ||
| sharpness: NaN, |
There was a problem hiding this comment.
undefined feels a better fit to me. Can you update volume too to match.
| PICTURE_SETTINGS.forEach((setting) => { | ||
| actions['set_' + setting.id] = { | ||
| name: setting.label, | ||
| options: [ |
There was a problem hiding this comment.
I was sort of meaning pull it out of their config via the API, but maybe that's more effort than it's worth.
| name: setting.label, | ||
| options: [ | ||
| { | ||
| type: 'number', |
There was a problem hiding this comment.
Honestly I don't know, I think our three options are:
- Stay with number, nicer UI with steps and min/max
- Switch to text, can enable variables, but probably need external value validation
- Do both with some fancy switch and isVisible toggling of two different fields
Maybe 1 makes sense just to start going for now. I think Companion centrally may improve the behaviour around that soon (it's just a shame they haven't done so already...).
2 participants
Added actions, presets, and variables for picture quality settings.
I have implemented the numerically configurable targets among those available in the following API.
https://pro-bravia.sony.net/develop/integrate/rest-api/spec/service/video/v1_0/setPictureQualitySettings/index.html