TeamNewPipe / NewPipe

A libre lightweight streaming front-end for Android.
https://newpipe.net
GNU General Public License v3.0
31.08k stars 3.01k forks source link

Standardize view animation duration #3038

Open Stypox opened 4 years ago

Stypox commented 4 years ago

If you check the usages of animateView you will see that the typical range will be 0 all the way to 500, with most in the 100-250 range.

It should've been done with a default value in the first place. It is pretty much arbitrary right now, it was whatever the person felt like at the time of writing.

I'll leave this task to another PR, which the goal will be to standardize the used duration of animations.

_Originally posted by @mauriciocolli in https://github.com/TeamNewPipe/NewPipe/pull/2309#discussion_r371625549

ludugz commented 4 years ago

Hi @Stypox , I check the source code and see that most values are from 0-500 as you mentioned above. Before fixing this, I want to make sure that is it OK if I change all the values from 0-500 to 250 (average)? In the case of value bigger than 500 ( f.e 800), should I just leave it there?

wb9688 commented 4 years ago

I vote for 420 ms :rofl:

If you want to find all magic numbers in our code that aren't a constant, you could uncomment https://github.com/TeamNewPipe/NewPipe/blob/dev/checkstyle.xml#L148, see https://checkstyle.org/config_coding.html#MagicNumber. We should reduce the amount of magic numbers in general.

Stypox commented 4 years ago

Hi @Stypox , I check the source code and see that most values are from 0-500 as you mentioned above. Before fixing this, I want to make sure that is it OK if I change all the values from 0-500 to 250 (average)? In the case of value bigger than 500 ( f.e 800), should I just leave it there?

250 seems good to me :-D Anyway, if that value doesn't turn out to be good it will be changeable with only one edit, since it would be a constant. So go for 250ms ;-)

ludugz commented 4 years ago

I also vote for 250ms :D, probably have the issue fixed this weekend.

Novmbrain commented 2 years ago

Hello, we are 7 students majoring in Computer Science from IMT Atlantique in France. We are willing to do some contributions to this project. We work together in this fork, MohammedAymane/NewPipe. Because the pull request of @ludugz were closed, could we take this issue? Looking forward to your reponse :)

TobiGr commented 2 years ago

@Novmbrain Thank your for taking a look at this issue. Are your contributions part of a seminar or course? If yes, please add some info on how quickly you need feedback to the PR description :)

Novmbrain commented 2 years ago

@Novmbrain Thank your for taking a look at this issue. Are your contributions part of a seminar or course? If yes, please add some info on how quickly you need feedback to the PR description :)

Hello, Yes! Our contributions are part of a course and also the main point of this course. We can give you this first feedback 02/11.

Novmbrain commented 2 years ago

Hello @TobiGr ! We are back We already set up the environment needed and launch Newpipe in our machine and we have watched the last @ludugz's works. Firstly, by searching key word 'animate(' in the project. We found all magic numbers passed to this (But we are not sure about it).

Secondly, wee found that ludugz defined two constants in AnimationsUtils

import static org.schabi.newpipe.util.AnimationUtils.DEFAULT_LONG_ANIM_DURATION;
import static org.schabi.newpipe.util.AnimationUtils.DEFAULT_SHORT_ANIM_DURATION;

Due a long time has passed since 16/04/2020, the source code is different that before. It seems to us that the class "AnimationsUtils" has been removed. We also want to defines two constants to replace all magic numbers, however magics numbers exist in plusieurs Classes such as LocalPlaylistFragment, LocalPlaylistStreamItemHolder etc. So we don't know Where we should define these two constants.

Thanks :)

Stypox commented 2 years ago

@Novmbrain all of the functions from util/AnimationUtils.java have been moved to ktx/View.kt. But I would not add constants to View.kt, as Kotlin would interpret them as fields in the View class. I would instead add them to the util/Constants.java class (there is already a time constant, DEFAULT_THROTTLE_TIMEOUT, defined there, so it makes sense in my opinion).