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

Nullability Annotations to Java Classes - Activity & Fragment - Replace findViewById with ViewBinding #18906

Open
7 of 54 tasks
Tracked by #18905
ParaskP7 opened this issue Aug 7, 2023 · 9 comments
Open
7 of 54 tasks
Tracked by #18905
Assignees
Milestone

Comments

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 7, 2023

Parent #18905

This issue is about adding replacing findViewById(...) with ViewBinding to as many Java-related Activity & Fragment classes as possible.

Instead of adding missing nullability annotations (@Nullable & @NonNull) to layout View related fields on such Java-related Activity & Fragment classes, it is better to migrate those fields, from the old way of assigning those (using findViewById(...)), and into the new way of referencing such view (direct via ViewBinding.

FYI: You could reference #14845 to get an idea on how to go about that.


Activity

Tasks (WordPress + ui.main)

Preview Give feedback
  1. Core Tech Debt
    ParaskP7

Tasks (WordPress + ui.comments)

Preview Give feedback
  1. Core Tech Debt
    ParaskP7
  2. Core Tech Debt
    ParaskP7

Tasks (WordPress + ui.media)

Preview Give feedback

Tasks (WordPress + ui.notifications)

Preview Give feedback
  1. Core Tech Debt
    ParaskP7

Tasks (WordPress + ui.photopicker)

Preview Give feedback

Tasks (WordPress + ui.plugins)

Preview Give feedback

Tasks (WordPress + ui.posts)

Preview Give feedback

Tasks (WordPress + ui.publicize)

Preview Give feedback

Tasks (WordPress + ui.reader + post)

Preview Give feedback

Tasks (WordPress + ui.reader + user)

Preview Give feedback

Tasks (WordPress + ui.reader + other)

Preview Give feedback
  1. Core Tech Debt
    ParaskP7

Tasks (WordPress + ui.stockmedia)

Preview Give feedback

Tasks (WordPress + ui.themes)

Preview Give feedback

Tasks (WordPress + ui + webview)

Preview Give feedback

Tasks (WordPress + ui + other)

Preview Give feedback

Fragment

Tasks (libs/editor)

Preview Give feedback

Tasks (WordPress + ui.accounts)

Preview Give feedback

Tasks (WordPress + ui.comments)

Preview Give feedback
  1. Core Tech Debt
    ParaskP7

Tasks (WordPress + ui.history)

Preview Give feedback
  1. Core Tech Debt
    ParaskP7

Tasks (WordPress + ui.media)

Preview Give feedback

Tasks (WordPress + ui.people)

Preview Give feedback

Tasks (WordPress + ui.plugins)

Preview Give feedback

Tasks (WordPress + ui.posts)

Preview Give feedback

Tasks (WordPress + ui.publicize)

Preview Give feedback

Tasks (WordPress + ui.reader + fragment)

Preview Give feedback

Tasks (WordPress + ui.themes)

Preview Give feedback

Tasks (WordPress + ui)

Preview Give feedback

(❗) This exclamation mark is added to those activities and fragments that are quite complicated to migrate, and thus, maybe those should be left out from this specific attempt to replace findViewById with ViewBinding.

@ParaskP7 ParaskP7 added this to the Future milestone Aug 7, 2023
@peril-wordpress-mobile
Copy link

Fails
🚫

Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by 🚫 dangerJS

@ParaskP7
Copy link
Contributor Author

👋 @develric @khaykov and @RenanLukas !

I just started work on EditCommentActivity.java and noticed the deprecation on this class, plus its associated Javadoc, mentioning that if someone is adding any features or modifying this class should first reach out to you.

I also noticed the CommentDetailFragment.java class, which is driving the opening of the comment for editing (via the editComment() method), and wondering what's the current state with the UnifiedCommentsCommentEditFeatureConfig.kt feature flag. Can we drop it and its single usage here, wdyt? 🤔

FYI: This will help me cleanup and delete this EditCommentActivity.java class, that is, instead of replacing any findViewById usages with ViewBinding, or in general, doing any unnecessary nullability related work with it.

@RenanLukas
Copy link
Contributor

👋 Thanks for the ping, @ParaskP7. Unfortunately I don't have context on the deprecation of EditCommentActivity.java or the usage of UnifiedCommentsCommentEditFeatureConfig in CommentDetailFragment.java 🤔
I've found a PR where I made a change to open notification comments using the new comments editor, but the feature flag was already there.

Maybe someone else has context on this.

@khaykov
Copy link
Member

khaykov commented Sep 19, 2023

I think I can add some context, and maybe @develric can correct me If I got something wrong :)

Comment reworking was a pretty big projects, but only part of it was finished, so we have this "Unified" package for new stuff. We deprecated all the files we were planning to change, and left the comments so folks would not add any new functionality to them while we are working on them.

As far as I remember we only finished the comment list, and maybe had some work on comment editor (since we have UnifiedCommentsEditActivity). I would double check that we are not opening EditCommentActivity, and if not I would assume it's ok to delete it.

@develric
Copy link
Contributor

develric commented Sep 19, 2023

Hey @ParaskP7 👋 , I second what @khaykov said above. Also I think removing the UnifiedCommentsCommentEditFeatureConfig.kt makes sense as a cleanup/simplification. AFAIU you are going to work on this issue soon-ish, feel free to send PR(s) my way or let's align on slack if we need to plan for more consistently supporting this (we cannot commit on this soon-ish but I think having an overall cleaning up for those deprecated classes where they now have a replacement class would be good, I made a draft note on our team gh board for this since afaiu a deeper cleanup can also happen separately).

@ParaskP7
Copy link
Contributor Author

👋 @RenanLukas @khaykov @develric and thanks for the quick reply, you all rock! 🙇 ❤️

I second what @khaykov said above. Also I think removing the UnifiedCommentsCommentEditFeatureConfig.kt makes sense as a cleanup/simplification.

Great, this is what I thought as well. 👍

Let me then know if you are okay with me proceeding with removing this UnifiedCommentsCommentEditFeatureConfig.kt feature flag and the associated unused EditCommentActivity.java activity class, and then, I'll definitely start work on that and send a PR your way. I just need a formal thumbs-up on that. 😊 💯 😊

FYI: From the looks of it, in total, we have this @Deprecated annotation on the below 6 classes:

@develric
Copy link
Contributor

develric commented Sep 20, 2023

Let me then know if you are okay with me proceeding with removing this UnifiedCommentsCommentEditFeatureConfig.kt feature flag and the associated unused EditCommentActivity.java activity class...I just need a formal thumbs-up on that.

Sounds good 👍 and thanks 🙇

FYI: From the looks of it, in total, we have this @deprecated annotation on the below 6 classes:

I would need to look better into the code, but as you reported already some of them are still in use, so probably until we come back to them with a future project we should remove the deprecation and clean-up what's possible (like not more needed feature flags/code branches or the like where possible 🙇...that's possibly the scope of the draft gh issue I placed in the team gh board, unless something makes sense to cleanup anyway while you go over the tasks you are tackling here, we can decide while we go with your PRs, and if we are lucky we end up with a good clean-up already 🤞 )

@ParaskP7
Copy link
Contributor Author

Sounds good 👍 and thanks 🙇

Awesome, thanks for another 👍 on that @develric , I will then go ahead and start work on this soon-ish, that is, after finishing my current in-progress work on CommentDetailFragment.java! 🙇

I would need to look better into the code, but as you reported already some of them are still in use, so probably until we come back to them with a future project we should remove the deprecation and clean-up what's possible (like not more needed feature flags/code branches or the like where possible 🙇...that's possibly the scope of the draft gh issue I placed in the team gh board, unless something makes sense to cleanup anyway while you go over the tasks you are tackling here, we can decide while we go with your PRs, and if we are lucky we end up with a good clean-up already 🤞 )

👍 🤞 🙏

@ParaskP7
Copy link
Contributor Author

👋 @develric !

Awesome, thanks for another 👍 on that @develric , I will then go ahead and start work on this soon-ish, that is, after finishing my current in-progress work on CommentDetailFragment.java! 🙇

After finishing #19232, I went ahead and created this #19239 for you to review. 🙏

PS: Feel free to re-assign this on any other reviewer you deem more appropriate.

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

No branches or pull requests

4 participants