For #22534 - Show history highlights and groups in "Recently visited"

[Mugurell]

For now the metadata groups don't support scoring so as an interim solution we
will show up to 9 items, evenly distributes, first favoring groups sorted by date
then history highlights pre-sorted by default.

Tapping a history highlight will switch to it's already open tab if available
or create a new one in which to load it if needed.

A "Remove" option will also be available for history highlights to remove it from
the screen and also from history.
Currently removing a group / highlight will not query new ones to again show up to
9 items, this will be implemented separately.

The updated feature supports more than history metadata so updating the overall
naming scheme seems needed.
To signal that this is a homescreen feature the entire package is moved to home.

RecentVisitsOnHomescreen.mp4

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

[gabrielluong]

Suggested change

	private fun RecentlyVisitedHighlight(
	private fun RecentlyVisitedHistoryHighlight(

I don't know if this adds clarity, maybe?

[Mugurell]

Inside the same same file I'd say the impact is small but this aligns the naming to the other RecentlyVisitedHistoryGroup composable. Thanks!

[gabrielluong]

Suggested change

	* @param modifier [Modifier] allowing to perfectly place this.
	* @param modifier Optional [Modifier] to be applied to the layout.

Using one of your previous kDoc description so we hopefully don't keep rewording this differently everywhere.

[Mugurell]

I would't say this is optional.
Thought about even not providing a default here to force users pass a Modifier.
Saw there are differences in how this composable should be placed depending on which type of history is shown so while a Modifier could control a lot of different options that comment should hint to the users that it is intended to help place this appropriately.

[gabrielluong]

Suggested change

	* @param modifier [Modifier] allowing to perfectly place this.
	* @param modifier Optional [Modifier] to be applied to the layout.

Using one of your previous kDoc description so we hopefully don't keep rewording this differently everywhere.

[csadilek]

@Mugurell another thing here is that we can't land ship this without some form of removal. I know the longer term plan is being discussed (likely via a block list or similar), but short-term we can do the same as we do for groups and simply show a remove option and delete all metadata for that highlight/url.

So, for groups we call: https://github.com/mozilla-mobile/android-components/blob/main/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt#L320

And we'd add a similar call which we'd invoke when removing highlights.

[csadilek]

@Mugurell opened a PR to add this removal API we should use in the interim for deleting highlights: mozilla-mobile/android-components#11307 (Talked to Nicole as well, we can talk about a better removal/delete approach separately.)

[mergify]

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

[gabrielluong]

Will do my final pass tomorrow.

[Mugurell]

After syncing with Christian I've updated the PR with sorting highlights and groups together.
Tested and it works but there is still a bit of cleanup needed and have to also add unit tests.

[csadilek]

Algorithm and feature logic look good to me! I left a few suggestions / nits for naming I would prefer updating before landing.

I think this can land once cleanup is addressed and tests are added, if @gabrielluong is fine with the UI bits!

[gabrielluong]

Was told to wait until tomorrow for my review to see the final state of the changes. Otherwise, I was also informed that we should wait to land on Monday so that nothing breaks on the weekend.

[mergify]

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

[mergify]

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

[csadilek]

This looks good to me now! I also did some more manual testing and everything worked as expected.

@gabrielluong can you take a look as well esp. at the compose bits and refactorings. Unless you spot a blocker I'd say let's land this to get some bake time in Nightly.

[csadilek]

nit: its.

[csadilek]

nit: will.

[gabrielluong]

Hmm I don't think this is about RecentBookmarks, but I am not gonna block this from landing since you're on holiday.

[gabrielluong]

Looking back at this entire review, I think I would recommend breaking out some of the commits into their own PR next time. For example, the renaming refactor could've been in a separate PR that landed ahead and outside the feature. One of the final checks I was doing (as I am writing this) was to see if the refactoring preserved the git history since there were also additional refactoring on top of the renaming, which made it appear as a completely new file on the GitHub UI. For context, there were some menu item changes in RecentlyVisited that I am aware of that needs to be corrected and losing that history would be a bit of an inconvenience.

[gabrielluong]

I would revert these to going back to @see RecentVisitsInteractor.onHistoryShowAllClicked in another PR since the content are the same. That way we can just focus on changing it once.