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

issue-10971-swipe-down-gesture-too-sensitive #11642

Closed
wants to merge 15 commits into from

Conversation

u7759982
Copy link

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

  1. change alphbet case from gradle file dependencies libraries, otherwise, gradle will fail
    Before :
    image
    After:
    image
  2. Implement distance threshold for the CustomBottomSheetBehavior
    Files that changed: app/src/main/java/org/schabi/newpipe/player/gesture/CustomBottomSheetBehavior.java
    Change screenshoots:
    image
    the distance threshold can be changed.
    image

Before/After Screenshots/Screen Record

  • Before: Swipe down gesture with very little distance but high speed will make the bottom sheet collapse
before.mp4
  • After: Implement a distance threshold to avoid too sensitive,also avoid accident swipe
after.mp4

Fixes the following issue(s)

APK testing

Due diligence

@github-actions github-actions bot added the size/medium PRs with less than 250 changed lines label Oct 25, 2024
@u7759982 u7759982 marked this pull request as draft October 25, 2024 09:50
@u7759982 u7759982 marked this pull request as ready for review October 25, 2024 09:52
@TobiGr TobiGr added GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) labels Oct 27, 2024
@TobiGr
Copy link
Member

TobiGr commented Oct 27, 2024

Thank you for your PR. Due to the influx of new PRs review might take some time. Please let me know if you are an ANU student for internal statistics and because I am writing my master thesis about open source contributions as part of learning computer science and computer science education at university.

@u7759982
Copy link
Author

u7759982 commented Oct 27, 2024

Thank you for your PR. Due to the influx of new PRs review might take some time. Please let me know if you are an ANU student for internal statistics and because I am writing my master thesis about open source contributions as part of learning computer science and computer science education at university.

Thanks for your reply, and yes, I am an ANU student at present. I think the CI fails possibly due to the reason I changed the character case, and I revert these changes.
1730024594814

Copy link
Member

@TobiGr TobiGr 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 for the PR. Please rebase it and follow the linter / checkstyle. I'll take a closer look once the pipelines succeed.

One general tip when working with platforms like GitHub and GitLab to propose changes and / or get code reviews: You do not need to post screenshots of your changes. The changes are tracked. If you want to comment on your changes, you can either reference them in the PR body or start discussions directly as part of the code review.

@@ -199,8 +199,7 @@ dependencies {
// name and the commit hash with the commit hash of the (pushed) commit you want to test
// This works thanks to JitPack: https://jitpack.io/
implementation 'com.github.TeamNewPipe:nanojson:1d9e1aea9049fc9f85e68b43ba39fe7be1c1f751'
// WORKAROUND: v0.24.2 can't be resolved by jitpack -> use git commit hash instead
implementation 'com.github.TeamNewPipe:NewPipeExtractor:176da72cb4c3ec4679211339b0e59f6b01bf2f52'
implementation 'com.github.TeamNewPipe:NewPipeExtractor:v0.24.2'
Copy link
Member

Choose a reason for hiding this comment

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

please clean your git history (rebase) and this is fixed on the dev branch.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your detailed advice. I am still wondering about whether implementing swipe down threshold is a proper methods to solve this. As when the threshold is implemented. Within the threshold, the user gesture will be ignored and no interaction can be seen. Previously, the instantce user drag the screen, the view window will move with the drag. After distance threshold implementation, within the threshold distance, when you drag, there is no response, the window will not move. I am not very sure if this is the correct direction to solve this problem.

@Stypox
Copy link
Member

Stypox commented Nov 12, 2024

Hello, and thank you for your contribution. I agree that just ignoring the gesture is not good UX. However in the refactored NewPipe gestures will be handled by NewPlayer, so any improvements should go there. Thank you anyway for your PR, I'm sorry for not giving you the chance to improve it right now, but we want to focus all of our energies on the refactor.

@Stypox Stypox closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) size/medium PRs with less than 250 changed lines
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

3 participants