-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
YouTube's Fast Forward/Rewind behavior #4833
Conversation
Nice, your PR has a very professional feel to it. :P |
Yup. Everything is smooth and awesome and absolutely amazeballs, as expected. (I tested the release version). The only thing that could make it better would be removing that background shadow. :* |
@opusforlife2 Preferably I'd like to add a setting to give a opportunity to adjust it individually - why doesn't Github provide a poll functionality xD Besides of that I've increased the time for the triangle animation cycle a little bit. What do you think? |
I don't remember how long it took in previous builds. The current one looks fine, though.
You could say:
or something like that. |
@opusforlife2 This poll option is not really "optimal" :) It's a little bit difficult to decide when there are only a few people voting (3 votes aren't very meaningful - just remember the complaint when the unified player was introduced o.0). The shadow definitely ensures readability under all circumstances. In the issue back then I made a small "config" button but stopped it due to niche. Now I thought I could implement it for testing - why not ^^: app-release-setting-test_20201111.zip Note: The setting is not saved (changes won't be applied to the video player itself). |
No shadow still seems better to me. Why doesn't the universe like no shadow? ( ;´༎ຶД༎ຶ`) |
Oh, if you are talking the dark shadow covering all of the screen (and not only the arc background), then I agree with you @opusforlife2 that the shadow is unneeded. @vkay94 could you rebase? I will then review ;-) |
Okay, fine. I'll set it like the following (where shadow is the screen covering shadow, arc the rounded shadow with text and ,transparent arc the one with less transparency shadow): State before double tapping => what happens visually when beginning: (arc is always animating in)
State during double tapping => what happens visually when it's finished: (arc is always animating out)
**: In the previous versions I animated in the shadow for all cases when the video is paused even if the shadow wasn't visible then. Since the shadow is unwanted that much ( :( ) I'll leave it out in this specific case I guess but I need to check how the animation looks when the arc is animating out while the shadow and controls are animating in simultaneously though). @Stypox Yeah I'll rebase it and commit the above change. There was no need to do it since the debug app works (and I don't like to force-push when it's unnecessary + the feature isn't merge-ready anyway due to the update problem). |
a368eef
to
95ad4ef
Compare
@opusforlife2 @Stypox I uploaded a version with the configs above and updated the OP with the testing apk. Let me know whether the dark arc color (second screenshot) is fine (for me it's too light for reading on some frames which aren't "monotonous", but darker make it unsuitable for other videos xD) |
YAY NO SHADOW Bugs found:
|
Shapes and colors look good and polished to me now :-D |
I can reproduce it rarely indeed. I may look into it but I can't find a reason why it happens with the changes made (I call the
That was (partially) designed to work like this, so that you can't rewind endlessly. I added a small puffer that you can rewind only when the stream progress is greater or equal of a half second. PArtially because it should fast forward in this situation. I'll add a separate check for the forwarding in this case ;)
Unfortunately, I can't reproduce it, works as expected for me. Could you give me some more information please? (E.g. Android Version or whether it happens for all streams or only rarely). Thank you. @Stypox @opusforlife2 Thanks for your bug reports :) |
Good catch! Yeah, this is a 0.20.3 issue. I'll open a new issue for it. |
Hmm, I tried to reproduce it under the same conditions as you show in the video (locked orientation, navigation bar enabled and same video). I tried it on two devices with Android 8.0 and Android 10.0, but it doesn't happen. I've looked into the code and there might be a race condition between changing the position and animating. @Stypox Short question since it's hard to see in the gif: When it appears and you keep seeking, does it change the position or does it keep on the same side for this series of taps? I did a small change which should fix the race condition (I apply the correct position before animating in). Could you test it please? app-debug-frff-test-change-position.zip Thanks |
@vkay94 if I keep tapping it keeps happening (the side is not correctly set even after skipping multiple tens of seconds), and the apk you provided is no different unfortunately |
@Stypox That's weird. I've converted your GIF to a video and analyzed the taps. The arc side is always correct as I see and also the direction of the triangles. The only thing which is incorrect is the position of the view itself. I'm changing it via constraints programmatically. May I ask you which Android version you're running? @opusforlife2 Could you check whether you encounter Stypox issue as well, please? Edit: |
@vkay94 Already tried it yesterday. I couldn't reproduce it. Could be Huawei specific. |
If it's a manufactor-specific issue then I can't do much because I haven't got such device here. But if the above change doesn't fix it, I have one option at least to try :) |
@vkay94 with your last apk I can't reproduce it anymore. It was a bug with constraint layout, it seems like |
95ad4ef
to
951f6fb
Compare
Great to hear that 👍 (that means I've to update my library in the next time too :')). |
951f6fb
to
8da83dc
Compare
8da83dc
to
9ded838
Compare
9ded838
to
d716d44
Compare
* Deduplicated and simplified a lot of code * Fixed ``invalidSeekConditions`` so that it's possible to seek while the player is loading (like currently the case)
* Removed alphaRelativeDuration as there is no use for it
* When rewinding: Check if <0,5s * When fast-forwarding: Check if player has completed or the current playback has ended This allows rewinding on the endscreen
60be860
to
54ef604
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested fullscreen, landscape, vertical videos, specific player states (e.g. just after switching player, or soundcloud music videos, etc.) on API 19 emulated, API 31 emulated and API 29 on-device and couldn't find any issue. @litetex if we want to include this in this release, merge!
Wait, I forgot to test changing the settings... and found an issue. When a video is already playing and the fast-forward duration setting is changed, the player still fast-forwards by the old amount until the app is restarted. This does not happen on current |
Before this PR it didn't show any seek amount so I think this works as expected. Just kidding. I will fix it. Already found the bad line: Update: Should be fixed now |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works, and the fast-inexact-seek also works ;-)
Guys, let me know if you need help in clicking on merge button. I have skills |
Thank you to all contributors! |
Is this real or am I dreaming? |
Video Preview
What is it?
Description of the changes in your PR
This PR adds multi-double-tap seeking with YouTube's progress indicator style and a small change to alpha animation handling (dynamic duration depending on the view's alpha value).
I've decided to open this PR to discuss it here even if it isn't merge-ready instead of in the issue since the code base is basically done when #4587 was merged and I've reduced the added code to the minimum.
Features
To-do
Fixes the following issue(s)
Relies on the following changes
In the moment there's a design "bug" which results in heavy UI lagging during fast double tapping in some fragments. The reason is that with each seeking the history and playlist fragment is reloaded. Since this feature increases the frequency such high reloading is noticeable.
It could be fixed if the lists fragments apply changes to single items instead of reloading the whole list. But that should be done in another PR, probably by changing to the Groupie library.
APK testing (2020-12-01)
When you try it, please make sure you haven't opened the history or playlist detail fragments opened (also in tabs).
Releases page
Here are some visuals for the 2020-12-01-version with its states:
Normal video frame for reference
Playing + dark arc shadow
Paused + controls were visible (shadow + dark transparent arc)
So let's continue discussing it here with the view of the code implementation ;)
Due diligence