Kotlin Android Extensions & Butterknife are deprecated #4664
Comments
|
Welcome back, @psh ! :) Thanks for bringing this to our attention. It felt like only yesterday that we were making the switch TO Butterknife. :( I will ping @madhurgupta10 and @ashishkumar468 for input on this, however it makes sense to me that we should start the migration. We definitely want to do it in stages, with modular PRs if possible. Will let you know when it's OK to start. |
|
Using Jetpack ViewBinding and DataBinding seems like a good thing to do, IMO. There might be some code that still uses old And yes, this should definitely be done in multiple PRs otherwise would be very difficult to test and merge. This could also be a beginner-friendly task :) |
|
Sounds good, thanks @madhurgupta10 . Let's do it. :) We can probably split this up by package - 1 PR for files in 1 package? |
|
@misaochan Yes sure, even more than one PR per package would be okay IMO. I will create a PR with the necessary dependencies and an example so it is easier for first-time contributors to follow up :) |
|
Hi, I'd really like to help out with this migration as my first open-source contribution. Is there a package to which I should start applying the changes outlined in @madhurgupta10's example?
EDIT: Figured some stuff out. |
|
@ThomasLatham Feel free to open a new issue if you need help, one or more team members will be happy to help you out :) |
|
Thank you @madhurgupta10! I'll see what I can figure out on my own, but I won't hesitate to open a new issue if I get stuck |
|
https://android-developers.googleblog.com/2022/02/discontinuing-kotlin-synthetics-for-views.html?m=1 Google says -
Looking at the code, there are only a few classes using synthetics left. I'll prioritize getting those done. |
|
Hi All @psh @misaochan @madhurgupta10 @ThomasLatham @prtksxna , I have raised a PR for the same. Am I allowed to do it ? |
|
Hi @nicolas-raoul , Can you review the above PRs kindly? |
|
Hi @madhurgupta10 is this issue is solved. |
|
@bosankus Sorry for the delay, I have started reviewing but not finished yet. @glitched-sudhanshu If you find other classes that need the same, please list them here and you can start working on them 🙂 |
|
Hi @nicolas-raoul , there are other areas to cover too which am yet to push code in future for closing this ticket. Are you sure this issue can be closed now ? |
Hi, sorry for not checking it. I was also working towards it, thats why I like getting assigned first to avoid collission between two contributors. Nevermind, I will close the PR will do the code review on your's PR as I have hands on at the code already. |
No issues, also this issue is of such nature that it can't be assigned to one person (as discussed by mentors above) and Ideal way is to contribute 1 PR per package at our pace. Our goal here is to complete this task as a team, thanks and good luck |
|
Hey @shashankiitbhu, Can I also work on it together ;) |
|
@kanahia1 Sure, I am already working on the media package (almost done with it) so you can start on others that are left like campaigns Also Review Package in Progress (Made some changes last time in that too so I would like to finish Review Package as well) |
|
@kanahia1 These are still left and you can work on them if you want : app/src/main/java/fr/free/nrw/commons/notification/NotificationActivity.java I hope nothing is missed here 🥲 |
Taking it up |
|
Hey @neeldoshii, are you working on them ? |
|
@kanahia1 @neeldoshii Also If you get stuck somewhere (specially if your tests are failing) then take a look at one of my PRs (above PR for Review Package is something you can take reference from), it will save you a lot of time and trouble and do let me know if you need help somewhere |
yes. |
* Moved Main Activity and Settings Activity to ViewBinding * Moving only SettingsActivity for now * Removed values-yue-hant directory * Removing previously done changes to main.xml
|
Thanks for the heads-up - I missed the "Fixes ____" in the PR comment. With these long running issues, if you want to tag that it's part of the effort, it would be better to say "Relates to ____" and Github wont auto-close the associated issue! |
|
@nicolas-raoul as this task is completed (only some OPEN PRs need to be merged) , shouldn't this be removed from the Phabricator Proposal Page? |
|
Great work on this migration effort! I did a search in the code and it turned up a couple of cleanup steps still left to do. Looks like there is still a little migration to go (binding of views, and click listeners)
Cleanup unused butterknife imports in
Once those are done, we should be able to remove |
On it , will be sharing a PR soon |
|
@psh My PR above removes butterknife and completes migration. |
|
@neeldoshii your PR removed unused Imports from BookmarkLocationsFragment and MediaLicenceFragment, well no worries, My PR is focused on removing other Butterknife elements, you should have confirmed first but no issues. |
|
Sorry for the delay, I merged #5655 :-) |
Summary:
Looking at "The future of Kotlin Android Extensions" from 23 November 2020, (see: https://android-developers.googleblog.com/2020/11/the-future-of-kotlin-android-extensions.html)
According to Google (https://developer.android.com/topic/libraries/view-binding/migration)
In addition, the author of Butternife has stated (https://github.com/JakeWharton/butterknife)
Would you like to work on the issue?
It looks like a pretty big migration, with a large number of files that will be touched. A search for
import butterknife.BindViewshows 40 files, andimport kotlinx.android.synthetica further 16 files; probably too big for a single person and something to do in stages. I would be happy to be part of the process.The text was updated successfully, but these errors were encountered: