[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Media Model Classes (breaking)
#2886
Conversation
Warnings: - "Non-final field 'mFieldName' in enum 'MediaFields'" - "Field 'mFieldName' may be 'final'"
Warning: "Condition 'other == null' covered by subsequent condition '!(other instanceof MediaModel)'" FYI: One could resolve this warning, simply by removing the unnecessary 'other == null' condition, but this would change the 'equals(...)' logic and possible introduce some kind of a regression, most liked performance wise. Thus, it is better to ignore this atm, just by suppressing it, and call this out of scope.
This 'setFieldsToUpdate(...)' method is actually meant to and used by client apps (ie. JP/WPAndroid). They just seem unused as they are not being directly used via the example app or any unit/ui tests within this repo.
FYI: 'n-a' stands for 'nullable annotations'.
FYI: 'n-a' stands for 'nullable annotations'.
FYI: 'n-a' stands for 'non-null annotations'.
FYI: 'n-a' stands for 'nullability annotations'. PS: This 'NotNullFieldNotInitialized' warning got suppressed because a table related 'model' class can never have its fields initialized via a constructor initialization, or otherwise for that matter.
This is done so that clients of that model, clients like JP/WPAndroid and WCAndroid, when trying to manually instantiating such a model, they will always use the non-default constructor, and thus always provide at least an 'localSiteId' and 'mediaId' argument (for an existing media). The empty default constructor was added because '@table' related models need to have one such constructor. Otherwise, its 'Mapper' related auto-generated class would fail to compile, in this case the 'MediaModelMapper.java' class. Also, a full-arguments constructor was added, which should be the preferred and goto constructor for most cases. FYI: The trick I used to first '@deprecated' this constructor is to help clients avoid using this constructor in the future, that is, when they manually construct one. Suppressing the 'DeprecatedIsStillUsed' warning was necessary because otherwise, even if every occurrence in the codebase is being switched to the non-default constructor, the auto-generated 'Mapper' related class will anyway use that default constructor, thus this warning will persist no matter what. PS: Note that on all the partial-argument constructors, an extra Javadoc is added to help clients understand when and why to use each. For all other cases, using the full-arguments constructor should be the preferred and goto constructor.
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
Warning: "Use of getter method instead of property access syntax"
Warning: "'assertTrue()' can be simplified to 'assertEquals()'"
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations' PS: Also, as part of this change, the 'getSiteImageCount(...)' method got removed as it is only being used within 'MediaStoreTest'. The is no reason to create a new method for 'getSiteImages(siteModel).size()' alone, and more so, when no client apps like JP/WP/WCAndroid are using it.
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations' PS: Also, as part of this change, the 'getUnattachedSiteMediaCount(...)' method got removed as it is only being used within 'MediaStoreTest'. The is no reason to have a new method for 'getSiteImages(siteModel).size()' alone, and more so, when no client apps like JP/WP/WCAndroid are using it.
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
Warning: "Field 'mRandom' may be 'final'"
Warning: "Condition 'i < imageIds.size()' is always 'false'"
Warning: "Exception 'java.lang.InterruptedException' is never thrown in the method"
Warning: "Actual value of parameter 'num' is always 'MediaSqlUtilsTest.SMALL_TEST_POOL'"
|
FYI: Per the merge instructions within the PR description, I am going to create the accompanying JP/WPAndroid#TODO and WCAndroid#TODO PRs tomorrow. Until then, feel free to start the review process and test this PR with the |
|
FYI: The accompanying JP/WPAndroid#19506 PR is up and ready. As such, the code review on the JP/WPAndroid side can be fully completed. @AjeshRPai |
|
FYI: The accompanying WCAndroid#10078 PR is up and ready. As such, the code review on the WCAndroid side can be fully completed. @0nko |
There was a problem hiding this comment.
I've tested the FluxC Example app and I found no issues 👍.
As for the code changes, I've looked at the parts that are relevant to WCAndroid and it looks good. I'll let @AjeshRPai take a more detailed look since he's probably more familiar with it.
I already mentioned it in the WCAndroid PR, but we'll also want to update the MediaPicker library.
👏
|
Thank you so much for reviewing, testing and approving these and the changes in the accompanying WCAndroid PR @0nko , you are awesome! 🙇 ❤️ 🚀
Oh true, this is interesting, I'll response to that in the accompanying WCAndroid PR, thanks for mentioning MediaPicker, I knew I picked the right person for this review! 💯 😊 💯 |
…ndroid into analysis/add-missing-nullability-annotations-to-media-model-classes
There was a problem hiding this comment.
I have tested the changes in this PR using the accompanying wordpress-mobile/WordPress-Android#19506 and everything looks good to me 👍🏼
I am approving but not merging since this change also affects WCAndroid.
|
Awesome, thank for reviewing and testing this @AjeshRPai , you rock! 🙇 ❤️ 🚀 FYI: I'll proceed with the merging myself. 🙏 |
ParaskP7
deleted the
analysis/add-missing-nullability-annotations-to-media-model-classes
branch
Parent #2799
Closes #2832
This PR adds any missing nullability annotation to MediaModel.java model and all related store & sql-util classes.
FYI: This change is
breaking, meaning that any client that depended on the MediaModel.java model should update to the new APIs (non-default constructors) as the existing API (default constructor) are now deprecated. As such, this change is inherentlyrisky, meaning that there are compile-time changes associated with this change and thus needs testing to verify correctness, both on the library's and client's side.PS: Any other changes on any other class are just some propagating missing nullability annotation changes, which stem from the main set of classes being updated, this PR's focus, plus, some other extra minor analysis, refactor and test related changes.
PS.1: @AjeshRPai I added you as the main reviewer, not so randomly (it being a continuation of #2878), since I just wanted someone from the Jetpack/WordPress mobile team to be aware of and sign-off on that change. Feel free to merge this PR directly yourself if you deem so.
PS.2: @0nko I am also adding you as an extra reviewer, not so randomly (it being a continuation of #2878), since I want someone from the WooCommerce mobile team to be aware of this change as well. However, there is no need for you to review it as well, thus duplicating the review work. Having one team member from either of the mobile teams reviewing and testing this change is enough for this change.
Nullability Annotations List (
Model):breaking)Nullability Annotations List (
Store):Nullability Annotations List (
SqlUtils):Nullability Annotations List (
Utils):Warnings Resolution List:
Warnings Suppression List:
Refactor List:
Test List:
Associated Clients
It is highly recommending that this change is being tested on the below associated clients:
To Test (
REST):FluxC Exampleapp via theMEDIAscreen. Verify everything is working as expected.To Test (
XMLRPC):FluxC Exampleapp via theMEDIAscreen. Verify everything is working as expected.[JP/WP] Media Screens [MediaBrowserActivity.java + MediaGridFragment.kt + MediaSettingsActivity.java + MediaPreviewActivity.java + MediaPreviewFragment.java]
ℹ️ This test applies to both, the
JetpackandWordPressapp.Mediascreen, verify it is shown and functioning as expected.Media Settingsscreen is shown and functioning as expected.Media Previewscreen is shown and functioning as expected.[WC] Photos Screens [ProductDetailFragment.java + ProductImagesFragment.kt + ProductImageViewerFragment.kt]
Productstab and tap on any product.Photosscreen is shown and functioning as expected.Photo Previewscreen is shown and functioning as expected.Merge instructions:
[PR] Not Ready for Mergelabel.