Gesture controls #1604

TheMatten · 2018-08-15T10:30:29Z

I carefully read the contribution guidelines and agree to them.

Current gesture controls work but look like an afterthought. This PR introduces circular design with progress bar and material icons that changes depending on current value.

Demo:

Changed version seems to work without any problems.

mauriciocolli · 2018-08-18T02:43:41Z

Thanks for the pull request, looks good!

Current gesture controls work but look like an afterthought

Yep, that's right, there's even a TODO there. I think you're talking about just the UI, but the code that handle gestures isn't really the best way to do it and it was meant as just a working solution, but yeah... (#503)

mauriciocolli · 2018-08-18T02:46:12Z

app/src/main/java/org/schabi/newpipe/player/MainVideoPlayer.java

+                        100
+                );
+                if (brightnessNormalized < 25) {
+                    playerImpl.getBrightnessImageView().setImageDrawable(


Crash in old versions of Android.
A very simple solution, thankfully: use AppCompatResources.getDrawable instead.

I was also worried about performance, but it seems to be cached, both the Android and compat library versions, so it's all good.

Thanks for that - AS stopped complaining when I used it through getResources(), but yeah, I didn't check it manually...

mauriciocolli · 2018-08-18T02:47:27Z

app/src/main/java/org/schabi/newpipe/util/AnimationUtils.java

+     * @param to          final progress value after animation
+     * @param duration    how long the animation will take, in milliseconds
+     */
+    public static void animateProgressBar(ProgressBar progressBar, int to, long duration) {


Put this function below the others, but on top of the Internals block.

OK, I'll do

TheMatten · 2018-08-18T11:43:07Z

@mauriciocolli Design seemed like a biggest problem, but while tinkering with it, I thought about implementing some sort of speed-based behavior, so that user can change values more precisely by moving finger slowly. May try it, as well as maybe introduce some "fake" state of progress bar that rounds to the closest value after gesture, instead of that choppy movement.

TheMatten · 2018-08-18T13:11:20Z

@mauriciocolli While talking about gesture controls, there's one more thing that doesn't feel right - when using fast rewind controls, bright background may make icons barely visible:

E.g. YT web player solves this problem with dark circle as a background:

The other thing we could do is temporarily dim the background (as with play/pause), but then rewind would still be in conflict with play button.

TheMatten · 2018-08-20T07:58:00Z

Wow - it seems like GestureDetector's and GestureDetectorCompat's onScroll gets called differently. Luckily, Compat version seems to be more sensitive (it works with slow movement).

After a lot of testing, to me the best solution seems to use event's distance directly as a value for progress incrementation and set max progress value based on screen size. Speed seems to be inconsistent and attempts to "normalize" value before use by e.g division or log result in number rounded down to zero when doing small movements.

I could maybe solve this by some overcomplicated solution I didn't think of or by skipping every Nth event and using progress animation again, but I don't think it would give us any advantage. Maybe if drawing on screen was too expensive, but actually, number of total motion events (including historical) should be <= touchscreen scan rate, which will probably match framerate (I've counted <= 60 per second on my device with input swipe command). And in reality, profiler barely shows any difference in load while using new implementation.

At the end, we can think of space used for gesture as an invisible seekbar, which is predictable, precise and smooth solution at the same time (and allows us to get from 0% to 100% at one long swipe). Value from it gets converted to nearest volume/brightness level and applied.

Demo:

(Edit: sorry for force push - I've corrected typo in one commit message)

mauriciocolli · 2018-08-22T03:10:00Z

when using fast rewind controls, bright background may make icons barely visible

IMO it doesn't matter much, the user is focused on the video and will certainly notice the rewind/fast forward. Though I don't mind if you want to make more visible, feel free to do it (of course, in a way that doesn't draw too much attention).

And the fact that the icons stay very shortly in screen (video is already loaded) makes the player seems snappy, though the "artifacts" (progress bar visible in a split second, for example) can be seen as a buggy player UI to the user. The optimal solution would be to keep the short animation and fix them.

we can think of space used for gesture as an invisible seekbar

Exactly, I like this implementation, works and feels much better.

Oh, and if you don't mind, please, when this PR is ready remove the commits that cancel each other.

mauriciocolli · 2018-08-22T03:13:13Z

app/src/main/java/org/schabi/newpipe/player/MainVideoPlayer.java

+        private int maxGestureLength;
+
+        {
+            DisplayMetrics m = getResources().getDisplayMetrics();


What do you think of using the rootView's width and height?

The code would be simpler, and using this approach, we could already support the multi window mode just by adding a listener to the rootView layout changes, that when we detect one, we could simply update the variable and the gesture would work correctly in the new size.

Okay, will do.

mauriciocolli · 2018-08-22T03:13:36Z

app/src/main/java/org/schabi/newpipe/player/MainVideoPlayer.java


        private final int MOVEMENT_THRESHOLD = 40;
-        private final int eventsThreshold = 8;
-        private boolean triggered = false;
-        private int eventsNum;

        // TODO: Improve video gesture controls


I think we can remove this TODO.

TheMatten · 2018-08-22T07:30:53Z

@mauriciocolli

Though I don't mind if you want to make more visible, feel free to do it (of course, in a way that doesn't draw too much attention).

Just a simple background similar to YT or those new gesture controls to make it consistent.

And the fact that the icons stay very shortly in screen (video is already loaded) makes the player seems snappy, though the "artifacts" (progress bar visible in a split second, for example) can be seen as a buggy player UI to the user.

Otherwise it's fine, I was just interested in "visibility". Frankly, any sort of long animation or effects would just get in the way of users while watching.

Oh, one more thing - #1603 (3. point) and #1199 - those raster images look terrible when scaling, but that's thing for separate PR.

TheMatten · 2018-08-22T12:27:32Z

Okay, cleaned up history - commits changing gesture control are squashed into one big (newer ones were just fixes anyway). Oh, and changed threshold check to not block detection while moving and added check for vertical movement - now there's no stuttering when moving through starting location and user will not trigger gesture when e.g. cleaning display :D

Checked both code and running app, everything seems fine.

mauriciocolli · 2018-08-22T15:48:45Z

app/src/main/java/org/schabi/newpipe/player/MainVideoPlayer.java

@@ -424,6 +440,20 @@ public void initViews(View rootView) {
            titleTextView.setSelected(true);
            channelTextView.setSelected(true);

+            getRootView().addOnLayoutChangeListener((view, l, t, r, b, ol, ot, or, ob) -> {
+                // Use smaller value to be consistent between screen orientations


Little thing that's bothering the micro-optimization part of me, we could return if the size itself didn't changed:

if (left == oldLeft && right == oldRight && top == oldTop && bottom == oldBottom){ return; }

What about if (l != ol || t != ot || r != or || b != ob) { // do things... } ?

It's fine, this piece of code is short enough to be a good a choice.

TheMatten · 2018-08-22T19:45:29Z

Added check to max length updates - I should be ready for merging.

mauriciocolli · 2018-08-23T03:20:18Z

app/src/main/java/org/schabi/newpipe/player/MainVideoPlayer.java

+                    // Use smaller value to be consistent between screen orientations
+                    // (and to make usage easier)
+                    int width = r - l, height = b - t;
+                    maxGestureLength = (int) ((width > height ? width : height) * MAX_GESTURE_LENGTH);


Isn't this check inverted?

And while you fix this, take the opportunity and move this to the initListeners function, I think it makes more sense there.

Oh, now I've got it, sorry...

Just throw a Math.min(width, height) in there, even easier to read.

Much better idea, thanks.

mauriciocolli · 2018-08-23T03:37:29Z

app/src/main/java/org/schabi/newpipe/player/MainVideoPlayer.java

+                if (DEBUG) Log.d(TAG, "onScroll().brightnessControl, currentBrightness = " + currentProgressPercent);
+
+                if (currentProgressPercent < 0.25) {
+                    playerImpl.getBrightnessImageView().setImageDrawable(


I think we could improve readability and be less redundant here (and above) introducing a variable:

// We could initialize at the "else" value, but I think // this way makes it more readable. final int iconRes; if (currentProgressPercent < 0.25) { iconRes = R.drawable.ic_brightness_low_white_72dp; } else if (currentProgressPercent < 0.75) { iconRes = R.drawable.ic_brightness_medium_white_72dp; } else { iconRes = R.drawable.ic_brightness_high_white_72dp; }

What do you think?

Not a bad idea, will do.

Hmm, ternary operator is right-associative... :

playerImpl.getVolumeImageView().setImageDrawable( AppCompatResources.getDrawable(getApplicationContext(), currentProgressPercent <= 0 ? R.drawable.ic_volume_off_white_72dp : currentProgressPercent < 0.25 ? R.drawable.ic_volume_mute_white_72dp : currentProgressPercent < 0.75 ? R.drawable.ic_volume_down_white_72dp : R.drawable.ic_volume_up_white_72dp ) );

Can I use this? :D

Definitely hard to read, but if you keep the condition result in the same line, yeah. A variable here can help it again to keep the line shorter and the style closer to what is used currently:

final int resId = currentProgressPercent <= 0 ? R.drawable.ic_volume_off_white_72dp : currentProgressPercent < 0.25 ? R.drawable.ic_volume_mute_white_72dp : currentProgressPercent < 0.75 ? R.drawable.ic_volume_down_white_72dp : R.drawable.ic_volume_up_white_72dp;

I just wanted to avoid additional variable... but okay, will do.

TheMatten · 2018-08-24T08:03:33Z

Fixed listener and res selection.

-Previous version used emojis for brightness and volume icons, which may be inconsistent across devices and do not fit well with other parts of UI (Frankly, previous version was more informative than eye-candy) -This commit replaces old version with circular progress bar that shows current value (before conversion). Gesture mode (volume/brightness) is indicated by icon that changes between (4/3) modes according to current value -Text information about current value was removed, because with progress bar present it does not add any real value to UI.

-White icon was barely visible on bright backgrounds -Secondly, drawable is set programmatically anyway and so it's setting in XML is good just for a confusion

TheMatten · 2018-08-24T17:08:23Z

@mauriciocolli Great! Thanks for your help (and patience) :)

TheMatten changed the title ~~Player controls~~ Gesture controls Aug 15, 2018

mauriciocolli suggested changes Aug 18, 2018

View reviewed changes

TheMatten force-pushed the player_controls branch from 4896b40 to 4db6004 Compare August 20, 2018 07:20

mauriciocolli suggested changes Aug 22, 2018

View reviewed changes

TheMatten force-pushed the player_controls branch 3 times, most recently from 6e35b29 to a2456cb Compare August 22, 2018 12:25

mauriciocolli reviewed Aug 22, 2018

View reviewed changes

TheMatten force-pushed the player_controls branch from a2456cb to e936640 Compare August 22, 2018 19:44

mauriciocolli suggested changes Aug 23, 2018

View reviewed changes

mauriciocolli reviewed Aug 23, 2018

View reviewed changes

TheMatten force-pushed the player_controls branch from e936640 to 711ff43 Compare August 24, 2018 07:56

TheMatten added 2 commits August 24, 2018 13:24

Add background to fast rewind icon, change android:src to tools:src

a469086

-White icon was barely visible on bright backgrounds -Secondly, drawable is set programmatically anyway and so it's setting in XML is good just for a confusion

mauriciocolli force-pushed the player_controls branch from fe46574 to a469086 Compare August 24, 2018 16:25

mauriciocolli approved these changes Aug 24, 2018

View reviewed changes

mauriciocolli merged commit 4e478c6 into TeamNewPipe:dev Aug 24, 2018

TheMatten mentioned this pull request Aug 24, 2018

ExoPlayer volume/brightens too sensitive #503

Closed

theScrabi mentioned this pull request Aug 28, 2018

Release v0.14.0 #1635

Closed

TobiGr mentioned this pull request Aug 29, 2018

Reciever not registered #1641

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gesture controls #1604

Gesture controls #1604

TheMatten commented Aug 15, 2018 •

edited

Loading

mauriciocolli commented Aug 18, 2018

mauriciocolli Aug 18, 2018

TheMatten Aug 18, 2018

mauriciocolli Aug 18, 2018

TheMatten Aug 18, 2018

TheMatten commented Aug 18, 2018

TheMatten commented Aug 18, 2018

TheMatten commented Aug 20, 2018 •

edited

Loading

mauriciocolli commented Aug 22, 2018

mauriciocolli Aug 22, 2018

TheMatten Aug 22, 2018

mauriciocolli Aug 22, 2018

TheMatten Aug 22, 2018

TheMatten commented Aug 22, 2018

TheMatten commented Aug 22, 2018 •

edited

Loading

mauriciocolli Aug 22, 2018 •

edited

Loading

TheMatten Aug 22, 2018

mauriciocolli Aug 23, 2018

TheMatten commented Aug 22, 2018

mauriciocolli Aug 23, 2018

TheMatten Aug 23, 2018 •

edited

Loading

mauriciocolli Aug 24, 2018

TheMatten Aug 24, 2018

mauriciocolli Aug 23, 2018 •

edited

Loading

TheMatten Aug 23, 2018

TheMatten Aug 23, 2018

mauriciocolli Aug 24, 2018

TheMatten Aug 24, 2018

TheMatten commented Aug 24, 2018

TheMatten commented Aug 24, 2018 •

edited

Loading

Gesture controls #1604

Gesture controls #1604

Conversation

TheMatten commented Aug 15, 2018 • edited Loading

mauriciocolli commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMatten commented Aug 18, 2018

TheMatten commented Aug 18, 2018

TheMatten commented Aug 20, 2018 • edited Loading

mauriciocolli commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMatten commented Aug 22, 2018

TheMatten commented Aug 22, 2018 • edited Loading

mauriciocolli Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMatten commented Aug 22, 2018

Choose a reason for hiding this comment

TheMatten Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciocolli Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMatten commented Aug 24, 2018

TheMatten commented Aug 24, 2018 • edited Loading

TheMatten commented Aug 15, 2018 •

edited

Loading

TheMatten commented Aug 20, 2018 •

edited

Loading

TheMatten commented Aug 22, 2018 •

edited

Loading

mauriciocolli Aug 22, 2018 •

edited

Loading

TheMatten Aug 23, 2018 •

edited

Loading

mauriciocolli Aug 23, 2018 •

edited

Loading

TheMatten commented Aug 24, 2018 •

edited

Loading