[Lint Warnings] Source Set: Main - Resolve All Warnings [`Correctness`] - Part.1 #18245

ParaskP7 · 2023-04-07T15:24:20Z

Parent #18162
Partially Closes #18193

This PR resolves the 1st part of correctness Lint related warnings for the Main source set:

Reconfigure Lint (Ignore):

Ignore locale folder lint warnings for main

Warnings Resolution List:

Resolve use get layout inflater lint warnings. + With tests instruction!
Resolve intent reset lint warning for publish settings. + With tests instruction!
Resolve fragment tag usage lint warnings everywhere else. + With tests instruction!

Warnings Suppression List:

Suppress use get layout inflater lint warn for number picker. + With tests instruction!
Suppress scoped storage lint warning for main.
Suppress fragment tag usage lint warnings for settings. + With tests instruction!
Suppress fragment tag usage lint warnings for widgets. + With tests instruction!

Refactor List:

PS: @irfano I added you as the main reviewer, randomly, since I just wanted someone from the WordPress team to be aware of and sign-off on that change for WPAndroid. I also added the @wordpress-mobile/apps-infrastructure team, but this in done only for monitoring purposes, as such, I am not expecting any active review from that team. Thus, feel free to merge this PR if you deem so.

To test:

Smoke test both, the WordPress and Jetpack apps, and see if they both work as expected.
In addition to the above smoke test, per the warnings resolution or suppression list, please expand below and follow the inner and more explicit test steps within:

Warnings Resolution List (✅):

[UseGetLayoutInflater]

1. Number Picker Dialog [NumberPickerDialog.java]

Go to My Site tab -> MENU sub-tab -> Side Settings screen.
Find the Post per page option and click on it.
Verify that the number picker dialog is shown and functioning as expected.

2. Post Settings Input Dialog [PostSettingsInputDialogFragment.java]

Go to Posts screen -> Find a post and click on it -> Post settings screen.
Find the Password option and click on it.
Verify that the post settings input dialog is shown and functioning as expected.

3. Text Input Dialog [TextInputDialogFragment.java]

Go to Me screen -> My Profile screen.
Find the First name option and click on it.
Verify that the text input dialog is shown and functioning as expected.

[IntentReset]

1. Publish Settings Bottom Sheet [PublishSettingsFragment.kt]

Go to Posts screen -> Create a new post -> Click on PUBLISH button.
Find the Publish Date option and click on it.
Find the Date and Time option and click on it.
Chose a future date and time. The Notification and Add to calendar options will appear.
Find the Add to calendar option and click on it.
Verify that you get redirected to the Calendar app and an draft calendar event is being created
on your behalf. Verify that the calendar title, description and dates are correct. Click on Save
to create the calendar event. Verify that the calendar event has been created and is as expected.

[FragmentTagUsage]

1. Edit Image Screen [activity_edit_image.xml]

Go to Blogs and add a new blog post.
Add a new image block.
Choose an image and wait for it to be uploaded within the image block.
Click on the media options of this image (top right) and then click edit.
Verify that the Edit Image screen is shown and functioning as expected.

2. My Profile Settings Screen [app_settings_activity.xml]

Go to Me screen -> My Profile settings screen.
Verify that the My Profile settings screen is shown and functioning as expected.

3. Debug Settings Screen [debug_settings_activity.xml]

Go to Me screen -> App Settings screen -> Debug Settings screen.
Verify that the Debug Settings screen is shown and functioning as expected.

4. Categories Setting Screen [site_settings_categories_list_activity.xml]

Go to My Site tab -> MENU sub-tab -> Site Settings screen.
Find the Writing section in the middle and click on its Categories option.
Verify that the Categories setting screen is shown and functioning as expected.

5. Insights Management Screen [insights_management_activity.xml]

Go to My Site tab -> MENU sub-tab -> Stats screen.
Either, Find the settings gear icon (top right) and click on it.
Or, scroll down and find the Add new stats card option and click on it.
Verify that the Add New Card stats setting screen is shown and functioning as expected.

6. Plans Screen [plans_activity.xml]

ℹ️ This my site menu option is only shown conditionally.

Go to My Site tab -> MENU sub-tab -> Plans screen.
Verify that the Plans screen is shown and functioning as expected.

7. Activity Log Screen [activity_log_list_activity.xml]

Go to My Site tab -> MENU sub-tab -> Activity Log screen.
Verify that the Activity Log screen is shown and functioning as expected.

8. Backup Download Screen [backup_download_activity.xml]

Go to My Site tab -> MENU sub-tab -> Backup screen.
Find a backup event item from the list and click on it.
From the 'Event' screen find the 'DOWNLOAD BACKUP' button and click on it.
Verify that the Download Backup screen is shown and functioning as expected.

9. Restore Screen [restore_activity.xml]

Go to My Site tab -> MENU sub-tab -> Backup screen.
Find a backup event item from the list and click on it.
From the 'Event' screen find the 'RESTORE' button and click on it.
Verify that the Restore screen is shown and functioning as expected.

10. Scan Screen [scan_activity.xml]

ℹ️ This my site menu option is only shown conditionally.

Go to My Site tab -> MENU sub-tab -> Scan screen.
Find a backup event item from the list and click on it.
Verify that the Scan screen is shown and functioning as expected.

11. Scan History Screen [scan_history_activity.xml]

ℹ️ This my site menu option is only shown conditionally.

Go to My Site tab -> MENU sub-tab -> Scan screen.
Click on the 'HISTORY' option (top right).
Verify that the Scan History screen is shown and functioning as expected.

12. Threat Details Screen [scan_history_activity.xml]

ℹ️ You would need to add a threat to your side so that after you trigger 'Scan now' on your side,
then let it complete, it will finally show some threats on your site.

Go to My Site tab -> MENU sub-tab -> Scan screen.
Find a threat item from the Thread found list and click on it.
Verify that the Threat Details screen is shown and functioning as expected.

13. Set Parent Screen [pages_parent_activity.xml]

Go to Pages screen and set a parent page for a page (page options).
Verify that the Set Parent screen is shown and functioning as expected.

14. Follow Topics Screen [reader_interests_activity.xml]

Go to Reader tab -> DISCOVER sub-tab.
Find the manager topics & sites icon (top right) and click on it.
Click the '+' FAB button to add a new topic.
Verify that the Follow topics screen is shown and functioning as expected.

15. At A Glance Widget Screen [stats_minified_widget_configure_activity.xml]

Add the At a glance widget to your home screen.
Verify that the At a glance widget screen is shown and functioning as expected.

Warnings Suppression List (❌):

FYI: Although this Lint warning got suppressed, I did try to resolve them first, tested them, which most of the times resulted into a crash, and then, reverted those to a suppression them instead. You can try that yourself following the below testing instructions:

[FragmentTagUsage]

1. Account Settings Screen [account_settings_activity.xml]

❌ Exception: ClassCastException

Go to Me screen -> My Profile screen.
Verify that the Account Settings screen is shown and functioning as expected.

2. App Settings Screen [app_settings_activity.xml]

❌ Exception: ClassCastException

Go to Me screen -> App Settings screen.
Verify that the App Settings screen is shown and functioning as expected.

3. Jetpack Security Settings Screen [fragment_jetpack_security_settings.xml]

❌ Exception: ClassCastException

ℹ️ It seems that this flow has been migrated into the Site Settings screen and its Jetpack Settings section at the bottom. Thus, it is hard to be tested and most importantly maybe worth removing at some point due to it ending-up becoming dead code.

Go to My Site tab -> MENU sub-tab -> Jetpack Settings screen.
Verify that the App Settings screen is shown and functioning as expected.

4. All Time + Today + Views + Views This Week Widget Screens [stats_xyz_widget_configure_activity.xml]

❌ Exception: UninitializedPropertyAccessException

ℹ️ The 'xyz' part of the file name is either all_time, today, views or week_views.

❓️ The widget title for views or week_views is always the same, the Views this week one. Maybe that needs to be corrected so the first one become a more generic Views title and much that of the views layout file.

Add an All-time, Today, Views this week or Views this week widget to your home screen.
Verify that the All-time, Today, Views this week or Views this week widget screen are
shown and functioning as expected.

Regression Notes

Potential unintended areas of impact
- Potential change of behaviour on PublishSettingsFragment and its AddToCalendar functionality.
- Potential breakage on screens that the LayoutInflater.from(getActivity()) tag got replaced with the activity().getLayoutInflater() tag.
- Potential breakage on screens that the <fragment> tag got replaced with the <androidx.fragment.app.FragmentContainerView> tag.
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.

Warning Message: "Use of LayoutInflater.from(getActivity()) detected. Consider using getLayoutInflater() instead" Explanation: "Using LayoutInflater.from(Context) can return a LayoutInflater that does not have the correct theme." ------------------------------------------------------------------------ The recommended use of 'getLayoutInflater()', instead of using the deprecated 'LayoutInflater.from(getActivity())' is causing a 'StackOverflowError' error. Thus, this error is suppress for 'NumberPickerDialog' (at least for the time being).

Warning Message: "Use of LayoutInflater.from(getActivity()) detected. Consider using getLayoutInflater() instead" Explanation: "Using LayoutInflater.from(Context) can return a LayoutInflater that does not have the correct theme." ------------------------------------------------------------------------ Using the recommended 'getLayoutInflater()', and on those specific cases via 'getActivity()', fix these warnings.

Warning Message: "WRITE_EXTERNAL_STORAGE no longer provides write access when targeting Android 11+, even when using 'requestLegacyExternalStorage'" Explanation: "Scoped storage is enforced on Android 10+ (or Android 11+ if using requestLegacyExternalStorage). In particular, WRITE_EXTERNAL_STORAGE will no longer provide write access to all files; it will provide the equivalent of READ_EXTERNAL_STORAGE instead." ------------------------------------------------------------------------ This warning is already addressed and will be fixed as part of the ongoing Android 13 media permissions work (see PR below): PR: #18183

Warning Message: "Calling `setType` after calling `setData` will clear the data: Call `setDataAndType` instead?" Explanation: "Intent provides the following APIs: setData(Uri) and setType(String). Unfortunately, setting one clears the other. If you want to set both, you should call setDataAndType(Uri, String) instead." ------------------------------------------------------------------------ This 78c2ea7 introduced this functionality. Testing without the 'calIntent.data = Events.CONTENT_URI' still works as expected. Thus, indeed it seems that the 'data' got cleared by setting the 'type' afterwards, thus the code, if kept as is, although still functioning, becomes misleading and worse, the 'data' related 'Events.CONTENT_URI' information ends-up being missing from the intent that starts the external calendar activity. From the looks of it, it seems that the author's intentions were to use both, 'data' and 'type'. As such, using the recommended 'setDataAndType(...)' fixes this warning. ------------------------------------------------------------------------ FOr more info see: - Calendar provider overview (Doc): https://developer.android.com/guide/topics/providers/calendar-provider - Use an intent to insert an event (Doc): https://developer.android.com/guide/topics/providers/calendar-provider #intent-insert

Warning Messages: - "The locale folder "'he'" should be called "'iw'" instead; see the 'java.util.Locale' documentation" - "The locale folder "'id'" should be called "'in'" instead; see the 'java.util.Locale' documentation" Explanation: "From the java.util.Locale documentation: "Note that Java uses several deprecated two-letter codes. The Hebrew ("he") language code is rewritten as "iw", Indonesian ("id") as "in", and Yiddish ("yi") as "ji". This rewriting happens even if you construct your own Locale object, not just for instances returned by the various lookup methods. Because of this, if you add your localized resources in for example values-he they will not be used, since the system will look for values-iw instead. To work around this, place your resources in a values folder using the deprecated language code instead." ------------------------------------------------------------------------ This Lint warning is moved within lint config and removed from Lint's baseline because in order to resolve this warning a proper release management localization solution should be set-up for both, the WordPress and Jetpack apps. PS: The previous such Jetpack related c272690 commit description was misleading (describing this was already properly done on WordPress). Related: Lint Warnings - Resolve 'LocaleFolder' - #18226

Warning Message: "Redundant 'field' used for Dagger qualifier annotation." Explanation: "It's redundant to use 'field:' site-targets for qualifier annotations." ------------------------------------------------------------------------ This was probably an oversight while this efb4b8f commit was developed. Removing the redundant 'field' site-target fixes these warnings.

Instead of using an arbitrary 'IO_THREAD' string value, it is better to depend on using the 'IO_THREAD' constant from within the 'ThreadModule' file, which anyway points to that same 'IO_THREAD' string value.

Warning Message: "Replace the <fragment> tag with FragmentContainerView." Explanation: "FragmentContainerView replaces the <fragment> tag as the preferred way of adding fragments via XML. Unlike the <fragment> tag, FragmentContainerView uses a normal FragmentTransaction under the hood to add the initial fragment, allowing further FragmentTransaction operations on the FragmentContainerView and providing a consistent timing for lifecycle events." ------------------------------------------------------------------------ These warnings are suppressed on those files because the app crashes otherwise with a 'ClassCastException' exception (see an example crash on 'AccountSettingsFragment' below): ------------------------------------------------------------------------ Caused by: java.lang.ClassCastException: org.wordpress.android.ui.prefs.accountsettings.AccountSettingsFragment cannot be cast to androidx.fragment.app.Fragment ------------------------------------------------------------------------

Warning Message: "Replace the <fragment> tag with FragmentContainerView." Explanation: "FragmentContainerView replaces the <fragment> tag as the preferred way of adding fragments via XML. Unlike the <fragment> tag, FragmentContainerView uses a normal FragmentTransaction under the hood to add the initial fragment, allowing further FragmentTransaction operations on the FragmentContainerView and providing a consistent timing for lifecycle events." ------------------------------------------------------------------------ These warnings are suppressed on those files because the app crashes otherwise with a 'UninitializedPropertyAccessException' exception (see below): ------------------------------------------------------------------------ kotlin.UninitializedPropertyAccessException: lateinit property widgetType has not been initialized ------------------------------------------------------------------------

Warning Message: "Replace the <fragment> tag with FragmentContainerView." Explanation: "FragmentContainerView replaces the <fragment> tag as the preferred way of adding fragments via XML. Unlike the <fragment> tag, FragmentContainerView uses a normal FragmentTransaction under the hood to add the initial fragment, allowing further FragmentTransaction operations on the FragmentContainerView and providing a consistent timing for lifecycle events." ------------------------------------------------------------------------ Using the recommended '<androidx.fragment.app.FragmentContainerView>' fix these warnings.

wpmobilebot · 2023-04-07T15:42:23Z

📲 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	pr18245-d7554d9
	Commit	`d7554d9`
	Direct Download	`jetpack-prototype-build-pr18245-d7554d9.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-04-07T15:43:44Z

📲 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	pr18245-d7554d9
	Commit	`d7554d9`
	Direct Download	`wordpress-prototype-build-pr18245-d7554d9.apk`

Note: Google Login is not supported on these builds.

Warning Message: "Use of LayoutInflater.from(getActivity()) detected. Consider using getLayoutInflater() instead" Explanation: "Using LayoutInflater.from(Context) can return a LayoutInflater that does not have the correct theme." ------------------------------------------------------------------------ Using the recommended 'getLayoutInflater()', and in this specific case via 'getActivity()', fixes this warning too. FYI: I previously suppressed it because I incorrectly used 'getLayoutInflater()' directly and not using 'getActivity()' first, like: 'getActivity().getLayoutInflater()'

irfano

There are well-explained changes. I have checked them all. LGTM! Great Job! 👍🏻
I have tested all your written test cases in both WP and JP, and they have all passed. 🟢
Although I have left a comment, I am not merging but approving it because the issue will be resolved after syncing with the trunk.

Maybe that needs to be corrected so the first one become a more generic Views title and much that of the views layout file.

It makes sense to me. We can open an issue for it.

WordPress/src/main/AndroidManifest.xml

ParaskP7 · 2023-04-19T14:32:46Z

There are well-explained changes. I have checked them all. LGTM! Great Job! 👍🏻
I have tested all your written test cases in both WP and JP, and they have all passed. 🟢

Awesome, thank you so much for reviewing and testing these changes @irfano , you rock! 🙇 ❤️ 🚀

Although I have left a comment, I am not merging but approving it because the issue will be resolved after syncing with the trunk.

Yes, your comment is spot-on and I was waiting on it to happen, on way or another, depending on which PR would be merged first, this one or the targetSdkVersion-to-33 one, thanks for taking the time to call that off! 💯

…ings

peril-wordpress-mobile · 2023-04-19T14:34:11Z

	Warnings
⚠️	PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

ParaskP7 added 10 commits April 7, 2023 11:42

Refactor: Use io thread constant as defined within thread module

f9e1d57

Instead of using an arbitrary 'IO_THREAD' string value, it is better to depend on using the 'IO_THREAD' constant from within the 'ThreadModule' file, which anyway points to that same 'IO_THREAD' string value.

ParaskP7 added Tooling Tech Debt labels Apr 7, 2023

ParaskP7 added this to the Future milestone Apr 7, 2023

ParaskP7 self-assigned this Apr 7, 2023

ParaskP7 requested review from irfano and a team April 7, 2023 16:05

ParaskP7 marked this pull request as ready for review April 7, 2023 16:05

ParaskP7 mentioned this pull request Apr 7, 2023

Lint Warnings - Source Set: Main [Correctness] #18193

Open

22 tasks

irfano approved these changes Apr 18, 2023

View reviewed changes

WordPress/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved

Merge branch 'trunk' into analysis/resolve-main-correctness-lint-warn…

d7554d9

…ings

ParaskP7 merged commit 4ea697d into trunk Apr 19, 2023

ParaskP7 deleted the analysis/resolve-main-correctness-lint-warnings branch April 19, 2023 15:09

This was referenced Apr 19, 2023

Lint Warnings - Resolve/Suppress Baseline Warnings #18162

Open

Lint Warnings - Reconfigure, Add Baseline & Resolve/Suppress Baseline Warnings #18142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lint Warnings] Source Set: Main - Resolve All Warnings [`Correctness`] - Part.1 #18245

[Lint Warnings] Source Set: Main - Resolve All Warnings [`Correctness`] - Part.1 #18245

ParaskP7 commented Apr 7, 2023 •

edited

Loading

wpmobilebot commented Apr 7, 2023 •

edited

Loading

wpmobilebot commented Apr 7, 2023 •

edited

Loading

irfano left a comment •

edited

Loading

ParaskP7 commented Apr 19, 2023

peril-wordpress-mobile bot commented Apr 19, 2023

[Lint Warnings] Source Set: Main - Resolve All Warnings [Correctness] - Part.1 #18245

[Lint Warnings] Source Set: Main - Resolve All Warnings [Correctness] - Part.1 #18245

Conversation

ParaskP7 commented Apr 7, 2023 • edited Loading

To test:

Regression Notes

wpmobilebot commented Apr 7, 2023 • edited Loading

wpmobilebot commented Apr 7, 2023 • edited Loading

irfano left a comment • edited Loading

Choose a reason for hiding this comment

ParaskP7 commented Apr 19, 2023

peril-wordpress-mobile bot commented Apr 19, 2023

[Lint Warnings] Source Set: Main - Resolve All Warnings [`Correctness`] - Part.1 #18245

[Lint Warnings] Source Set: Main - Resolve All Warnings [`Correctness`] - Part.1 #18245

ParaskP7 commented Apr 7, 2023 •

edited

Loading

wpmobilebot commented Apr 7, 2023 •

edited

Loading

wpmobilebot commented Apr 7, 2023 •

edited

Loading

irfano left a comment •

edited

Loading