Jetpack Mobile: Scan Introduce UiState by ashiagr · Pull Request #13578 · wordpress-mobile/WordPress-Android

ashiagr · 2020-12-10T11:20:23Z

This is a small preparatory PR with a scope to just link things (Service, VM, Fragment, ViewHolder) together for the Scan screen UI. It

introduces recycler view + adapter to the ScanFragment
introduces Content UiState to the ScanViewModel
populates basic elements (scan state icon, title) on the screen using the state received from the scan service.

I plan to use (in the upcoming PRs) different view types - scan state (with icon, title, description, action buttons), progress, threat header, threat items e.t.c. inside a RecyclerView so that entire screen is scrollable .

To test

Prerequisite: Make sure both Scan feature flag and MySiteImprovements flag are set on in the App settings.

Select a site with scan capability on the My Site tab
Notice that scan menu is displayed
Click scan menu item
Notice that basic elements (icon, title) are displayed on the Scan screen from the service.

Note

Used placeholder icons, texts. Ignore all stying.
ScanListItemState has to be fine tuned. I'll pickup and fine tune different view types (with their states) in the upcoming PRs.
I'll create a separate FluxC PR for storing/ retrieving threats into/ from db.

Merge Instructions

Make sure Jetpack Mobile: Scan menu #13555 is merged
Remove "Not ready for merge" label
Merge this PR to develop

PR submission checklist:

I have considered adding unit tests where possible.
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.

…ew items ScanListItemState will be structured similar to ActivityLogListItem

peril-wordpress-mobile · 2020-12-10T11:20:32Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2020-12-10T11:28:59Z

You can test the changes on this Pull Request by downloading the APK here.

ashiagr · 2020-12-10T12:54:56Z

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

