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

Standardize view animation duration #3038

Open
Stypox opened this issue Jan 30, 2020 · 11 comments
Open

Standardize view animation duration #3038

Stypox opened this issue Jan 30, 2020 · 11 comments
Labels
feature request Issue is related to a feature in the app good first issue Easy/simple issues perfect for newcomers to get involved in the project GUI Issue is related to the graphical user interface

Comments

@Stypox
Copy link
Member

Stypox commented Jan 30, 2020

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 #2309 (comment)

@Stypox Stypox added feature request Issue is related to a feature in the app good first issue Easy/simple issues perfect for newcomers to get involved in the project GUI Issue is related to the graphical user interface labels Jan 30, 2020
@ludugz
Copy link

ludugz commented Apr 12, 2020

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
Copy link
Contributor

wb9688 commented Apr 12, 2020

I vote for 420 ms 🤣

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
Copy link
Member Author

Stypox commented Apr 15, 2020

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
Copy link

ludugz commented Apr 16, 2020

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

@Novmbrain
Copy link

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
Copy link
Contributor

TobiGr commented Oct 26, 2021

@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
Copy link

@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
Copy link

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
Copy link
Member Author

Stypox commented Nov 13, 2021

@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).

@ChristoJobyAntony
Copy link

ChristoJobyAntony commented Oct 10, 2024

I can see that the previous PR has been closed without merge. If this issue is still active and accepting contributions, I would love to work on it.

@Stypox
Copy link
Member Author

Stypox commented Oct 11, 2024

@ChristoJobyAntony the UI is being rewritten and the View animation code is not going to be used anymore, so it may be better if you work on something else, e.g. #11603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app good first issue Easy/simple issues perfect for newcomers to get involved in the project GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants