[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All SDK Override Methods for Activities #18997

ParaskP7 · 2023-08-17T13:51:35Z

This PR adds any missing nullability annotation to all SDK override methods on Activity classes.

Note that the nullability information wasn't available on all of the super method signatures and thus I had to make a call on making a method's parameter and/or its return value either NonNull or Nullable. Thus, and in order to make the best call possible, I researched each such method separately (*), used ChatGPT to increase my confidence, and based my call on existing usages of such methods.

While doing so I also documented all those methods and grouped them per package, per library, see below:

Methods + Nullability

android
  app
    Activity.java (Class)
    - void onRestoreInstanceState(@NonNull Bundle savedInstanceState) {
    - boolean onCreateOptionsMenu(@NonNull Menu menu) { ... } (*)
    - boolean onPrepareOptionsMenu(@NonNull Menu menu) { ... } (*)
    - boolean onOptionsItemSelected(@NonNull MenuItem item) { ... }
    - boolean onMenuOpened(int featureId, @NonNull Menu menu) { ... } (*)
    - @Nullable Dialog onCreateDialog(int id) { ... } (*)
    DialogFragment.java (Class)
    - @NonNull Dialog onCreateDialog(@Nullable Bundle savedInstanceState) { ... } (*)
  view
    View.java (Class) -> OnClickListener (Interface)
    - void onClick(@NonNull View v) { ... } (*)
    View.java (Class) -> OnLongClickListener (Interface)
    - boolean onLongClick(@NonNull View v) { ... } (*)
    View.java (Class) -> OnScrollChangeListener (Interface)
    - void onScrollChange(@NonNull View v, int scrollX, int scrollY, int oldScrollX, int oldScrollY) { ... } (*)
    MenuItem.java (Interface) -> OnActionExpandListener (Interface)
    - boolean onMenuItemActionExpand(@NonNull MenuItem item) { ... }
    - boolean onMenuItemActionCollapse(@NonNull MenuItem item) { ... }
    animation
      Animation.java (Abstract Class) -> AnimationListener (Interface)
      - void onAnimationStart(@NonNull Animation animation) { ... } (*)
      - void onAnimationEnd(@NonNull Animation animation) { ... } (*)
      - void onAnimationRepeat(@NonNull Animation animation) { ... } (*)
  widget
    SeekBar.java (Class) -> OnSeekBarChangeListener (Interface)
    - void onStopTrackingTouch(@NonNull SeekBar seekBar) { ... } (*)
    - void onStartTrackingTouch(@NonNull SeekBar seekBar) { ... } (*)
    - void onProgressChanged(@NonNull SeekBar seekBar, int progress, boolean fromUser) { ... } (*)
  content
    DialogInterface.java (Interface) -> OnClickListener (Interface)
    - void onClick(@Nullable DialogInterface dialog, int which) { ... } (*)
    BroadcastReceiver.java (Abstract Class)
    - void onReceive(@NonNull Context context, @NonNull Intent intent) { ... } (*)
  text
   TextWatcher.java (Interface)
    - void beforeTextChanged(@NonNull CharSequence s, int start, int count, int after) { ... } (*)
    - void onTextChanged(@NonNull CharSequence s, int start, int before, int count) { ... } (*)
    - void afterTextChanged(@NonNull Editable s) { ... } (*) + (⚠️)
  webkit
    WebChromeClient.java (Class)
    - void onProgressChanged(@NonNull WebView view, int progress) { ... } (*)
androidx
  activity
    ComponentActivity.java (Class)
    - void onSaveInstanceState(@NonNull Bundle outState) { ... }
    - void onNewIntent(@NonNull Intent intent) { ... } (*) + (⚠️)
  fragment
    app
      FragmentActivity.java (Class)
      - void onCreate(@Nullable Bundle savedInstanceState) { ... }
      - void onActivityResult(int requestCode, int resultCode, @Nullable Intent data) { ... }
      - void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) { ... }
      DialogFragment.java (Class)
      - @NonNull Dialog onCreateDialog(@Nullable Bundle savedInstanceState) { ... } (*)
      FragmentPagerAdapter.java (Abstract Class)
      - @NonNull Fragment getItem(int position) { ... }
      - @NonNull Object instantiateItem(@NonNull ViewGroup container, int position) {
      FragmentStatePagerAdapter.java (Abstract Class)
      - @NonNull Object instantiateItem(@NonNull ViewGroup container, int position) { ... }
      - void destroyItem(@NonNull ViewGroup container, int position, @NonNull Object object) { ... }
      - void restoreState(@Nullable Parcelable state, @Nullable ClassLoader loader) { ... }
  appcompat
    widget
      SearchView.java (Class) -> OnQueryTextListener (Interface)
      - boolean onQueryTextSubmit(@NonNull String query) { ... } (*) + (⚠️)
      - boolean onQueryTextChange(@NonNull String newText) { ... } (*) + (⚠️)
    view
      ActionMode.java (Abstract Class) -> Callback (Interface)
      - boolean onCreateActionMode(@NonNull ActionMode mode, @NonNull Menu menu) { ... } (*)
      - boolean onPrepareActionMode(@NonNull ActionMode mode, @NonNull Menu menu) { ... } (*)
      - boolean onActionItemClicked(@NonNull ActionMode mode, @NonNull MenuItem item) { ... } (*)
      - void onDestroyActionMode(@NonNull ActionMode mode) { ... } (*)
  recyclerview
    widget
      RecyclerView.java (Class) -> Adapter<VH extends ViewHolder> (Abstract Class)
      - @NonNull VH onCreateViewHolder(@NonNull ViewGroup parent, int viewType) { ... }
      - void onBindViewHolder(@NonNull VH holder, int position) { ... }
  viewpager
    widget
      PagerAdapter.java (Abstract Class)
      - @Nullable CharSequence getPageTitle(int position) { ... }
com
  google
    android
      material
        tabs
          TabLayout.java (Class) -> BaseOnTabSelectedListener<T extends Tab> (Interface)
          - void onTabSelected(@NonNull TabLayout.Tab tab) { ... } (*)
          - void onTabUnselected(@NonNull TabLayout.Tab tab) { ... } (*)
          - void onTabReselected(@NonNull TabLayout.Tab tab) { ... } (*)
        appbar
          AppBarLayout.java (Class) -> OnOffsetChangedListener (Interface)
          - void onOffsetChanged(@Nullable AppBarLayout appBarLayout, int verticalOffset) { ... } (*)
        snackbar
          BaseTransientBottomBar.java (Abstract Class) -> BaseCallback<B> (Abstract Static Class)
          - void onDismissed(@Nullable Snackbar transientBottomBar, int event) { ... } (*)

Methods + Nullability to Triple Check

android.text.TextWatcher.java (Interface)
- void afterTextChanged(@NonNull Editable s) { ... } (*) + (⚠️)
androidx.activity.ComponentActivity.java (Class)
- void onNewIntent(@NonNull Intent intent) { ... } (*) + (⚠️)
androidx.appcompat.widget.SearchView.java (Class) -> OnQueryTextListener (Interface)
- boolean onQueryTextSubmit(@NonNull String query) { ... } (*) + (⚠️)
- boolean onQueryTextChange(@NonNull String newText) { ... } (*) + (⚠️)

(*): Nullability info not available on the super method signature and thus arbitrary added.
(⚠️): Better to double (triple) check that this @NonNull added won't cause us any trouble!

Nullability Annotation List:

Lint Warnings Suppression List:

Refactor List:

To test:

Smoke test both, the WordPress and Jetpack apps, and see if everything is working as expected.

Regression Notes

Potential unintended areas of impact
- On androidx.activity.ComponentActivity.java like changes where the Intent intent parameter got assigned a @NonNull annotation, and thus, any safe call got removed, a null-pointer exception might occur if this nullability annotation is incorrectly set, that is, instead of using @Nullable (see (*) + (⚠️) above).
What I did to test those areas of impact (or what existing automated tests I relied on)
- See To test section above.
What automated tests I added (or what prevented me from doing so)
- N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
Talkback.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
Large and small screen sizes. (Tablet and smaller phones)
Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

FYI: 'n-a' stands for 'nullability annotations'.

Warning Message: "Resource IDs will be non-final by default in Android Gradle Plugin version 8.0, avoid using them in const fields" Explanation: "Avoid the usage of resource IDs where constant expressions are required. A future version of the Android Gradle Plugin will generate R classes with non-constant IDs in order to improve the performance of incremental compilation." ------------------------------------------------------------------------ This Lint warning is suppressed, that is, instead of it being resolved, since a resolution would require a proper investigation. As such, it might be best to ignore this as out of scope, for now, and so as to not introduce any breaking changes with this upgrade overall. Related Commit: f8bc363

FYI: 'n-a' stands for 'nullability annotations'.

Warning Message: "Resource IDs will be non-final by default in Android Gradle Plugin version 8.0, avoid using them in const fields" Explanation: "Avoid the usage of resource IDs where constant expressions are required. A future version of the Android Gradle Plugin will generate R classes with non-constant IDs in order to improve the performance of incremental compilation." ------------------------------------------------------------------------ This Lint warning is suppressed, that is, instead of it being resolved, since a resolution would require a proper investigation. As such, it might be best to ignore this as out of scope, for now, and so as to not introduce any breaking changes with this upgrade overall. Related Commit: f8bc363

FYI: 'n-a' stands for 'nullability annotations'.

This is done so as to have the 'onActivityResult(...)' method match the super method signature.

This is done so as to have the 'onQueryTextSubmit(...)' method match the super method signature.

This is done so as to have the 'onQueryTextChange(...)' method match the super method signature.

This is done so as to have the 'onCreateActionMode(...)' method match the super method signature.

This is done so as to have the 'onPrepareActionMode(...)' method match the super method signature.

This is done so as to have the 'onActionItemClicked(...)' method match the super method signature.

This is done so as to have the 'onDestroyActionMode(...)' method match the super method signature.

This is done so as to have the 'onBindViewHolder(...)' method match the super method signature.

This is done so as to have the 'onNewIntent(...)' function for Kotlin files match the 'onNewIntent(...)' method for Java files. Otherwsie, half of those files were using 'intent' as nullable and the other half were using 'intent' as non null, which creates confusion.

This is done so as to have the 'onClick(...)' method match the super method signature.

wpmobilebot · 2023-08-17T14:08:05Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18997-e1dc640
	Commit	`e1dc640`
	Direct Download	`wordpress-prototype-build-pr18997-e1dc640.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-08-17T14:08:47Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18997-e1dc640
	Commit	`e1dc640`
	Direct Download	`jetpack-prototype-build-pr18997-e1dc640.apk`

Note: Google Login is not supported on these builds.

… into analysis/add-missing-nullability-annotations-to-all-sdk-override-methods-for-activities # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

mkevins

Hi Petros 👋 😄, thank you for all the work you've done to analyze and catalog the nullness through our Java code pertaining to Activity methods!

I left a few comments where I think we should be a bit careful about assumptions. In particular, I think we should try to avoid refactoring to a "fail-fast" mode when the existing code already has some robust guard for the @Nullable case. This is because, I would rather avoid introducing any new crashes with this work.

That said, much of the work in this PR is actually primarily concerned with compile time correctness, and it'd be ideal, imo, to separate these kinds of changes from those which can affect the behavior of the running app. This would have several benefits:

It will allow us to make smaller PRs, reducing the barrier to contribution by reviewers
It enables some PRs to be reviewable and testable without smoke tests (e.g. when the changes only affect compile time)
It isolates changes that have potential to introduce crashes or bugs (when nullness is ambiguous from un-annotated framework or library methods)

Also, the additional effort to refactor various nearby parts of the code is commendable! But, I wonder, too, if this should be contributed as part of a separate PR, since it seems that many of these improvements will not affect behavior of the running app. If it is the case that some crash is introduced by making a wrong guess (e.g. annotating @NotNull when it should have been @Nullable, etc.), it would be great to be able to revert the PR that introduced it without losing all of the other improvements.

Fwiw, I see that you have very neatly organized the commits, so, in theory, such a revert could still be applied on a commit by commit basis. However, whatever discussion that may surround such cases may be deserving of it's own PR.

In summary, I propose to split this (and similar) PRs by the following three categories:

Analysis PRs which only affect compile time 👈 these are the low-hanging fruit, imo
Analysis PRs which can affect behavior of the running app (more scrutiny needed)
Refactor PRs which address various formatting issues, other lint rule violations, etc.

Wdyt?

WordPress/src/main/java/org/wordpress/android/ui/AddQuickPressShortcutActivity.java

mkevins · 2023-08-24T04:09:10Z

WordPress/src/main/java/org/wordpress/android/ui/AppLogViewerActivity.java

        super.onCreateOptionsMenu(menu);
        MenuInflater inflater = getMenuInflater();
        inflater.inflate(R.menu.app_log_viewer_menu, menu);
        return true;
    }

    @Override
-    public boolean onOptionsItemSelected(final MenuItem item) {


I don't feel strongly about this, but typically, final is used to prevent reassignment of the variable within the method. In this case, the method is rather simple, and item is only even used in the default call to the super method. But final parameters have no effect at the call site, so I'd generally be ok with keeping it - but in this case, since the method is very simple anyway, I'm also fine with removing it. 👍

The only reason I removed this final keyword is because I wanted every such method across the codebase to be consistent with each other.

Thus, and because I found very few occurrences of such final usage on such onOptionsItemSelected(...) methods across the whole codebase, that it is better to remove it here as well, rather than adding it everywhere else. No matter, I think it is better for us to use a consistent signature for such methods. Wdyt? 🤔

PS: Also, and since we don't have a documented way of using final, and since I rarely seen it being used, I think it is better to avoid adding it unnecessarily everywhere. Thus, I decided it might be better to remove it instead, whenever found on those @Override methods.

I think it is better for us to use a consistent signature for such methods

I agree with this, but my point above is that final is not really part of the method signature. That's what I meant by "no effect at the call site".

Since, in this instance, the usage is for such a simple method, I think it's ok to remove it. Generally speaking, though, the intention for using this is entirely compatible with keeping signatures consistent. final on a parameter only pertains to the usage of that reference within the method body, and is sometimes used to prevent reassignment of the variable. This can be a useful restriction in long / complex methods.

On this same topic, there are many parameter renaming changes, in which the commit message mentions this was done for the sake of consistent method signatures, but parameter names are not part of a methods signature in Java. I didn't see any issue with the renamings that you did, but was curious to know your motivation. Does it relate somehow to Kotlin?

To play devils advocate, I could envision deviating from the overriden super class parameter names for clarity's sake - to I don't think we should have a rule that all parameter names must match that of the super class. Especially because some super methods have inappropriately terse parameter names.

I agree with this, but my point above is that final is not really part of the method signature. That's what I meant by "no effect at the call site".

👍

Generally speaking, though, the intention for using this is entirely compatible with keeping signatures consistent.

I agree with all your points @mkevins and apologies for the confusion here. What I meant with keeping signatures consistent wan't related to super vs implementing signature, but rather project wise, that is, so that when one is searching/copy-pasting such methods, that they are not seeing a mixture of both, some of them using a final, while others are not. Now, I agree that the usage of final is a case-by-case scenario, but on those specific cases I doubt that it was done like that (an assumption from my side). Instead, I think this was initial done purely because it is a best practice to use final as much and as widely as possible, and then, that was blindly copy-pasted thereafter. However, since us using final vs not using it at all hasn't been discussed, or agreed upon, and since me seeing and experiencing us not using final during our day-to-day work with Java, that is, unless the compiler complains or warns us about it, I was thinking it is better to remove those usages as well. This way, IMHO, the next person to copy-paste such method would not unnecessarily copy-paste the final as well, even if that means safer code... 🤷

The other way around for me would have been to add the final to every such @Override method. 🤔

Having said all the above, it is easier for me to plain ignore such changes if this will make it easier for you to review. I am only doing those to increase the consistency and make sure that we are using such @Override method consistently across the codebase, just to have make us that bit more consistent. But, I can stop doing that if that makes you uncomfortable as removing final is indeed less safe that having it there, or adding it everywhere missing, even if someone just copy-pasted that there from a previous such existing activity. 🤷

mkevins · 2023-08-24T05:26:02Z

WordPress/src/main/java/org/wordpress/android/ui/comments/EditCommentActivity.java

@@ -98,8 +99,8 @@ public void handleOnBackPressed() {

        loadComment(getIntent());

-        if (icicle != null) {
-            if (icicle.getBoolean(ARG_CANCEL_EDITING_COMMENT_DIALOG_VISIBLE, false)) {


I had to look this up 😅 .. this is a throwback to pre Android 1.0 🤯 , when onSavedInstanceState was called onFreeze (release notes). That said, I'm fine with renaming this. 🫡 farewell, icicle.

Yeaaa... 🤣 Bye, bye icicle and thanks for all the fish! 🎣

mkevins · 2023-08-24T05:33:47Z

WordPress/src/main/java/org/wordpress/android/ui/comments/EditCommentActivity.java

                boolean hasError = (editContent.getError() != null);
-                boolean hasText = (s != null && s.length() > 0);


I agree with your estimation that this probably shouldn't ever be null, but this line makes me wonder...

also, there are a few cases with null guards in the TextView source.. so maybe it's safer to assume nullable? Wdyt?

I agree with your estimation that this probably shouldn't ever be null, but this line makes me wonder...

I think so too... 🤞

also, there are a few cases with null guards in the TextView source.. so maybe it's safer to assume nullable?

Can you help me connect the dots of the null guards you shared in the TextView source and this @NonNull Editable s, I honestly fail to find a quick connection, but, I am also a bit rusty with everything TextView, EditText and TextWatcher related, so please forgive me... 😊

Can you help me connect the dots of the null guards you shared

I didn't connect any dots myself, just noted that there were several guards throughout that file where the underlying text (among other things) could be null. That said, it might be the case that it is safe to consider it @NonNull, (considering things like this, for example). It is unfortunate that this was left unannotated in the docs, but my feeling is that it is probably safe. Still, I'd prefer that if we remove a guard, we do it in a separate PR so that we can attach an explicit crash monitoring task, to be proactive in observing the effect in production.

mkevins · 2023-08-24T05:39:47Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanActivity.kt

@@ -31,11 +31,9 @@ class ScanActivity : AppCompatActivity(), ScrollableViewInitializedListener {

    private var binding: ScanActivityBinding? = null

-    override fun onNewIntent(intent: Intent?) {


This is another case where there is a suppression on UnknownNullness in the framework source 🤷‍♂️ . I wonder if it is safer to leave it as @Nullable? Also, strangely, I'm seeing this error:

Overriding method should call `super.onNewIntent`

but, I'm not sure if it is related with the signature 🤔

This is another case where there is a suppression on UnknownNullness in the framework source 🤷‍♂️ .

Yea, spot-on, I also researched the codebase and saw half of those intent being used as nullable, while other half as non-null (Java and Kotlin files included). Them being already used as @NonNull made me take the risky decision to mark them all as @NonNull, making this signature consistent across the whole codebase. 🤞

I wonder if it is safer to leave it as @nullable?

Obviously it is safer to leave it and change everything else as @Nullable, but I wonder if it is better to "fail fast" or "fail silently"... I am usually on the "fail fast" side. 🤷

Also, strangely, I'm seeing this error:

Overriding method should call `super.onNewIntent`

but, I'm not sure if it is related with the signature 🤔

About this warning, I've also noticed it, but this is unrelated to the signature problem as for the life of me I could get rid of it no matter what I tried. PS: I am also founding this to happen occasionally on other such methods as well. 🤔

I wonder if it is better to "fail fast" or "fail silently"... I am usually on the "fail fast" side.

I'm usually on the "fail faster" side, which, to me, means failing at compile time. I don't think that assigning @Nullable will be "fail silently", since the compiler would fail to even build the binary.

That said, the wide usage (half, as you mentioned) of this as @NonNull is a good indication that this is likely safe.. but as I've mentioned before, I think changes that could introduce a crash should be on a separate PR so that we could also have an associated crash monitoring task. Then, the other improvements (that could not possibly introduce a crash) would be unblocked.

I agree on having a separate PR @mkevins ! 👍

PS: With "fail silently" here I didn't mean at compile time, or even at runtime. What I meant is that we would have branches of code that are doing nothing, and then, us developers thinking that those branches are active, we would unnecessary maintain and develop on top of those. I am sorry to use "fail silently" and "fail fast" to mean various things, and all at once, I just didn't want to introduce "something else" to avoid making it harder to follow this discussion.

Thanks for the explanation. 👍 I agree that if we wrongly annotate something as @Nullable and then add (or even keep) guards in place, that those conditional branches might be (and have always been) dead code.

mkevins · 2023-08-24T05:43:53Z

WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java

@@ -1311,7 +1311,7 @@ private void setSite(Intent data) {

    @Override
    @SuppressWarnings("deprecation")
-    public void onActivityResult(int requestCode, int resultCode, Intent data) {
+    protected void onActivityResult(int requestCode, int resultCode, @Nullable Intent data) {


This should be fine, especially if the compiler does not complain. I'm wondering how this came to be, since typically when you autocomplete an override, it will use the same access modifier as the super method 🤔 . Reseting it makes sense, since if there was a need for this to be public, the build would fail. 👍

Reseting it makes sense, since if there was a need for this to be public, the build would fail. 👍

Exactly, and again, I am going for consistency here and making this safe minor changes seems easy enough for the better they produce, not having us to worry again about why the protected was/is used. 👍

mkevins · 2023-08-24T05:55:47Z

WordPress/src/main/java/org/wordpress/android/ui/media/MediaSettingsActivity.java

-            public void onOffsetChanged(AppBarLayout appBarLayout, int verticalOffset) {
-                if (mScrollRange == -1) {
+            public void onOffsetChanged(@Nullable AppBarLayout appBarLayout, int verticalOffset) {
+                if (mScrollRange == -1 && appBarLayout != null) {


Just curious, should anything be added to an else block when appBarLayout is null? I wonder, since, if appBarLayout is null, and it is the container for collapsingToolbar, would that also imply that collapsingToolbar is null, or perhaps in some invalid state? 🤔

Great question and my short answer here is; I am not sure. I didn't want to invest more time into going deeper than than. However, I think this question of yours about an invalid state makes a case about being in favor of the "fail fast" case. Otherwise, going the "fail silently" road, we might introduce invalid states that are going to be much harder to identify going forward. 🤷 🤔

I think the only "fail silently" part about this is the addition of the appBarLayout != null, which is a quick way to make the compile error go away, but not necessarily a "valid" thing to do. To me, I think of @Nullable as failing at compile time, and @NonNull as failing at runtime, and thus @Nullable is failing faster, since it fails before even reaching users.

I think the conflation here is that additional changes are being introduced, alongside the application of the @Nullable annotation, to satisfy the compiler (or make the red things go away in the IDE 😄). But, in many cases, significant effort is required to do this correctly, as well as a deeper understanding of the implementation in question.

This is another reason I think these kinds of changes should be postponed to a different PR. The effort to review these may vary greatly from case to case, and it would be ideal to have input from folks more familiar with a given implementation (especially regarding the correct handing of the @Nullable cases).

To me, I think of @nullable as failing at compile time, and @nonnull as failing at runtime, and thus @nullable is failing faster, since it fails before even reaching users.

Hmmm... yea @mkevins , I get that but let me provide an extra thing that @Nullable is to me. In addition to it failing at compile time, and thus failing "faster", if used incorrectly, it is now actually failing "silently" as now we are introducing a whole application flow and could never be valid, or an application state that is ignored, which, in its turn will propagate this downstream everywhere else. Plus, doing that, we are now giving the wrong example for others to follow.

Thus, to me, using @NonNull is already falling "faster" vs using @Nullable as I consider that you could never fail with that anyway, you could only "fail" during compile time, but I personally don't consider that failing, I only consider failing a runtime thing. 🤔

It seems I should clarify what I mean by @Nullable failing. I don't mean that it is failing in the sense that we add dead code unnecessarily. I mean that the build will fail, because the compiler will not let us dereference a pointer to memory that is annotated this way.

I only consider failing a runtime thing

To me, I have a strong preference that as many failures (or errors, etc.) are surfaced at compile time as possible. The preference of where a "failure" should be surfaced is: [runtime] < [compile time] < [lint].

if used incorrectly, it is now actually failing "silently" as now we are introducing a whole application flow and could never be valid, or an application state that is ignored, which, in its turn will propagate this downstream everywhere else. Plus, doing that, we are now giving the wrong example for others to follow.

I don't think having wrongly applied @Nullable annotations would lead to an application state which is ignored or invalid, but rather that it may make the compile complain until non-existent cases are handled (i.e. the compile will force the writing of dead code). I'm also not worried for the propagation of this because we have things like !! and Object.requireNotNull (in Kotlin and Java, respectively), which give us the power to decide how far along the codepaths we want to provide null-safety for an uncertain nullness. We do not have an equivalent mechanism for @NonNull annotations. Once annotated as such, the NPE will occur anywhere this wrong assumption leads. I.e., user crashes that are trickier to track down, because the originally incorrect annotation is not the site of the crash.

Another point to consider is that "propagation" is going to happen with either of the annotations. For example, annotating a return type as @Nullable requires any instances, fields, other return types, etc. that consume such a method be annotated similarly, while annotating a parameter with @NonNull similarly requires at the call site that any arguments passed are non-null. In either case, in order for the code to remain safe and valid, the developer would need to provide some handling of the potentially null case before usage, with the choices being primarily conditionally / gracefully failing, or asserting and crashing. It is, of course, easier to just crash, but users tend not to like that 😅 . But handling these cases gracefully requires an understanding of the domain - a scope far to large to handle in bulk, imo.

mkevins · 2023-08-24T06:06:25Z

WordPress/src/main/java/org/wordpress/android/ui/plugins/PluginBrowserActivity.java

+    public boolean onQueryTextChange(@NonNull String newText) {
+        mViewModel.setSearchQuery(newText);


Similar to the comment about afterTextChanged, I wonder whether this is safe to do? I'm especially curious because the query != null ternary guard falls back to an empty string 🤔 - if this has survived a decade in production, do we risk introducing a "fail-fast crash" to replace the empty string fallback? I.e. is the existing implementation already more robust?

Yea, I know, me looking at the change (05f5add) that introduced this didn't give me enough evidence of its need, thus me being more bold on that, and again me basing this change on all other usages of it across the whole codebase (Java and Kotlin included, and especially Kotlin, where it is solely used as non-null, without the extra ?). 🤞

Having said that, and as per all the discussion we are having on this, "fail fast" vs "being safe", I wonder what is best, to keep having something there that might never be true, thus us needed to create a fallback, but then risking having every other client code that using this onQueryTextChange(...) think it being potentially falsely @Nullable, or risk a potential very low chance of a quickly revertible crash, especially since testing verified that this is indeed an empty string ("") instead of it being null when the search result is empty... 🤷

PS: I personally think that we can just quickly smoke test this and all those cases and be better of having a @NonNull annotation, then spread that across the codebase, than have a @Nullable one and spread that. No matter, we should either should one or the other, I wouldn't be comfortable having a mix of those and thus ending up having an inconsistent signature for other to replicate inconsistently as well. Wdyt? 🤔

The evidence you mention is compelling, imo, i.e. that this is already being treated as @NonNull throughout the codebase and without issue. So, I agree with your rationale for consistently annotating it as such. That said, like the others, I think this could be on another PR with dedicated crash monitoring. My biggest concern is that we introduce a new crash and negatively affect the user experience. If we are very confident we can avoid that (from the recognition of wide usage already, along with scrutiny during beta testing), I think it's ok to annotate as @NonNull.

I agree on having a separate PR @mkevins ! 👍

mkevins · 2023-08-24T06:21:15Z

WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderPhotoViewerActivity.java

@@ -279,6 +281,7 @@ public void restoreState(Parcelable state, ClassLoader loader) {
            }
        }

+        @NonNull


Since newInstance is analyzed and confirmed to be @NotNull, I wonder if the annotations should be applied there as well. Wdyt?

Ah yea, I can do that, nice catch, but then again, I wonder how deep I should go with that? 🤔

FYI: I have arbitrarily (usually) stopped at the first level, when working on this PR, with the plan being that this annotations at this level will then help with other follow-up PRs where will be focusing on a case-by-case scenario, like adding missing annotations to ReaderPhotoViewerFragment.java or any other strategy we chose to follow going forward.

I wonder how deep I should go with that?

I had assumed (perhaps wrongly so 😅), that you already did analyze the return value of this static factory method. It reads like a constructor, but unfortunately, unlike a constructor, does not have any guarantees about the return type being @NonNull. But, a quick look at the method suggests so, and I thought that is how you came to annotate this as such.

But reflecting on this now, I see that it is coming from the other way around, that you annotated this return type as @NotNull, to match the super type signature. I think this is fine, since we already know it was working (and we also know it can't be null). When I reviewed this, I checked the return type of the newInstance call to verify it was appropriately @NonNull, so in this case it would also be good to annotate that as well, but feel free to defer to another PR for that.

Yea @mkevins , these kind of PRs and work, unless being discussed and agreed ahead of time, it is hard to make assumptions on how one would approach such work, or how was something that is already completed have been built to completion and why. 🤷

When I reviewed this, I checked the return type of the newInstance call to verify it was appropriately @nonnull, so in this case it would also be good to annotate that as well, but feel free to defer to another PR for that.

As explained above, I will indeed prefer to defer that to another PR just to have this or anything other such super type signature method matching PR progress faster, both author and review wise. 👍

ParaskP7 · 2023-08-24T10:47:35Z

👋 @mkevins and thank you so much for the detailed comment and for sharing your thoughts and suggestions, this is very much appreciated! 🥇 💯 🙇

I left a few comments where I think we should be a bit careful about assumptions. In particular, I think we should try to avoid refactoring to a "fail-fast" mode when the existing code already has some robust guard for the @nullable case. This is because, I would rather avoid introducing any new crashes with this work.

I was thinking about it too, and after some thinking, plus me usually being more on the "fail-fast" side, I decided to move forward with being more risky than staying on the safe side and having an unnecessary @Nullable annotation when @NonNull would suffice.

The reason why I made this decision is the fact that this change will not end-up being a one-of change for a specific method, but will then apply to every such usage across the whole codebase. Thus, if we end-up taking the safe side, you are risking writing unnecessary guard code going forward everywhere, people will start rightfully copy-pasting it, which would then propagate on other places too.

My bet is that even if we end-up being wrong on a @NonNull annotation, we will be able to quick identify and fix this NPE crash during the beta testing period, worse case scenario, a quick hotfix.

Us trying to avoid introducing any new crashed with this work might be efficient in terms of increasing the crash free rate of JP/WPAndroid, but, would not be the most efficient in terms of adding the most appropriate nullability annotation possible.

I am thinking about it this way, us being on the safe side and thus making this the most efficient NPE project, we might as well add @Nullable everywhere possible, that is, when we luck concrete evidence of the opposite. But this would mean that after us adding this @Nullable annotation, the chances that this will get changed to @NonNull at some point will end-up being extremely low, that is, comparing that to not having any nullability annotation, and of course comparing that to being a bit more risky and using @NonNull first.

That said, much of the work in this PR is actually primarily concerned with compile time correctness, and it'd be ideal, imo, to separate these kinds of changes from those which can affect the behavior of the running app. This would have several benefits:

It will allow us to make smaller PRs, reducing the barrier to contribution by reviewers

It enables some PRs to be reviewable and testable without smoke tests (e.g. when the changes only affect compile time)

It isolates changes that have potential to introduce crashes or bugs (when nullness is ambiguous from un-annotated framework or library methods)

I agree with this idea and I like it, having PRs that are compile-time focused, that is, versus having PRs that have the potential to introduce crashes or bugs (example here). 👍

Having said that I am still on the more risky side of creating such PRs, even if they have the potential of introducing crashes or bugs. Else, I would suggest we just ignore creating any PRs, that is, instead creating PRs that are adding a @Nullable annotations, just because we don't have enough information about the opposite. IMHO, It is better to not have any annotation than having an unnecessary @Nullable annotation staying in our codebase forever and then become converted as such to Kotlin (later on).

Also, the additional effort to refactor various nearby parts of the code is commendable! But, I wonder, too, if this should be contributed as part of a separate PR, since it seems that many of these improvements will not affect behavior of the running app.

Please let me respectfully disagree on that, I consider every refactor commit a safe commit and I would like to keep adding those to increase the consistency and coherence of our codebase when I see fit, that is, without the need for a separate PR, IMHO a separate commit should suffice. Thus, when I create such change, I usually separate them from other commits (something that admirably wasn't done on this other PR, for a different reason, mainly due to that fact that I wanted to avoid creating such extra, extremely safe, commits, example here, a separate commit renaming the view to v vs. here where it was done as part of adding the @NonNull annotation, all in one commit).

If it is the case that some crash is introduced by making a wrong guess (e.g. annotating @NotNull when it should have been @nullable, etc.), it would be great to be able to revert the PR that introduced it without losing all of the other improvements.

Fwiw, I see that you have very neatly organized the commits, so, in theory, such a revert could still be applied on a commit by commit basis. However, whatever discussion that may surround such cases may be deserving of it's own PR.

Yes, with having such commits, reverting could be applied on the commit level and not PR level. Having said that, I am not sure if you would prefer having one PR with one commit instead, such to keep the discussion specific to a single method that is wrongly annotated with @NonNull? 🤔

The reason I didn't go for multiple PRs like that is because I fear that doing so will explore the scope of this project and it will take us a huge amount of time dealing with every singe such method (then field, etc). But, for sure we will be a bit more safe, I agree. I decide to go for a more batch approach, just to be a bit more efficient, but we can adjust accordingly... 🤷

In summary, I propose to split this (and similar) PRs by the following three categories:

Analysis PRs which only affect compile time 👈 these are the low-hanging fruit, imo

Analysis PRs which can affect behavior of the running app (more scrutiny needed)

Refactor PRs which address various formatting issues, other lint rule violations, etc.

I think I agree on (1.) + (2.), but I wouldn't go for (3.) as separate PRs, instead I would add such refactor commit as I go and have them be part of any of the major (1.) + (2.) PRs that we'll be working from now on (as explained above).

Btw, why would you have (3.) as separate PRs, assuming that those changes are extra minor and safe, can you explain your reasoning for suggesting that again? 🤔

PS: I am used to add such extra refactor commits for many years now, including when working on feature PRs, where I was using feature commits for the main work and separating this work from any other commit. I find that engineers either ignore such changes, or make them part of feature commits, but I rarely seen engineers go back and create accompanying refactor PRs to cover for that, after the fact, nor I find that efficient when that happens, just to be very honest here.

Having discussed the above, would you like me to now close this PR, after replying to your other comments and then:

Based on (1.), to create a PR that only affects compile-time, which basically means me avoiding adding any @NonNull annotation to those methods that are missing this information in their super method signature, thus avoid making any other related change, but still add any @Nullable annotation to those methods that are missing this information in their super method signature? Or, do you suggest me avoid adding any nullability annotation whatsoever to all those methods that are missing this information in their super method signature? 🤔
Based on (2.), to create multiple and separate PRs for every method that is missing this information in their super method signature, especially for those that @NonNull annotation is to be added, the risky path? Also, maybe add one PR for every method that is missing this information in their super method signature, but a @Nullable annotation is to be added, the safe path? 🤔

I also have another suggestion for you, for me to create separate PRs per @Override method (vertical approach), instead of doing this work for all Java related Activities/Fragment at once (horizontal approach), wdyt? Then, I could categorize each of these PRs as them being safe/compile time (1.) or else them being risky/behavior affecting (2.), depending on which nullability annotation is added to each and for what reason. Wdyt? 🤔

Again, many thanks for opening this discussion and for sharing all your thoughts, much appreciated! 🙇

mkevins · 2023-08-25T03:41:26Z

I was thinking about it too, and after some thinking, plus me usually being more on the "fail-fast" side, I decided to move forward with being more risky than staying on the safe side and having an unnecessary @Nullable annotation when @NonNull would suffice.

The reason why I made this decision is the fact that this change will not end-up being a one-of change for a specific method, but will then apply to every such usage across the whole codebase. Thus, if we end-up taking the safe side, you are risking writing unnecessary guard code going forward everywhere, people will start rightfully copy-pasting it, which would then propagate on other places too.

My bet is that even if we end-up being wrong on a @NonNull annotation, we will be able to quick identify and fix this NPE crash during the beta testing period, worse case scenario, a quick hotfix.

I understand your rationale, and I don't disagree, in principle, especially for cases where we are already treating the types as @NonNull without issue throughout our code. But, I think we should classify the risk distinctly from annotation usage which can only reduce the possibility of crashes.

Us trying to avoid introducing any new crashed with this work might be efficient in terms of increasing the crash free rate of JP/WPAndroid, but, would not be the most efficient in terms of adding the most appropriate nullability annotation possible.

I agree with this point, and this may be another way of looking at the split in PRs that I suggested - to separate the concerns. Changes that could (however unlikely we estimate) introduce a new crash should, imo, be reviewed / followed up on, etc. differently from those changes which cannot possibly introduce a new crash, and could only possibly improve the developer and user experience. Those in the latter category make for an easy PR review, since CI is the main testing step.

I am thinking about it this way, us being on the safe side and thus making this the most efficient NPE project, we might as well add @Nullable everywhere possible, that is, when we luck concrete evidence of the opposite. But this would mean that after us adding this @Nullable annotation, the chances that this will get changed to @NonNull at some point will end-up being extremely low, that is, comparing that to not having any nullability annotation, and of course comparing that to being a bit more risky and using @NonNull first.

Having said that I am still on the more risky side of creating such PRs, even if they have the potential of introducing crashes or bugs. Else, I would suggest we just ignore creating any PRs, that is, instead creating PRs that are adding a @Nullable annotations, just because we don't have enough information about the opposite. IMHO, It is better to not have any annotation than having an unnecessary @Nullable annotation staying in our codebase forever and then become converted as such to Kotlin (later on).

I think it might help if I clarify what I'm envisioning with regard to this. I see this effort as something that will be approached gradually, and what the lint rule will help to achieve after raising sufficient awareness of the underlying issue (that is, nullness ambiguity). To me, this ambiguity is costly, and addressing it is also costly:

Annotation	Developer Impact	User Impact
`@Nullable`	Work required to ensure validity	No crashes, and nice experience
`@NonNull`	Trivial to add	Potential crashes, degraded experience
None	Combination of above, case by case	Combination of above

As a matter of principle, I value the user experience above the developer experience, so avoiding crashes or undefined behavior is a high priority, imo. That said, I believe you have a valid point about the reduced risk of crashes when we've already been treating certain cases as @NonNull in an ad hoc fashion.

I noticed that you mentioned the idea in a few places that there is a trade-off between "fail fast" and "fail silently", and that @NonNull is associated with the former, and @Nullable the latter. I want to interject on this point, because I think there is a big difference in how we envision @Nullable to be applied. Personally, I think adding @Nullable requires significantly more effort on the part of the developer, unless the existing code already gracefully and appropriately handles the null cases. I do not think that @Nullable should ever be added without appropriately walking the downstream codepaths to ensure validity (i.e. not just adding a null guard on a conditional, without a thought about what this does to the logical state of the running code). @NonNull, on the other hand, is trivial to apply from the developer side. Instead, it exposes users to potential issues / crashes.

With this in mind, I still agree with your sentiments about inferring @NonNull from wide usage without NPEs. I also think that @Nullable is not preferred in a bulk PR - not because of "silent failure", but rather because I don't think this should be done without at least familiarity with the underlying feature and implementation.

Also, the additional effort to refactor various nearby parts of the code is commendable! But, I wonder, too, if this should be contributed as part of a separate PR, since it seems that many of these improvements will not affect behavior of the running app.

Please let me respectfully disagree on that, I consider every refactor commit a safe commit and I would like to keep adding those to increase the consistency and coherence of our codebase when I see fit, that is, without the need for a separate PR, IMHO a separate commit should suffice.

I'm not sure what you "disagree" with, as I wasn't stating an opinion, but rather the opposite - I am unsure where these commits belong. But on thinking about it further, I think these commits can be separated in the same way as the others. If they won't cause runtime issues, or if they might. Fwiw, a quick look across all the "refactor" commits, so far this is the only one that could introduce runtime issues.

Yes, with having such commits, reverting could be applied on the commit level and not PR level. Having said that, I am not sure if you would prefer having one PR with one commit instead, such to keep the discussion specific to a single method that is wrongly annotated with @NonNull? 🤔

The reason I didn't go for multiple PRs like that is because I fear that doing so will explore the scope of this project and it will take us a huge amount of time dealing with every singe such method (then field, etc). But, for sure we will be a bit more safe, I agree. I decide to go for a more batch approach, just to be a bit more efficient, but we can adjust accordingly... 🤷

I think it's ok to batch, especially since you have very nicely organized the commits to make reverts easy.

I think I agree on (1.) + (2.), but I wouldn't go for (3.) as separate PRs, instead I would add such refactor commit as I go and have them be part of any of the major (1.) + (2.) PRs that we'll be working from now on (as explained above).

Btw, why would you have (3.) as separate PRs, assuming that those changes are extra minor and safe, can you explain your reasoning for suggesting that again? 🤔

You're right about 3, it can be split up by the same criteria and added to 1 and 2.

Having discussed the above, would you like me to now close this PR, after replying to your other comments and then:

Maybe it'd be best to "convert" this PR into 1, and spin up another for 2? Wdyt?

Based on (1.), to create a PR that only affects compile-time, which basically means me avoiding adding any @NonNull annotation to those methods that are missing this information in their super method signature, thus avoid making any other related change, but still add any @Nullable annotation to those methods that are missing this information in their super method signature? Or, do you suggest me avoid adding any nullability annotation whatsoever to all those methods that are missing this information in their super method signature? 🤔

I would suggest for this "1" PR that we add as many annotations as possible, including annotations which are not present on a super method signature (by inferring as you have already done), with the exception that if such annotations require you to change any other code (removing or adding a guard, etc.), those should be omitted. Essentially, the compiled output should remain unchanged by this PR.

Based on (2.), to create multiple and separate PRs for every method that is missing this information in their super method signature, especially for those that @NonNull annotation is to be added, the risky path? Also, maybe add one PR for every method that is missing this information in their super method signature, but a @Nullable annotation is to be added, the safe path? 🤔

I also have another suggestion for you, for me to create separate PRs per @Override method (vertical approach), instead of doing this work for all Java related Activities/Fragment at once (horizontal approach), wdyt? Then, I could categorize each of these PRs as them being safe/compile time (1.) or else them being risky/behavior affecting (2.), depending on which nullability annotation is added to each and for what reason. Wdyt? 🤔

Hm.. I'm not quite sure whether slicing the work from a different angle would help separate the cases between compile-time and runtime PRs 🤔 , but if you think it will help, I'd say go for it! I think that the "compile time" PR should be something where very little effort is required to review, and I'd expect the other changes (affecting runtime) to be more carefully scrutinized. How this breaks down along lines of override method usage, or between Activities / Fragments, etc. is not 💯 clear to me at the moment, so use your best judgement 😄 .

ParaskP7 · 2023-08-29T11:35:53Z

👋 @mkevins !

I understand your rationale, and I don't disagree, in principle, especially for cases where we are already treating the types as @nonnull without issue throughout our code. But, I think we should classify the risk distinctly from annotation usage which can only reduce the possibility of crashes.

I agree! 👍

I agree with this point, and this may be another way of looking at the split in PRs that I suggested - to separate the concerns. Changes that could (however unlikely we estimate) introduce a new crash should, imo, be reviewed / followed up on, etc. differently from those changes which cannot possibly introduce a new crash, and could only possibly improve the developer and user experience. Those in the latter category make for an easy PR review, since CI is the main testing step.

I agree! 👍

I think it might help if I clarify what I'm envisioning with regard to this. I see this effort as something that will be approached gradually, and what the lint rule will help to achieve after raising sufficient awareness of the underlying issue (that is, nullness ambiguity). To me, this ambiguity is costly, and addressing it is also costly:

Nicely done on the table! 💯

As a matter of principle, I value the user experience above the developer experience, so avoiding crashes or undefined behavior is a high priority, imo.

Totally, I am aligned with you on that, and I understand how me focusing on developer experience might make me seen as siding more on the developers side, more so than the users side. I want to take this opportunity to clarify that this is not it.

Instead, the way I think about it, to avoid crashes does not always mean a better user experience. Sometime, it might mean that by avoiding crashes the user enters into an undefined behavior. This could take much more effort for the user to figure out and report, and then, it would might even more time and resources (accumulatively) for it to get prioritized and fixed, that is, comparing to fixing a NPE crash, which is reported instantly and usually doesn't require prioritzation.

That said, I believe you have a valid point about the reduced risk of crashes when we've already been treating certain cases as @nonnull in an ad hoc fashion.

👍

I noticed that you mentioned the idea in a few places that there is a trade-off between "fail fast" and "fail silently", and that @nonnull is associated with the former, and @nullable the latter. I want to interject on this point, because I think there is a big difference in how we envision @nullable to be applied. Personally, I think adding @nullable requires significantly more effort on the part of the developer, unless the existing code already gracefully and appropriately handles the null cases. I do not think that @nullable should ever be added without appropriately walking the downstream codepaths to ensure validity (i.e. not just adding a null guard on a conditional, without a thought about what this does to the logical state of the running code). @nonnull, on the other hand, is trivial to apply from the developer side. Instead, it exposes users to potential issues / crashes.

👍

With this in mind, I still agree with your sentiments about inferring @nonnull from wide usage without NPEs. I also think that @nullable is not preferred in a bulk PR - not because of "silent failure", but rather because I don't think this should be done without at least familiarity with the underlying feature and implementation.

👍

I'm not sure what you "disagree" with, as I wasn't stating an opinion, but rather the opposite - I am unsure where these commits belong. But on thinking about it further, I think these commits can be separated in the same way as the others. If they won't cause runtime issues, or if they might. Fwiw, a quick look across all the "refactor" commits, so far this is the only one that could introduce runtime issues.

Ah, again, I think the confusion here was due to this "refactor" commit, which is actually not the same as the other such commits, thus, you were stating your opinion on that, while I was thinking that you were talking about all the other ones. 😭

Apologies for this "refactor" commit. I just couldn't figure out whether to use "analysis" or "refactor" there, from our discussion and the fact that this commit might end-up being harmful, I now seen it more clearly, I should have used the "analysis" wording there. That might have helped us avoiding having this discussion in the first place as I am now seeing we are both aligned on having "minor" refactor separate unharmful commits.

This all was my bad. 😞

I think it's ok to batch, especially since you have very nicely organized the commits to make reverts easy.

👍

You're right about 3, it can be split up by the same criteria and added to 1 and 2.

👍

Maybe it'd be best to "convert" this PR into 1, and spin up another for 2? Wdyt?

Let me start with (1.) and then have another quick individual PR for (2.) just for a specific @Override signature change (like the potential risky onNewIntent(...) change). Then, we can take it from there and see if I should create more such (2.) individual PR for all the other such potential risky changes.

Actually, let's do the vertical approach I suggested (one PR per @Override method), even for (1.), and then take it from there. This way will will have something that we will end-up merging to trunk as quick as possible, for both (1.) and (2.) types of PRs, the safe and risky PRs correspondingly.

Wdyt? 🤔

I would suggest for this "1" PR that we add as many annotations as possible, including annotations which are not present on a super method signature (by inferring as you have already done), with the exception that if such annotations require you to change any other code (removing or adding a guard, etc.), those should be omitted. Essentially, the compiled output should remain unchanged by this PR.

Yea, I like that approach too, seems the safer choice, to have a compiled output unchanged for the (1.) type of PRs. 👍

But, as suggested above, let me try to work vertically on those PRs (one PR per @Override method), even for (1.), and then we can take it from them.

Hm.. I'm not quite sure whether slicing the work from a different angle would help separate the cases between compile-time and runtime PRs 🤔 , but if you think it will help, I'd say go for it!

Yea, I think it will, or at least make sure that we adhere to everything we have already discussed so far and see the results of that merging to trunk quicker. Also, me doing that will make for a good experiment so I don't spend an unnecessary amount of time on creating a PR that we might again end-up closing (for good reasons, just like it is done for this PR).

I think that the "compile time" PR should be something where very little effort is required to review, and I'd expect the other changes (affecting runtime) to be more carefully scrutinized. How this breaks down along lines of override method usage, or between Activities / Fragments, etc. is not 💯 clear to me at the moment, so use your best judgement 😄 .

Using my best judgement it is then, wish us luck! 🤣 🍀 ⚖️

ParaskP7 · 2023-08-29T11:37:57Z

As per this I am now closing this PR and will be creating vertical type of PRs (one PR per @Override method), for both, the safe (1.) type of PRs and the more risky (2.) type of PRs. Cc @mkevins

mkevins · 2023-08-29T21:47:26Z

Thank you for your huge effort on all of this Petros!

Instead, the way I think about it, to avoid crashes does not always mean a better user experience. Sometime, it might mean that by avoiding crashes the user enters into an undefined behavior. This could take much more effort for the user to figure out and report, and then, it would might even more time and resources (accumulatively) for it to get prioritized and fixed, that is, comparing to fixing a NPE crash, which is reported instantly and usually doesn't require prioritzation.

I totally agree about this as well, so I think we are aligned, and our discussions have been productive toward illustrating the common facets of the "null problem".

Ah, again, I think the confusion here was due to this "refactor" commit, which is actually not the same as the other such commits, thus, you were stating your opinion on that, while I was thinking that you were talking about all the other ones. 😭

Indeed, that commit in particular stands out as one of the "risky" ones, compared to the others. The other refactor commits seem to me to not even be capable of harm, whereas this one potentially could (though I think the risk is likely rather low). Thanks for separating them out! 👍

ParaskP7 · 2023-08-30T08:07:29Z

Thank for this constructive discussion we had @mkevins , now onwards and upwards we go, we will find you, we will kill you, you Mr. NPE! 🙇 ❤️ 🚀

ParaskP7 added 30 commits August 17, 2023 13:01

Analysis: Add missing n-a to sdk override methods for web view

346a8f8

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for wp web view

be35881

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for theme web

1ebb789

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for jetpack web view

064b8ee

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for ssl certs view

91a632a

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for add qp shortcut

2d6457f

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for app log viewer

e31f056

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for jetpack connection

adb1e7f

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for share receiver

56febf4

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for login

4737bfb

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for login epilogue

dfefa3b

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for login magic link

7f4cbaa

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for signup epilogue

4e9a2c1

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for comments detail

a0da7cd

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for edit comment

3e92a6d

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for deep linking

8a38d8c

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for site picker

88d274f

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for wp main

2b54a8b

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for media browser

7a7fc66

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for media preview

4152299

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for media settings

7753264

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for notifications

387099e

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for people management

4db5b4e

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for photo picker

cd60734

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for plugin browser

4b0d8a3

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for plugin detail

9a80cba

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for edit post

2f3028f

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to sdk override methods for post settings tag

9f0e7fd

FYI: 'n-a' stands for 'nullability annotations'.

ParaskP7 added 11 commits August 17, 2023 15:36

Refactor: Change modifier to protected on on activity result method

8405c3b

This is done so as to have the 'onActivityResult(...)' method match the super method signature.

Refactor: Rename s param to query on on query text submit method

5a7504a

This is done so as to have the 'onQueryTextSubmit(...)' method match the super method signature.

Refactor: Rename s param to new text on on query text change method

922e9d9

This is done so as to have the 'onQueryTextChange(...)' method match the super method signature.

Refactor: Rename action mode param to mode on on create action mode

0acc778

This is done so as to have the 'onCreateActionMode(...)' method match the super method signature.

Refactor: Rename action mode param to mode on on prepare action mode

6068d3a

This is done so as to have the 'onPrepareActionMode(...)' method match the super method signature.

Refactor: Rename action mode param to mode on on action item clicked

83b9261

This is done so as to have the 'onActionItemClicked(...)' method match the super method signature.

Refactor: Rename menu item param to item on on action item clicked

975642e

This is done so as to have the 'onActionItemClicked(...)' method match the super method signature.

Refactor: Rename action mode param to mode on on destroy action mode

15fefd7

This is done so as to have the 'onDestroyActionMode(...)' method match the super method signature.

Refactor: Remove final keyword from item on on bind viewHolder method

a452dbf

This is done so as to have the 'onBindViewHolder(...)' method match the super method signature.

Refactor: Rename view param to v on on click method

4886871

This is done so as to have the 'onClick(...)' method match the super method signature.

ParaskP7 added Core Tech Debt labels Aug 17, 2023

ParaskP7 added this to the Future milestone Aug 17, 2023

ParaskP7 requested a review from mkevins August 17, 2023 13:51

ParaskP7 self-assigned this Aug 17, 2023

ParaskP7 mentioned this pull request Aug 10, 2023

Nullability Annotations to Java Classes - Activity & Fragment - SDK @Override Methods #18909

Open

Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-Android…

e1dc640

… into analysis/add-missing-nullability-annotations-to-all-sdk-override-methods-for-activities # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

mkevins reviewed Aug 24, 2023

View reviewed changes

ParaskP7 closed this Aug 29, 2023

ParaskP7 mentioned this pull request Aug 29, 2023

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All SDK Override Methods for Fragments #19004

Closed

12 tasks

jkmassel deleted the analysis/add-missing-nullability-annotations-to-all-sdk-override-methods-for-activities branch October 17, 2024 18:59

		boolean hasError = (editContent.getError() != null);
		boolean hasText = (s != null && s.length() > 0);

		@@ -31,11 +31,9 @@ class ScanActivity : AppCompatActivity(), ScrollableViewInitializedListener {

		private var binding: ScanActivityBinding? = null

		override fun onNewIntent(intent: Intent?) {

		public boolean onQueryTextChange(@NonNull String newText) {
		mViewModel.setSearchQuery(newText);

                           }
                       }
+                      @NonNull

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All SDK Override Methods for Activities #18997

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All SDK Override Methods for Activities #18997

Conversation

ParaskP7 commented Aug 17, 2023 • edited Loading

To test:

Regression Notes

PR submission checklist:

UI Changes testing checklist:

wpmobilebot commented Aug 17, 2023 • edited Loading

wpmobilebot commented Aug 17, 2023 • edited Loading

mkevins left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParaskP7 commented Aug 24, 2023

mkevins commented Aug 25, 2023

ParaskP7 commented Aug 29, 2023 • edited Loading

ParaskP7 commented Aug 29, 2023

mkevins commented Aug 29, 2023

ParaskP7 commented Aug 30, 2023

ParaskP7 commented Aug 17, 2023 •

edited

Loading

wpmobilebot commented Aug 17, 2023 •

edited

Loading

wpmobilebot commented Aug 17, 2023 •

edited

Loading

mkevins left a comment •

edited

Loading

ParaskP7 commented Aug 29, 2023 •

edited

Loading