+        val nonNullActivity = requireActivity()
+        (nonNullActivity.application as WordPress).component()?.inject(this)
+
+        val site = if (savedInstanceState == null) {


I think we can update FluxC code to just use siteId. We can eliminate this passing and retrieval of the entire site model. Lmkwyt.

👋 @ashiagr !

I think you are right and I agree. Since only siteId is the only property needed and considering that no other property from the entire site model will be used in the future, I believe the best course of action is to replace site with siteId.

IMHO, the main gain from doing something like that is to make it explicit to the reader that this is the only thing needed, so they know straight away and can score the feature down a bit. Obviously, there is a performance optimization as well, but to be honest this will not save us much.

I wanted a second opinion if we should be doing it, thanks for sharing your thoughts 🙏. I'll not make any changes as of now, may be just add "nice to have" in the backlog.

Yeap, definitely! But, on the other hand, it might be decided and for a good reason to keep using whole models instead, confronting my argument above, so I keep my mind open, as it indeed can help not having to make two PRs every time you want to add an extra property from the model or the whole model next time you needs more from it. 🤔

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

ParaskP7

Good job and thanks for including me in this PR @ashiagr ! 🌟

I have few comments, but very minor ones and most of them are kind of question related.

PS: The testing part will be done on a separate commit or on another PR?

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

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

WordPress/src/main/res/layout/scan_fragment.xml

ParaskP7 · 2020-12-11T11:12:37Z

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

+    private fun setupObservers() {
+        viewModel.uiState.observe(
+            viewLifecycleOwner,
+            Observer { uiState ->


Question (❓ ): Does your IDE also suggests that this redundant SAM-constructor can be removed?
Minor (🔍 ): In the end I can make this code look like the below:

viewModel.uiState.observe(viewLifecycleOwner, { uiState -> if (uiState is Content) refreshContentScreen(uiState) })

PS: Usually, in Kotlin, I tend to keep simple, not complex, if statements, a one-liner. Thus, the suggestion above. If I can recall correctly this is also something that is suggested in the official Android Kotlin style guide.

Does your IDE also suggests that this redundant SAM-constructor can be removed?

My IDE highlights it as well. It is for this reason mentioned in another PR review that I included it:

we're not removing them yet in the code for compatibility with older AS versions, right?

I tend to keep simple, not complex, if statements, a one-liner.

I just followed existing code convention in the VMs which use braces around if:

if (isStarted) { return }

My IDE highlights it as well. It is for this reason mentioned in another PR review that I included it:

Interesting, thanks for sharing Ashita! I have left the below comment there and quoting it here.

Hmmm... Is this due to compatibility reason with AS version or Kotlin versions? I am thinking the later, right? And if that is the case, I suggest having every develop be forced to update the AS Kotlin plugin to at least the Gradle's Kotlin version, currently 1.4.10, as specified by our build files. This will make sure everyone is on the same boat. Wdyt?

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

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

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

WordPress/src/main/res/layout/scan_list_scan_state_item.xml

ashiagr · 2020-12-11T12:46:25Z

I have few comments, but very minor ones and most of them are kind of question related.

Thanks so much for a thorough review @ParaskP7! 🌟. I've addressed your comments and answered questions. Let me know if any thing is not clear.

PS: The testing part will be done on a separate commit or on another PR?

They will be covered in a separate PR once UiStates are fine-tuned.

Ready for another round 🙇‍♀️.

ParaskP7 · 2020-12-11T13:23:58Z

Thanks so much for a thorough review @ParaskP7! 🌟. I've addressed your comments and answered questions. Let me know if any thing is not clear.

Thank YOU Ashita for addressing all of them and so gracefully!

They will be covered in a separate PR once UiStates are fine-tuned.

Perfect, I was sure of that, just trying to make sure I am not missing something.

Ready for another round 🙇‍♀️.

All good from my side! I just kept one comment unresolved just to trigger a short discussion for anyone interested! 😊

malinajirka

Thank you @ashiagr, great job! ;) I've left some comments. Let me know what you think.

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

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

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

.../main/java/org/wordpress/android/ui/jetpack/scan/adapters/viewholders/ScanStateViewHolder.kt

WordPress/src/main/res/drawable/ic_scan_idle_threats_found.xml

WordPress/src/main/res/layout/scan_fragment.xml

WordPress/src/main/res/values/strings.xml

ashiagr · 2020-12-15T15:56:07Z

Thanks for the review @malinajirka !

For this one:

⚠️ Should we replace "7 hours" with a parameter?

I've already covered some of the dynamic texts in the next PR #13605. I mentioned in the PR description that I've used placeholder icons and texts but thanks so much for noticing it 🌟.

Regarding below warnings, Android scan screen specs are not yet ready. I can add temporary dp width for now. Ideally I'd wait for the first set of specs to be shared with us before I start making these refinements. Lmkwyt.

⚠️ We usually name the resources ic_[NAME]_[WIDTH]dp.xml. I think it'd be good to update the names to keep the consistency. Thanks!

⚠️ : We might want to use a resources from colors.xml. If it doesn't exist there, we might want ask Chris if there is any similar color or if we should really introduce a new color.

malinajirka · 2020-12-15T20:58:50Z

Thanks @ashiagr! All LGTM ;)

Regarding below warnings, Android scan screen specs are not yet ready. I can add temporary dp width for now. Ideally I'd wait for the first set of specs to be shared with us before I start making these refinements. Lmkwyt.

Sounds good to me. One thing we might want to consider is adding checkboxes to the parent issue (#13326) so we don't forget about it. Feel free to merge this PR.

…ui-states # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanStatusService.kt

ashiagr · 2020-12-17T09:41:42Z

One thing we might want to consider is adding checkboxes to the parent issue (#13326) so we don't forget about it.

Checkbox added under Scan - Polish the UI:

Fine-tune icons: include dp width in the icon file names, color resources from colors.xml.

Merging is blocked after base change. Re-requesting approval.

malinajirka

LGTM, thanks!

ashiagr added 5 commits December 10, 2020 12:38

Pass site to VM

9b235c1

Introduce recyclerview, adapter, viewholder to contain scan screen vi…

c52fb47

…ew items ScanListItemState will be structured similar to ActivityLogListItem

Include UiState for content in VM

b185e84

Introduce basic elements in the scan state view

a353267

Inject scan service and try to populate basic content

7fcded0

ashiagr added Jetpack Jetpack Mobile labels Dec 10, 2020

ashiagr added this to the 16.5 milestone Dec 10, 2020

ashiagr self-assigned this Dec 10, 2020

ashiagr added the [Status] Not Ready for Merge label Dec 10, 2020

Remove unused strings

3da5709

ashiagr mentioned this pull request Dec 10, 2020

Jetpack Section: Scan #13326

Closed

67 tasks

ashiagr commented Dec 10, 2020

View reviewed changes

ashiagr requested review from ParaskP7, malinajirka and zwarm December 10, 2020 12:59

ashiagr added 2 commits December 10, 2020 19:04

Rename resource name

30d3447

Refactor slightly for more clarity

f4ff13e

ParaskP7 reviewed Dec 11, 2020

View reviewed changes

ashiagr added 6 commits December 11, 2020 17:38

Extract site state related code to another function

01129a8

Use abstract instead of open, remove explicit Long

a576c37

Use full qualifiers for states

0149797

Use actual string res in tools text

f680209

Move isStarted = true closer to if

deb7a70

Fix compile

fddda49

ashiagr marked this pull request as ready for review December 15, 2020 10:44

malinajirka self-assigned this Dec 15, 2020

malinajirka reviewed Dec 15, 2020

View reviewed changes

ashiagr mentioned this pull request Dec 15, 2020

Jetpack Section: Refactor Rewind/Backup/ScanStatusServices #13606

Closed

ashiagr added 2 commits December 15, 2020 20:52

Remove lastScanState variable

ca0c82a

Use ImageManager for loading drawable

6522dff

Move setHasStableIds to adapter itself

a6a7faf

Merge branch 'issue/13326-scan-menu' into issue/13326-scan-introduce-…

48b1a13

…ui-states # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanStatusService.kt

ashiagr mentioned this pull request Dec 16, 2020

Jetpack Mobile: Scan UiStates part(ii) #13605

Merged

3 tasks

Base automatically changed from issue/13326-scan-menu to develop December 17, 2020 08:06

ashiagr removed the [Status] Not Ready for Merge label Dec 17, 2020

ashiagr requested a review from malinajirka December 17, 2020 09:41

malinajirka approved these changes Dec 17, 2020

View reviewed changes

malinajirka merged commit af3f269 into develop Dec 17, 2020

malinajirka deleted the issue/13326-scan-introduce-ui-states branch December 17, 2020 10:27

Conversation

ashiagr commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashiagr Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

ashiagr Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ParaskP7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ParaskP7 Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

ashiagr Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashiagr commented Dec 11, 2020

Uh oh!

ParaskP7 commented Dec 11, 2020

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashiagr commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka commented Dec 15, 2020

Uh oh!

ashiagr commented Dec 17, 2020

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ashiagr commented Dec 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 10, 2020 •

edited

Loading

ashiagr Dec 11, 2020 •

edited

Loading

ParaskP7 Dec 11, 2020 •

edited

Loading

ashiagr commented Dec 15, 2020 •

edited

Loading