Issue/11119 search in set parent page

[zwarm]

Fixes #: #11119

Description:
This PR adds search capabilities to the Set Parent page. It is 95% complete. Following along with the
pattern in Site Pages, this implementation separates the search into a PageParentSearchFragment
and PageParentSearchVieModel, both of which have been added to the necc. dagger modules. A new layout file (page_parent_fragment.xml) was introduced for PageParentFragment to hold the FrameLayout for the search and page_parent_menu.xml was expanded to hold a new SearchView. The searching takes advantage of LiveData, ViewModels and observability to provide the user with a clean UI experience.

While this PR is 95% complete, there are still a few lingering questions that may require action.
1 - Does the text need to be adjusted when showing the suggestion or empty results? Right now that
text is shared as ...
No pages matching your search
Search pages
If changes are necessary, two new string would be required + any language translations required.

2 - As of this PR submittal, the search is locally performed on the parent page options during page load. It is unclear if the search needs to be performed against the database each and every time. If so, then the search logic will need to be updated to reflect that.

Testing instructions:
Test starting prompt is shown

• Navigate to Site pages
• Tap on a action menu (3 vertical dots) for a page and Select "Set Parent"
• Tap on the search icon
• "Search hint" is available on the toolbar
• "Search pages" is visible in the page_parent_fragment

Test search text found

• Navigate to Site pages
• Tap on a action menu for a page and Select "Set Parent"
• Tap on the search icon
• Enter search text (jot dowan a title snippet from the pages view & search for that)
• A list of possible parent pages is shown

Test search text found

• Navigate to Site pages
• Tap on a action menu for a page and Select "Set Parent"
• Tap on the search icon
• Enter jibberish as the search text
• "No pages matching your search" is shown

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.

[peril-wordpress-mobile]

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

Generated by 🚫 dangerJS

[malinajirka]

Thanks @zwarm, great job;)!

I've reviewed the code and tested the app and I encountered a couple of issues.

1. Search state gets lost on configuration change

• Open Page list
• Click on Set Page
• Click on Search icon in the Toolbar
• Enter a query
• Rotate the device and observe the state gets lost

2. Save button state gets lost on configuration change (This might not be related to this PR. In case it isn't related, please create a new issue on GitHub)

• Open Page list
• Click on Set Page
• Select one of the pages -> notice "Save" button appears
• Click on Search icon
• Click on the Up button
• Notice "Save" button is invisible even though the page is still selected

---

this implementation separates the search into a PageParentSearchFragment
and PageParentSearchVieModel

Have you considered filtering the items directly in PageParentViewModel/PageParentFragment? I'm not saying the approach you took isn't good. I understand you decided to prefer consistency. I'd just love to know your thoughts about it.

[zwarm]

@malinajirka

Have you considered filtering the items directly in PageParentViewModel/PageParentFragment? I'm not saying the approach you took isn't good. I understand you decided to prefer consistency. I'd just love to know your thoughts about it.

Yes I did, I had coded both solutions and in the end decided to PR this one to maintain consistency. I can just as easily kill this PR & submit the other instead. lmk

[zwarm]

I need some clarification around expected behavior upon searching .... Thanks!

Save button state gets lost on configuration change (This might not be related to this PR. In case it isn't related, please create a new issue on GitHub)
Open Page list
Click on Set Page
Select one of the pages -> notice "Save" button appears
Click on Search icon
Click on the Up button
Notice "Save" button is invisible even though the page is still selected

Scenario 1

• Tap "set parent" to show view
• Initial parent is shown as selected => Save is GONE
• Select a different parent => Save is VISIBLE
• Tap search icon => Should Save be VISIBLE or GONE during search?
• Tap UP => Save should be VISIBLE

Scenario 2

• Tap "set parent" to show view
• Initial parent is shown as selected => Save is GONE
• Tap search icon => Save is GONE
• Tap UP => Save should be GONE

Scenario 3

• Tap "set parent" to show view
• Initial parent is shown as selected => Save is GONE
• Tap search icon => Save is GONE
• Enter search text => Save is GONE
• Select a parent => Save is VISIBLE
• "X" is tapped to clear search
  -- What happens here? Is the selected item cleared? Is Save visible or Gone
• Tap UP => Should save be visible or not?

Scenario 4

• Tap "set parent" to show view
• Initial parent is shown as selected => Save is GONE
• Tap search icon => Save is GONE
• Enter search text => Save is GONE
• "X" is tapped to clear search
• Tap back => Save is GONE

Scenario 5 (Happens in production)

• Tap "set parent" to show view
• Initial parent is shown as selected => Save is GONE
• Tap item to select as parent => Save is VISIBLE
• Tap initial parent again => Save is still VISIBLE (I would think that save should be gone if selection matches initial value)

[malinajirka]

Thanks for all the improvements @zwarm ! The changes look great. I haven't tested the code as I wasn't sure if you plan to make changes to the Save button behavior.

Yes I did, I had coded both solutions and in the end decided to PR this one to maintain consistency. I can just as easily kill this PR & submit the other instead. lmk

Ohh nice ;). I don't want you to replace the solution, I like it. I was just curious how did you make the decision. It's all clear now :). Thanks

---

Thanks for listing the different scenarios! ;)

Scenario #1 - Should Save be VISIBLE or GONE during search?

I think Save should be gone.

Scenario #3 -- What happens here? Is the selected item cleared? Is Save visible or Gone

I think Save should be gone.

Scenario #3 Tap UP => Should save be visible or not?

I think Save should be gone.

Scenario #5 Tap initial parent again => Save is still VISIBLE (I would think that save should be gone if selection matches initial value)

I agree with you. Could you please fill a bug report. Thanks!

To summarize it. I think Save should be visible when a non-initial item is selected and visible on the screen (Save should be also visible when the selected item is not visible due to a different scroll position).

---

SideNote:
A couple of tips on how to make the PR easier to review

• In general, we prefer to leave all the in-code discussions unresolved and let the reviewer resolve them. It's a bit cumbersome when the reviewer needs to open all the resolved discussions to double check they have been resolved as they expected and the fixes don't contain any bugs.
• I believe it's really useful when you leave a commit hash of the fix in the reply to an in-code comment -> example Offline Pages : Added Support for unpublished revision to Page List  #11228 (comment). (If you just paste a plain commit hash, GitHub will turn it into a short-hash with a link.)

Wdyt?

[zwarm]

@malinajirka - Still have one open question regarding "text" ... leave as is or needs to be something custom?

1 - Does the text need to be adjusted when showing the suggestion or empty results? Right now that
text is shared as ...
No pages matching your search
Search pages
If changes are necessary, two new string would be required + any language translations required.

[zwarm]

SideNote:
A couple of tips on how to make the PR easier to review

In general, we prefer to leave all the in-code discussions unresolved and let the reviewer resolve them. It's a bit cumbersome when the reviewer needs to open all the resolved discussions to double check they have been resolved as they expected and the fixes don't contain any bugs.
I believe it's really useful when you leave a commit hash of the fix in the reply to an in-code comment -> example #11228 (comment). (If you just paste a plain commit hash, GitHub will turn it into a short-hash with a link.)

👍 - thanks for letting me know, will definitely follow this guideline from this point forward.

This will be adjusted as part of the Save button visibility changes for searching - can't really avoid fixing it if the logic is changing. Do you still want a separate ticket for it?
Scenario #5 Tap initial parent again => Save is still VISIBLE (I would think that save should be gone if selection matches initial value)

I haven't tested the code as I wasn't sure if you plan to make changes to the Save button behavior.
-Good idea because I haven't made the Save button behavior changes yet.

Thanks for all the comments. :)

[malinajirka]

Still have one open question regarding "text" ... leave as is or needs to be something custom?

Ohh sorry, I forgot about that. I've discussed it with a designer and it's ok to keep it as it is.

This will be adjusted as part of the Save button visibility changes for searching - can't really avoid fixing it if the logic is changing. Do you still want a separate ticket for it?

Nope, no need to create a separate ticket. If you expect it'll require more changes, feel free to open a new PR against this PR.

[zwarm]

@malinajirka
Updated the save button behavior in e9b4991

I think should take care of everything you wanted done. Please let me know if there is a scenario I have not accounted for. Thanks.

For quick reference, this is what was implemented for the behavior

