Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2938913002:
Updates volume to change by .05 increments. (Closed)
DescriptionUpdates volume to change by .05 increments.
BUG=730519
Review-Url: https://codereview.chromium.org/2938913002
Cr-Commit-Position: refs/heads/master@{#488373}
Committed: https://chromium.googlesource.com/chromium/src/+/f5793c4dfa7d3e80e208e756d4fbc5eb9ea7cdae
Patch Set 1 #Patch Set 2 : Calls volume change in loop #Patch Set 3 : Addresses comment 22. #
Messages
Total messages: 30 (17 generated)
lethalantidote@google.com changed reviewers: + fbeaufort@chromium.org, lethalantidote@google.com, mlamouri@chromium.org
This changes it so that volume changes by .05 increments. However, if we do it this way, its a global change, meaning that using a mouse, we are also changing the volume by .05 increments. This may be annoying to some users. Since I generally manipulate global volume to balance issues like this out, I don't have a strong opinion, but we must be aware of that side-effect. If we want to have it only apply to global-keyboard controls, we can simulate multiple keyboard interactions instead. That is a more involved solution, but would give more fine grain control. That flow would mean that if the user interacts with the whole video using the keyboard to manipulate the volume, the steps would decrease by .05, but if they tab over to the volume control itself, more fine grain .01 control would be available. Please tell me what you think.
The CQ bit was checked by lethalantidote@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've created a custom volume button, set the step value to 0.05 on a 70px slider and plugged to a video to see how that feels when I move the cursor. I didn't notice any difference to be honest. On Wed, Jun 14, 2017 at 10:49 PM <lethalantidote@google.com> wrote: > This changes it so that volume changes by .05 increments. However, if we > do it > this way, its a global change, meaning that using a mouse, we are also > changing > the volume by .05 increments. This may be annoying to some users. Since I > generally manipulate global volume to balance issues like this out, I > don't have > a strong opinion, but we must be aware of that side-effect. > > If we want to have it only apply to global-keyboard controls, we can > simulate > multiple keyboard interactions instead. That is a more involved solution, > but > would give more fine grain control. That flow would mean that if the user > interacts with the whole video using the keyboard to manipulate the > volume, the > steps would decrease by .05, but if they tab over to the volume control > itself, > more fine grain .01 control would be available. > > Please tell me what you think. > > https://codereview.chromium.org/2938913002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I've created a custom volume button, set the step value to 0.05 on a 70px slider and plugged to a video to see how that feels when I move the cursor. I didn't notice any difference to be honest. On Wed, Jun 14, 2017 at 10:49 PM <lethalantidote@google.com> wrote: > This changes it so that volume changes by .05 increments. However, if we > do it > this way, its a global change, meaning that using a mouse, we are also > changing > the volume by .05 increments. This may be annoying to some users. Since I > generally manipulate global volume to balance issues like this out, I > don't have > a strong opinion, but we must be aware of that side-effect. > > If we want to have it only apply to global-keyboard controls, we can > simulate > multiple keyboard interactions instead. That is a more involved solution, > but > would give more fine grain control. That flow would mean that if the user > interacts with the whole video using the keyboard to manipulate the > volume, the > steps would decrease by .05, but if they tab over to the volume control > itself, > more fine grain .01 control would be available. > > Please tell me what you think. > > https://codereview.chromium.org/2938913002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
One alternative approach is to use step=0.05 would be to set step=0.01 and implement handlers for key up and key down in MediaControlVolumeSliderElement.cpp that will call StepUp(5) and StepDown(5) from https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... That should have very little side effect apart that if you tab to the volume slider, up and down arrows will do 0.05 moves instead of 0.01. Another approach with 0 side effect and most likely simpler is to not rely on the volume slider but simply change the volume from MediaControlsImpl when up/down are used. That means that in here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/media_... you could do: ``` if (key == "ArrowDown") { MediaElement().SetVolume(std::min(0, volume() - 0.05)); return; } if (key == "ArrowUp") { MediaElement().SetVolume(std::max(1, volume() + 0.05)); return; } ``` The benefit of this approach is that you don't rely on the UI and the volume slider can have a different behaviour. Setting the volume should fire an event which should update the slider.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/15 13:37:10, mlamouri (slow) wrote: > One alternative approach is to use step=0.05 would be to set step=0.01 and > implement handlers for key up and key down in > MediaControlVolumeSliderElement.cpp that will call StepUp(5) and StepDown(5) > from > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > That should have very little side effect apart that if you tab to the volume > slider, up and down arrows will do 0.05 moves instead of 0.01. > > Another approach with 0 side effect and most likely simpler is to not rely on > the volume slider but simply change the volume from MediaControlsImpl when > up/down are used. That means that in here > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/media_... > you could do: > ``` > if (key == "ArrowDown") { > MediaElement().SetVolume(std::min(0, volume() - 0.05)); > return; > } > if (key == "ArrowUp") { > MediaElement().SetVolume(std::max(1, volume() + 0.05)); > return; > } > ``` > The benefit of this approach is that you don't rely on the UI and the volume > slider can have a different behaviour. Setting the volume should fire an event > which should update the slider. So I tried the latter implementation, and got weird behavior, where it seems to have rounding error and also changed the volume by .1 instead of the desired .05. I tested this implementation out and it seems to work fine. Please let me know if there is an issue with it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/26 at 23:55:16, lethalantidote wrote: > So I tried the latter implementation, and got weird behavior, where it seems to have rounding error and also changed the volume by .1 instead of the desired .05. I think the .1 change is because we fire the event on keydown/press/etc so we end up calling the method twice. > I tested this implementation out and it seems to work fine. Please let me know if there is an issue with it. Is the "rounding" issue solved with this implementation?
On 2017/06/27 18:20:22, mlamouri (slow) wrote: > On 2017/06/26 at 23:55:16, lethalantidote wrote: > > So I tried the latter implementation, and got weird behavior, where it seems > to have rounding error and also changed the volume by .1 instead of the desired > .05. > > I think the .1 change is because we fire the event on keydown/press/etc so we > end up calling the method twice. > > > I tested this implementation out and it seems to work fine. Please let me know > if there is an issue with it. > > Is the "rounding" issue solved with this implementation? Yes it is.
On 2017/06/27 18:26:37, CJ wrote: > On 2017/06/27 18:20:22, mlamouri (slow) wrote: > > On 2017/06/26 at 23:55:16, lethalantidote wrote: > > > So I tried the latter implementation, and got weird behavior, where it seems > > to have rounding error and also changed the volume by .1 instead of the > desired > > .05. > > > > I think the .1 change is because we fire the event on keydown/press/etc so we > > end up calling the method twice. > > > > > I tested this implementation out and it seems to work fine. Please let me > know > > if there is an issue with it. > > > > Is the "rounding" issue solved with this implementation? > > Yes it is. Hello, did we have any further opinion on this?
On 2017/07/18 at 04:37:13, lethalantidote wrote:
> On 2017/06/27 18:26:37, CJ wrote:
> > On 2017/06/27 18:20:22, mlamouri (slow) wrote:
> > > On 2017/06/26 at 23:55:16, lethalantidote wrote:
> > > > So I tried the latter implementation, and got weird behavior, where it
seems
> > > to have rounding error and also changed the volume by .1 instead of the
> > desired
> > > .05.
> > >
> > > I think the .1 change is because we fire the event on keydown/press/etc so
we
> > > end up calling the method twice.
> > >
> > > > I tested this implementation out and it seems to work fine. Please let
me
> > know
> > > if there is an issue with it.
> > >
> > > Is the "rounding" issue solved with this implementation?
> >
> > Yes it is.
>
> Hello, did we have any further opinion on this?
Sorry about that. I think this is fine. Can you add some tests though? (and
remove the { } to follow the coding style)
On 2017/07/18 21:13:39, mlamouri (slow - plz ping) wrote:
> On 2017/07/18 at 04:37:13, lethalantidote wrote:
> > On 2017/06/27 18:26:37, CJ wrote:
> > > On 2017/06/27 18:20:22, mlamouri (slow) wrote:
> > > > On 2017/06/26 at 23:55:16, lethalantidote wrote:
> > > > > So I tried the latter implementation, and got weird behavior, where it
> seems
> > > > to have rounding error and also changed the volume by .1 instead of the
> > > desired
> > > > .05.
> > > >
> > > > I think the .1 change is because we fire the event on keydown/press/etc
so
> we
> > > > end up calling the method twice.
> > > >
> > > > > I tested this implementation out and it seems to work fine. Please let
> me
> > > know
> > > > if there is an issue with it.
> > > >
> > > > Is the "rounding" issue solved with this implementation?
> > >
> > > Yes it is.
> >
> > Hello, did we have any further opinion on this?
>
> Sorry about that. I think this is fine. Can you add some tests though? (and
> remove the { } to follow the coding style)
Added tests, please take a look.
The CQ bit was checked by mlamouri@chromium.org
lgtm
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500565071833520,
"parent_rev": "c1dd61f01ced0c94ccc2efe373a93c696bb98c2f", "commit_rev":
"f5793c4dfa7d3e80e208e756d4fbc5eb9ea7cdae"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500565071833520,
"parent_rev": "c1dd61f01ced0c94ccc2efe373a93c696bb98c2f", "commit_rev":
"f5793c4dfa7d3e80e208e756d4fbc5eb9ea7cdae"}
Message was sent while issue was closed.
Description was changed from ========== Updates volume to change by .05 increments. BUG=730519 ========== to ========== Updates volume to change by .05 increments. BUG=730519 Review-Url: https://codereview.chromium.org/2938913002 Cr-Commit-Position: refs/heads/master@{#488373} Committed: https://chromium.googlesource.com/chromium/src/+/f5793c4dfa7d3e80e208e756d4fb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f5793c4dfa7d3e80e208e756d4fb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
