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

Better popup resizing with pinch gestures #3075

Merged
merged 4 commits into from Apr 10, 2020

Conversation

harshlele
Copy link
Contributor

@harshlele harshlele commented Feb 11, 2020

The current popup window does resize with pinch gestures, but it's a bit weird. It resizes to the distance between the 2 fingers, and changes its position to one of the fingers. I changed it so that the window size scales to the pinch-in/out gesture and stays in the same position.

Fixes #515 fixes #2880 fixes #2918

APK:
app-debug.zip

@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Feb 11, 2020
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 for this useful contribution!

checkPopupPositionBounds();
updateScreenSize();
if(firstPointerMoveX > scaledTouchSlop ||firstPointerMoveY > scaledTouchSlop ||
secPointerMoveX > scaledTouchSlop || secPointerMoveY > scaledTouchSlop){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
secPointerMoveX > scaledTouchSlop || secPointerMoveY > scaledTouchSlop){
secPointerMoveX > scaledTouchSlop || secPointerMoveY > scaledTouchSlop) {

double currentPointerDistance = Math.sqrt(currentXDiff * currentXDiff + currentYDiff * currentYDiff);

//scale popup width
double scale = 1 + (currentPointerDistance - initPointerDistance)/ initPointerDistance;
Copy link
Member

Choose a reason for hiding this comment

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

This is algebraically equal to:

Suggested change
double scale = 1 + (currentPointerDistance - initPointerDistance)/ initPointerDistance;
double scale = currentPointerDistance / initPointerDistance;

Comment on lines 1125 to 1127
float currentXDiff = event.getX(0) - event.getX(1);
float currentYDiff = event.getY(0) - event.getY(1);
double currentPointerDistance = Math.sqrt(currentXDiff * currentXDiff + currentYDiff * currentYDiff);
Copy link
Member

Choose a reason for hiding this comment

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

There is the Java Math.hypot() function that does exactly this

Suggested change
float currentXDiff = event.getX(0) - event.getX(1);
float currentYDiff = event.getY(0) - event.getY(1);
double currentPointerDistance = Math.sqrt(currentXDiff * currentXDiff + currentYDiff * currentYDiff);
double currentPointerDistance = Math.hypot(event.getX(0) - event.getX(1), event.getY(0) - event.getY(1));

Comment on lines 1137 to 1138
}
else{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
else{
} else {

@Stypox Stypox self-assigned this Mar 3, 2020
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, almost ready ;-)
When you are finished implementing these suggestions, could you provide another test apk, so that I can test on the latest changes? Thanks

Comment on lines 1118 to 1119
if(firstPointerMoveX > scaledTouchSlop ||firstPointerMoveY > scaledTouchSlop ||
secPointerMoveX > scaledTouchSlop || secPointerMoveY > scaledTouchSlop) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(firstPointerMoveX > scaledTouchSlop ||firstPointerMoveY > scaledTouchSlop ||
secPointerMoveX > scaledTouchSlop || secPointerMoveY > scaledTouchSlop) {
if (firstPointerMoveX > scaledTouchSlop || firstPointerMoveY > scaledTouchSlop
|| secPointerMoveX > scaledTouchSlop || secPointerMoveY > scaledTouchSlop) {

@@ -1082,25 +1105,47 @@ public boolean onTouch(View v, MotionEvent event) {
private boolean handleMultiDrag(final MotionEvent event) {
if (event.getPointerCount() != 2) return false;

final float firstPointerX = event.getX(0);
final float secondPointerX = event.getX(1);
if(initPointerDistance != -1){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(initPointerDistance != -1){
if (initPointerDistance != -1) {

@Hubcapp
Copy link

Hubcapp commented Mar 8, 2020

I do prefer this behavior, but it seems that expanding isn't as smooth as it was with the weird "wherever your finger is" resize.

When i drag the popup box to the right side of the screen and expand, it remains the same size until I'm finished resizing it, (or for about that amount of time anyway...), but it moves the box over to the left.

For 4:3 video, you can see the black bars coming off the sides (why they need to be there at all for popup mode, i don't know, but it's an indicator that resizing is laggy)

@ghost
Copy link

ghost commented Mar 8, 2020

@harshlele There is a little glitch during popup scaling, in practice animation doesn't correctly follow the fingers movements

@ghost
Copy link

ghost commented Mar 8, 2020

Except this all working well.

@harshlele
Copy link
Contributor Author

@Hubcapp @Oizaro im not sure how to solve that problem. i tried a few things but its always a bit stuttery.

@harshlele
Copy link
Contributor Author

@Stypox is there like a linting test for the coding style so all these small coding style mistakes could be caught before I commit?

APK:
app-debug.zip

@Stypox
Copy link
Member

Stypox commented Mar 9, 2020

@harshlele running Android Studio code refactoring on the files you changed should do the trick

@Stypox
Copy link
Member

Stypox commented Apr 4, 2020

I tested this once again and could not find problems. I can't reproduce the issue pointed out by @Oizaro, but even if the bug exists I don't think it is really a big problem, compared to how clunky popup resizing is now.
So could you @harshlele rebase and fix checkstyle problems, so than it can then be merged? Sorry for the delay ;-)

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.

A few style issues, but this is ready :-D

//scale popup width
double scale = currentPointerDistance / initPointerDistance;

newWidth = (popupWidth * scale);
Copy link
Member

@Stypox Stypox Apr 4, 2020

Choose a reason for hiding this comment

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

Remove the double newWidth = popupWidth; declaration above and put it directly here:

Suggested change
newWidth = (popupWidth * scale);
double newWidth = popupWidth * scale;


//change co-ordinates of popup so the center stays at the same position
popupLayoutParams.x += (popupWidth - newWidth)/2;

Copy link
Member

Choose a reason for hiding this comment

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

Also remove some of these spaces and group similar/related statements toghether, to enhance readability

@Stypox Stypox added this to the 0.19.3 milestone Apr 10, 2020
harshlele and others added 4 commits April 10, 2020 22:10
Also calculate differently the moved distance of a pointer: use euclidean and not manhattan geometry
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 again for fixing this long-lasting issue! I pushed the style fixes myself, since they were very little changes, so this is now ready to be merged. I tested on Android 7.1, Android 4.4 and Android 10

@Stypox Stypox merged commit feab633 into TeamNewPipe:dev Apr 10, 2020
This was referenced Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resizing popup player {Add Feature}popup resize problem Popup window resize
4 participants