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

Convert AnimationUtils functions to extension functions. #5333

Merged

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Jan 2, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Convert the static methods defined in AnimationUtils to Kotlin extension functions.

APK testing

debug.zip

Due diligence

Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

Did you test everything that you changed? Since there are only very little unit tests, refactoring like this requires extensive manual testing.

@Isira-Seneviratne Isira-Seneviratne force-pushed the Convert_AnimationUtils_to_extensions branch from 2521e39 to 590db7a Compare January 9, 2021 09:20
@Isira-Seneviratne
Copy link
Member Author

Did you test everything that you changed? Since there are only very little unit tests, refactoring like this requires extensive manual testing.

I made sure to preserve the existing behavior.

Just to be sure, I tested it out and didn't find any issues.

@Isira-Seneviratne Isira-Seneviratne force-pushed the Convert_AnimationUtils_to_extensions branch 5 times, most recently from 33cc190 to f4dadf0 Compare January 16, 2021 03:58
Stypox
Stypox previously approved these changes Jan 16, 2021
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.

Looks good to me, thank you :-)

TobiGr
TobiGr previously requested changes Jan 16, 2021
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Small thing

@Stypox
Copy link
Member

Stypox commented Jan 16, 2021

Oh, then also TAG should be changed to = CurrentJavaClass.class.getSimpleName()

@Isira-Seneviratne Isira-Seneviratne force-pushed the Convert_AnimationUtils_to_extensions branch from a98c0de to 920e560 Compare January 16, 2021 09:19
@Isira-Seneviratne
Copy link
Member Author

Oh, then also TAG should be changed to = CurrentJavaClass.class.getSimpleName()

There doesn't seem to be a way to get a reference to the generated class for top-level declarations.

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. @TobiGr can this be merged?

@Stypox
Copy link
Member

Stypox commented Jan 16, 2021

I tested many places and everything seems to work well

@Stypox Stypox merged commit b73eb94 into TeamNewPipe:dev Jan 16, 2021
@Isira-Seneviratne Isira-Seneviratne deleted the Convert_AnimationUtils_to_extensions branch January 16, 2021 10:51
This was referenced Jan 18, 2021
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 20, 2021
…nvert_AnimationUtils_to_extensions

Convert AnimationUtils functions to extension functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants