Add volume booster above 100% #1444
Conversation
|
This should be ready for review now... |
There was a problem hiding this comment.
I have only tested this briefly so more bugs might be found later. However at the moment you have two critical bugs that I require you to fix.
- The showtoast is spammed when dragging over from under 100% to 100%. This might not seem like an issue to you depending on your OS, but for me it flickers.
- The color of the left bar even when under 100% is slightly more yellow/orange then the right brightness bar.
Both those should be fixed although I couldn't really reproduce the first one. |
|
Issue two is fixed, but issue one remain. I recommend to look at the log in android studio as showToast should print out repeatedly. |
|
While issue two is fixed, I would much rather you make the background progress of the orange bar invisible so it does not change the white background color at all above 100%. |
I tried numerous ways to do this and nothing seemed to work. Only thing that did work was a new drawable but I don't want to do that here, |
…ime and they can get annoying + bug prone
I decided to just have toasts show once as they get annoying anyway if you change volume a lot, and I think you can get the point after the first time anyway. |
| } | ||
| } | ||
|
|
||
| val level1Color = Color.WHITE |
There was a problem hiding this comment.
This entire part can be removed by adding android:progressTint="@color/colorPrimaryOrange"
There was a problem hiding this comment.
Whoops lol. That was a leftover from an old way this was done. Thanks!
|
|
||
| KeyEvent.KEYCODE_VOLUME_DOWN, | ||
| KeyEvent.KEYCODE_VOLUME_UP -> { | ||
| if (isLayout(PHONE or EMULATOR)) { |
There was a problem hiding this comment.
Why not tv, can you please explain why only the tv layout does not get volume boosting?
There was a problem hiding this comment.
Because TVs don't need it as much and a lot might not support it. In essence it doesn't do anything and instead would cause the volume bar used on phones to appear on TV when overriding the volume key yet sometimes it doesn't do anything different. So decided to just disable it here.
There was a problem hiding this comment.
If so, please note this with a comment.
There was a problem hiding this comment.
Also, if some devices do not support this, is there a possibility to query this support? Then disable in the case of no support.
There was a problem hiding this comment.
There probably is I just didn't think it was all that useful on TVs so just disabled it on the layout. I will add a comment though.
| @@ -981,7 +997,10 @@ open class FullScreenPlayer : AbstractPlayerFragment() { | |||
| val maxVolume = | |||
| audioManager.getStreamMaxVolume(AudioManager.STREAM_MUSIC) | |||
|
|
|||
| currentRequestedVolume = currentVolume.toFloat() / maxVolume.toFloat() | |||
| if (currentRequestedVolume == 0.0f) { | |||
There was a problem hiding this comment.
This can cause a very bad experience for the user, and is why it always queried in the old code. To exemplify this, imagine that the user watch a movie on the max volume, pauses the app, and switched to another app. Under the course of a few hours lowers the volume, then comes back.
In this case the volume would still be low, however the moment any volume is changed by only an single step the movie is back at maximum volume.
| var maxVolumePercentage = 200 | ||
| if (verticalAddition >= 0f) { | ||
| val nextVolume = currentRequestedVolume + verticalAddition | ||
| if (isAdjustingVolume && currentRequestedVolume <= 1.0f && nextVolume >= 1.0f) { |
There was a problem hiding this comment.
isAdjustingVolume just feels like a bad solution for this.
There was a problem hiding this comment.
If it is I will probably just abandon this as I spent probably 4-5 hours trying to find some better solution lol. I don't have it in me to find another solution again unless you have some suggestion?
There was a problem hiding this comment.
Well if you only want to have this lock feature, then just add a variable called isVolumeLocked = currentVolumePercentage < 100.0f or similar at the start of any swipe.
Additionally with this you could reset hasShownVolumeToast by if (isVolumeLocked) { hasShownVolumeToast = false; } in the very next line to allow for more then 1 toast per player instance.
There was a problem hiding this comment.
That's a great idea actually.
| // We only do this if the volume buttons are pressed as | ||
| // we handle sliding a bit different. | ||
| if (!hasShownVolumeToast && isVolumeUp && currentVolume < maxVolume) { | ||
| showToast(R.string.volume_exceeded_100) |
There was a problem hiding this comment.
This does not show up if we start a video at exactly 100% volume.
Resolves #1086