Scenario 1
Tap "set parent" to show view
Initial parent is shown as selected => Save is GONE
Select a different parent => Save is VISIBLE
Tap search icon => Save is GONE
Tap UP => Save is VISIBLE

Scenario 2
Tap "set parent" to show view
Initial parent is shown as selected => Save is GONE
Tap search icon => Save is GONE
Tap UP => Save is GONE

Scenario 3
Tap "set parent" to show view
Initial parent is shown as selected => Save is GONE
Tap search icon => Save is GONE
Enter search text => Save is GONE
Select a parent => Save is VISIBLE
"X" is tapped to clear search => Save is GONE
Tap UP => SAVE is VISIBLE

Scenario 4
Tap "set parent" to show view
Initial parent is shown as selected => Save is GONE
Tap search icon => Save is GONE
Enter search text => Save is GONE
"X" is tapped to clear search
Tap UP => Save is GONE

**Scenario 5 **
Tap "set parent" to show view
Initial parent is shown as selected => Save is GONE
Tap item to select as parent => Save is VISIBLE
Tap initial parent again => Save is GONE

**Note: If a value is selected while in search, that value remains selected when search is closed.

[malinajirka]

Thanks for all the changes @zwarm! The code looks good. I've two more scenarios about which I'm not sure about. Let me know what you think about them:).

1. Select a parent (eg "Test page")
2. Tap on search
3. Type "Test"
4. Notice "Test Page" is visible and selected, but the SAVE button isn't (Should it be selected? Should the Save button be visible? Wdyt?)

---

1. Tap on search
2. Type something so there is at least one result
3. Rotate your device
4. Click on "Clear search"
5. Notice the results didn't disappear

I don't expect a real user will encounter either of the scenarios, wdyt?

[zwarm]

Select a parent (eg "Test page")
Tap on search
Type "Test"
Notice "Test Page" is visible and selected, but the SAVE button isn't (Should it be selected? Should the Save button be visible? Wdyt?)

Tap on search
Type something so there is at least one result
Rotate your device
Click on "Clear search"
Notice the results didn't disappear
I don't expect a real user will encounter either of the scenarios, wdyt?

@malinajirka - 1) - If an item is selected, it should stay selected. I can go either way on the save button. It sounds like that would be a designer/behavior decision. 2) Not a scenario I had thought of, but if you did then it's absolutely possible that customer may.

[malinajirka]

Thanks for the reply ;)!

#1 - If an item is selected, it should stay selected. I can go either way on the save button. It sounds like that would be a designer/behavior decision. #2 Not a scenario I had thought of, but if you did then it's absolutely possible that customer may.

I don't wont to delay the trial on such minor issues about which we are not even sure how to proceed. Could you please create GitHub issues for them. I'll add a low priority label to both of them and "needs design" label to the second one. Thank you 🙇

The only remaining thing before I merge this PR is this comment - https://github.com/wordpress-mobile/WordPress-Android/pull/11269/files#r377068726. I'd rather avoid changing the Project.xml, wdyt?

SideNot:
Btw using #[number] syntax is reserved for referencing other issues/PRs on GitHub. Since issues with numbers 1-5 are quite old, it doesn't really matter. But I'd rather avoid using this syntax anyway as it redirects the user to a completely unrelated issue. Thanks!

[zwarm]

Oops on the "pound" sign - didn't even cross my mind it would link out. I edited my post. I see the other comment, I'll revert it back.

[zwarm]

@malinajirka #d4000c0 Reverted back the .idea/codeStyles/Project.xml file to the commit before mine.

Obviously I needed to make a new commit reverting the file back to that state. I can’t just change history because it’s already been pushed. You know, editing history throws off everyone else’s stuff.

[malinajirka]

Oops on the "pound" sign - didn't even cross my mind it would link out. I edited my post. I see the other comment, I'll revert it back.

No worries ;)

#d4000c0 Reverted back the .idea/codeStyles/Project.xml file to the commit before mine.
Obviously I needed to make a new commit reverting the file back to that state. I can’t just change history because it’s already been pushed. You know, editing history throws off everyone else’s stuff.

👍 Yeah, revert is fine. Thanks!

LGTM 🚢