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.

Sign up for GitHub

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

Jump to bottom

Fix persistent hover overlay when in desktop/DeX mode or using a mouse/non-touch input #8316

Merged
merged 9 commits into from
Nov 9, 2022
Merged

Fix persistent hover overlay when in desktop/DeX mode or using a mouse/non-touch input #8316

merged 9 commits into from
Nov 9, 2022

Conversation

han-sz
Copy link
Contributor

@han-sz han-sz commented Apr 30, 2022 •
edited
Loading

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

A non-touch input device that supports hover events results in a persistent transparent overlay on top of the video player. When using an Android device in desktop mode or with a mouse, the content is obscured with said overlay.

The bug was first reported here: #4357

This PR adds an isDesktopMode check and removes the hover overlay for the detailThumbnailRootLayout only when in desktop mode (DeX, Android desktop, etc.)

Before/After Screenshots/Screen Record

  • Before:
    Screenshot_20220501-022300_NewPipe

  • After:
    Screenshot_20220501-022400_NewPipe fix_video_mouse_hover_overlay

Fixes the following issue(s)

APK testing

APK for testing: app-debug.apk.zip

Due diligence

Stypox
Stypox requested changes May 2, 2022
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, code looks almost good. But return directly ;-)

app/src/main/java/org/schabi/newpipe/util/DeviceUtils.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/DeviceUtils.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/DeviceUtils.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/DeviceUtils.java Outdated Show resolved Hide resolved
@han-sz han-sz force-pushed the fix_video_mouse_hover_overlay branch 2 times, most recently from 28f997d to dfe4594 Compare May 2, 2022 23:53
litetex
litetex previously requested changes May 3, 2022
View reviewed changes
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering NewPipe is designed for Android which is designed for touch input devices I'm not sure if we should implement this in the first place.

app/src/main/java/org/schabi/newpipe/util/DeviceUtils.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/DeviceUtils.java Outdated Show resolved Hide resolved
Stypox
Stypox approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks :-)
Let's wait for what @litetex has to say

@cybersphinx
Copy link
Contributor

cybersphinx commented Jul 7, 2022 •
edited
Loading

I see the same problem on my random cheap android tv shitbox (X88 Pro 10) so I tried this patch. As it is it doesn't help, because DeviceUtils.isDesktopMode doesn't return true, if I remove the condition it does work. Not sure if there's another way to detect usage as tv box, or an attached mouse (since presumably this would be annoying regardless of platform.)

edit: #4422 says that there are more devices / conditions where this overlay appears unwantedly. Maybe just disabling it unconditionally would be an option, since the player is highlighted by a border as well?

@cybersphinx
Copy link
Contributor

I've added a check for devices with cursors: cybersphinx@e86a34b (full branch)

With this it works on my tv box (and phone with attached mouse), I don't have any of the fancy devices mentioned in #4422 with stylus or finger hover detection.

There's an SDK version check around it that can/should be removed if minSdk is raised to 21.

@opusforlife2
Copy link
Collaborator

There's an SDK version check around it that can/should be removed if minSdk is raised to 21.

The barrier has been lifted.

@cybersphinx
Copy link
Contributor

Updated my branch to remove the check, and fix a silly oversight.

@Stypox
Copy link
Member

Stypox commented Jul 18, 2022

@cybersphinx do you want me to add your commits here? Or do you want to open a new PR?

@cybersphinx
Copy link
Contributor

If you can add them here I guess that's the best place.

@Stypox
Copy link
Member

Stypox commented Jul 20, 2022

Done; @litetex could you review again?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TobiGr TobiGr added bug Issue is related to a bug device/software specific Issues that only happen on some devices or with some specific hardware/software labels Jul 20, 2022
@opusforlife2 opusforlife2 requested a review from litetex September 1, 2022 15:15
@AudricV AudricV dismissed litetex’s stale review November 8, 2022 19:56

Outdated review and currently busy

@AudricV AudricV force-pushed the fix_video_mouse_hover_overlay branch from e9e68b4 to abf1cc5 Compare November 9, 2022 15:24
@AudricV
Copy link
Member

AudricV commented Nov 9, 2022

I rebased the PR and pushed a commit to the branch which improves the code added, let me know if it is good for you :)

@AudricV AudricV added GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) labels Nov 9, 2022
@AudricV AudricV removed the request for review from litetex November 9, 2022 15:26
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TobiGr TobiGr merged commit 627c6e2 into TeamNewPipe:dev Nov 9, 2022
@han-sz
Copy link
Contributor Author

han-sz commented Nov 10, 2022

Thanks @AudricV, looks good to me!

@ShareASmile ShareASmile mentioned this pull request Nov 10, 2022
Closed
@Stypox Stypox mentioned this pull request Jan 22, 2023
Closed
3 tasks
@Andronx

This comment was marked as resolved.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stypox Stypox Stypox approved these changes

@TobiGr TobiGr TobiGr approved these changes

@litetex litetex litetex left review comments

Assignees
No one assigned
Labels
bug Issue is related to a bug device/software specific Issues that only happen on some devices or with some specific hardware/software GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Physical Mouse: remove the white overlay on mousover from the videoplayer window
8 participants
@han-sz @cybersphinx @opusforlife2 @Stypox @AudricV @Andronx @TobiGr @litetex