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

Use view binding in activities. #4762

Merged

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Nov 2, 2020

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

APK testing

debug.zip

Due diligence

@Disqu3-mirOir-qt
Copy link

for me work , some bug fixed , perfect

i get only 1 error

@Disqu3-mirOir-qt

This comment has been minimized.

@Stypox
Copy link
Member

Stypox commented Nov 2, 2020

@Disqu3-mirOir-qt that error is not related to the contents of this PR

@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_view_binding_in_activities branch 3 times, most recently from fbd2162 to 6d89b22 Compare November 5, 2020 12:21
@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_view_binding_in_activities branch 3 times, most recently from f4047de to 56d971d Compare November 19, 2020 02:08
@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_view_binding_in_activities branch 6 times, most recently from d88be91 to 5cce950 Compare November 23, 2020 00:47
@Stypox
Copy link
Member

Stypox commented Dec 12, 2020

@TobiGr I'd like to merge this as soon as possible, otherwise @Isira-Seneviratne will be overwhelmed by rebasing. I will review and test tomorrow.
@Isira-Seneviratne could you rebase one last time? Thank you!

@Stypox Stypox mentioned this pull request Dec 12, 2020
5 tasks
@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_view_binding_in_activities branch 3 times, most recently from 26f53ba to 6e2a276 Compare December 16, 2020 01:01
@TacoTheDank
Copy link
Member

@Stypox It's been updated. Merging time! :)

Stypox
Stypox previously approved these changes Dec 17, 2020
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.

Code looks good to me, thank you! I tested on API 19 and on Android 7.0 and everything worked well. Could someone else review, before merging, though? I have no experience with view binding ;-) @TacoTheDank @TobiGr @vkay94

@TacoTheDank
Copy link
Member

TacoTheDank commented Dec 17, 2020

I have a little bit of experience with it. Did a quick test on my Android 10 device and everything seems to work as it should. Nice job Isira!

@vkay94
Copy link
Contributor

vkay94 commented Dec 17, 2020

I checked the "id moving" (discussed here in another view binding PR: #5029 (comment)), but it should be fine since the Id isn't used in the code :)

TobiGr
TobiGr previously approved these changes Dec 18, 2020
Copy link
Member

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

Thank you!
Code looks good to me. I could not find any bugs on Android 4..4, 5.0, 9 and 11 while testeing various scenarios.

It looks like @Stypox - how dare you :P - caused a merge conflict. Can you rebase one last time, please?

@TacoTheDank
Copy link
Member

It looks like @Stypox - how dare you :P - caused a merge conflict. Can you rebase one last time, please?

Lol stop torturing this poor guy!

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 again! :-D

@Stypox Stypox merged commit 75e34a5 into TeamNewPipe:dev Dec 19, 2020
@Isira-Seneviratne Isira-Seneviratne deleted the Use_view_binding_in_activities branch December 19, 2020 07:51
This was referenced Dec 21, 2020
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.

6 participants