Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brightness bug when "changing player" (after unified player, 2907) #4043

Closed
B0pol opened this issue Aug 1, 2020 · 7 comments · Fixed by #4223
Closed

Brightness bug when "changing player" (after unified player, 2907) #4043

B0pol opened this issue Aug 1, 2020 · 7 comments · Fixed by #4223
Labels
bug Issue is related to a bug GUI Issue is related to the graphical user interface

Comments

@B0pol
Copy link
Member

B0pol commented Aug 1, 2020

Version

Steps to reproduce the bug

  1. Disable auto brightness
  2. Lower your brightness
  3. Open a video, in fullscreen
  4. Increase the brightness via the player, by swiping up
  5. Exit fullscreen video (go to video details)
  6. (Optional) Minimize and close the video

Expected behavior

The brightness is back to system brightness. The fullscreen brightness is only used in fullscreen, not in video details or popup player (not the case with the last one).

Actual behaviour

The brightness is the same as the fullscreen video brightness. With step 6, it's really stupid because the video isn't even playing but we still have this brightness, and no way to change it except play a new video and lower brightness.

@B0pol B0pol added bug Issue is related to a bug GUI Issue is related to the graphical user interface labels Aug 1, 2020
@B0pol B0pol changed the title Brightness bug after #2907 Brightness bug when "changing player" (after unified player, 2907) Aug 1, 2020
@avently
Copy link
Contributor

avently commented Aug 2, 2020

it's really stupid because the video isn't even playing but we still have this brightness

You can still watch a video in portrait mode and the brightness you setup is needed in this case.
Let's say you are watching video at night and have low brightness while browsing other apps. Then you opened NewPipe in portrait mode and trying to see something but you can't because brightness is too low. For video it is important. Maybe you didn't notice that on Amoled but on IPS there is not so much contrast and the brightness for a video should be higher even in portrait (actually it's more important than in landscape because picture is smaller and some details could be missing from your eyes because of low brightness). I'm telling you as a guy who has a bad eyesight if it matters.

@B0pol
Copy link
Member Author

B0pol commented Aug 2, 2020

hen you opened NewPipe in portrait mode and trying to see something but you can't because brightness is too low.

then play fullscreen, otherwise the rest of the screen will burn your eyes.
Video playing on video details should be treated like popup mode, i.e. system brightness.

here is not so much contrast and the brightness for a video should be higher

I totally agree. The reverse thing also apply: when not playing a video, the brightness should be lower.
When you are on video details and scroll down you are not watching the video anymore, but reading comments. Therefore the brightness should be lower -> system brightness.

If you want higher brightness on video details or minized, just increase your system brightness or play in fullscreen, because most of the screen is not about the video. And even worse when you read comments or close the minimized state (when you scroll down), nothing on the screen is about video but we keep video brightness…

@MD77MD
Copy link

MD77MD commented Sep 2, 2020

I think this should be prioritized as it's missing badly with our eyes 🤤

@avently
Copy link
Contributor

avently commented Sep 3, 2020

@B0pol check this PR #4223

@opusforlife2
Copy link
Collaborator

Wow, it was worse than I thought. The brightness value persists across app restarts and shows up again when you open video details. But strangely, when you go fullscreen the brightness lowers to system level. And even more strangely, it goes back to the high value if you go back to video details. Very weird. Anyway, this was a much needed fix, so thanks, @avently.

@avently
Copy link
Contributor

avently commented Sep 3, 2020

@opusforlife2

The brightness value persists across app restarts

This is how it works for years :)

But strangely, when you go fullscreen the brightness lowers to system level

Nope, it's the same as was set in videoplayerfragment, no changes

And even more strangely, it goes back to the high value if you go back to video details

Still no, if you just watched a video in fullscreen and than rotated to portrait the brightness will be the same as was set by videodetailsfragment after initial load.

Anyway, after the PR the brightness will work the same way expect it will be set only for fullscreen mode. Or will be system default otherwise.

@opusforlife2
Copy link
Collaborator

This is how it works for years :)

Yes, but that was the correct behaviour in that case, because the video player was the only place with that high brightness. The video details page was still at system brightness. With the unified player, the video details page was consequently made high brightness as well, which is why this was considered a bug. The behaviour after your fix is more intuitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants