Android: Migrate auto-complete to Flow and Coroutines #5123

anikiki · 2024-10-10T13:35:48Z

Task/Issue URL: https://app.asana.com/0/1200581511062568/1208040472309277/f

Description

Removed rxjava from autosuggest.

Steps to test this PR

Smoke test the autosuggest. There should be no user facing changes.

This is the final PR. See scenarios and screenshots in https://app.asana.com/0/1200581511062568/1208264037618741/f

NO UI changes

anikiki · 2024-10-10T13:36:15Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @anikiki and the rest of your teammates on Graphite

CrisBarreiro

Looks good and works as expected. Left a coupe of small comments, but great work! 🎉

Now that we aren't using Rx for autocomplete anymore, can we remove the rxjava dependencies somewhere? I believe we should be able to remove them from at least the history modules

CrisBarreiro · 2024-10-15T08:34:03Z

app/src/main/java/com/duckduckgo/app/autocomplete/api/AutoComplete.kt

-                        .sortedByDescending { it.score }
-                        .map { it.suggestion }
-                }
+        val savedSites = getAutoCompleteBookmarkResults(query)


For simplicity, can we just combine all flows in one single operation?

Thanks for the suggestion 👍 Updated.

CrisBarreiro · 2024-10-15T08:35:29Z

app/src/main/java/com/duckduckgo/app/autocomplete/api/AutoComplete.kt


-        return savedSitesObservable.zipWith(getAutoCompleteSearchResults(query)) { bookmarksAndTabsAndHistory, searchResults ->
-            val topHits = (searchResults + bookmarksAndTabsAndHistory).filter {
+        savedSites.collect { (bookmarksAndFavoritesAndTabsAndHistory, searchResults) ->


Can't we just map instead of collect and emit?

Thanks for the suggestion 👍 Updated.

CrisBarreiro · 2024-10-15T08:37:07Z

app/src/main/java/com/duckduckgo/app/autocomplete/api/AutoComplete.kt

@@ -468,3 +466,15 @@ class AutoCompleteApi @Inject constructor(
        val score: Int = DEFAULT_SCORE,
    )
 }
+
+@VisibleForTesting


Do we need the annotation since this is internal?

The annotation is there to mark the fact that this function should be private, but the visibility was changed for testing purposes. I didn't add what the visibility should have been. By default, if not mentioned, is private (as in this case).

@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)

CrisBarreiro · 2024-10-15T08:38:47Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

@@ -1681,6 +1681,9 @@ class BrowserTabFragment :
                duckPlayerScripts.sendSubscriptionEvent(it.duckPlayerData)
            }
            is Command.SwitchToTab -> {
+                binding.focusedView.gone()


Curious why we need this, since it doesn't seem to be related to the migration

This code was added to cover the scenario where the Switch to tab suggestion matches the current tab. In this case we need to close the suggestions and have the UI in a state similar as when opening a different tab.

…losed suggestions when only one tab is open.

…f flow of mutable list.

…s on each section.

joshliebe · 2024-10-21T21:29:37Z

app/src/test/java/com/duckduckgo/app/systemsearch/SystemSearchViewModelTest.kt

-        val testObserver = TestObserver.create<String>()
-        testee.resultsPublishSubject.subscribe(testObserver)
+//        val testObserver = TestObserver.create<String>()
+//        testee.resultsStateFlow.subscribe(testObserver)


You can delete the commented-out lines in this file

Thanks! Removed :)

Task/Issue URL: https://app.asana.com/0/0/1208581391309609/f ### Description Fixed autocomplete flow and pixels. ### Steps to test this PR Steps to repro: - [x] Install from branch `feature/ana/android_migrate_auto_complete_to_flow_and_coroutines` - [x] Visit a site in a new tab - [x] Open ... menu - [x] Click add bookmark - [x] Observe that autocomplete pixels are firing (they should not): m_autocomplete_displayed_bookmark m_autocomplete_displayed_switch_to_tab Test the fix: - [x] Install from this branch - [x] Visit a site in a new tab - [x] Open ... menu - [x] Click add bookmark - [x] Autocomplete pixels are not firing (check for "m_autocomplete" in logs) ### NO UI changes

…ggest (#5018) Task/Issue URL: https://app.asana.com/0/1200581511062568/1208260875798966/f ### Description The full implementation will span across a few tasks. This is the main task for the implementation of [Android: Include existing open tabs in search autosuggest](https://app.asana.com/0/715106103902962/1208040193300233/f) ### Steps to test this PR See: - #5019 - #5042 - #5063 - #5064 - #5065 - #5105 - #5123 See scenarios and screenshots in https://app.asana.com/0/1200581511062568/1208264037618741/f --------- Co-authored-by: Dax The Translator <[email protected]>

anikiki mentioned this pull request Oct 10, 2024

Address the tasks in the Meta section #5105

Merged

6 tasks

anikiki changed the title ~~Removed rxjava from autosuggest.~~ Android: Migrate auto-complete to Flow and Coroutines Oct 10, 2024

anikiki marked this pull request as ready for review October 10, 2024 13:40

anikiki added the STACKED PR - DO NOT MERGE label Oct 10, 2024

anikiki marked this pull request as draft October 10, 2024 13:41

anikiki force-pushed the feature/ana/address_the_tasks_in_the_meta_section branch from 418720f to cabd77b Compare October 10, 2024 15:29

anikiki force-pushed the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch from 60cab4e to c3e69d7 Compare October 10, 2024 15:29

anikiki force-pushed the feature/ana/address_the_tasks_in_the_meta_section branch from cabd77b to 16012e8 Compare October 10, 2024 15:39

anikiki force-pushed the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch from c3e69d7 to 6ec2529 Compare October 10, 2024 15:40

anikiki force-pushed the feature/ana/address_the_tasks_in_the_meta_section branch from 16012e8 to ed1fee2 Compare October 10, 2024 15:45

anikiki force-pushed the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch from 6ec2529 to 37a6efa Compare October 10, 2024 15:45

anikiki force-pushed the feature/ana/address_the_tasks_in_the_meta_section branch from ed1fee2 to f679db7 Compare October 11, 2024 16:07

anikiki force-pushed the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch 2 times, most recently from e412c4c to b33504d Compare October 11, 2024 20:45

anikiki marked this pull request as ready for review October 14, 2024 11:53

anikiki assigned CrisBarreiro Oct 14, 2024

CrisBarreiro approved these changes Oct 15, 2024

View reviewed changes

anikiki force-pushed the feature/ana/address_the_tasks_in_the_meta_section branch from 4b8c8a2 to e5b9a08 Compare October 18, 2024 12:10

anikiki force-pushed the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch from cc552b6 to 03a8d05 Compare October 18, 2024 12:10

anikiki added 4 commits October 21, 2024 16:14

Removed rxjava from autosuggest.

915f73e

Added dividers for suggestions in system search. Cached flag value. C…

b6bb379

…losed suggestions when only one tab is open.

Fixed tests in SystemSearchViewModelTest file.

05beaac

Updated test.

b65d91e

anikiki added 10 commits October 21, 2024 16:14

Moved url phrase formatting and updated tests.

81aefb9

Updated getAutoCompleteSearchResults to return flow of list instead o…

5e8207f

…f flow of mutable list.

Removed logs.

c2ca650

Removed the quick insert from history search.

4c0e428

Updated autocomplete algo to ensure max 12 results with expected size…

59d7e42

…s on each section.

Aligned icons.

c5d815b

Adjusted paddings.

bb01859

Updated icons.

7fb205e

Removed the rxjava dependency from the history module.

45cea4f

Refactered the autocomplete function in AutoCompleteApi.

7a4e8c7

anikiki force-pushed the feature/ana/address_the_tasks_in_the_meta_section branch from e5b9a08 to bb5d02d Compare October 21, 2024 15:15

anikiki force-pushed the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch from 03a8d05 to 7a4e8c7 Compare October 21, 2024 15:15

This was referenced Oct 21, 2024

Bug: autocomplete pixels firing when Adding Bookmark #5165

Merged

Bug: Unable to delete autocomplete history items #5168

Merged

joshliebe reviewed Oct 21, 2024

View reviewed changes

anikiki and others added 2 commits October 22, 2024 09:20

Removed commented-out lines.

55b2ba0

anikiki merged commit 92090e4 into feature/ana/address_the_tasks_in_the_meta_section Oct 22, 2024
4 checks passed

anikiki deleted the feature/ana/android_migrate_auto_complete_to_flow_and_coroutines branch October 22, 2024 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: Migrate auto-complete to Flow and Coroutines #5123

Android: Migrate auto-complete to Flow and Coroutines #5123

anikiki commented Oct 10, 2024 •

edited

Loading

anikiki commented Oct 10, 2024 •

edited

Loading

CrisBarreiro left a comment

CrisBarreiro Oct 15, 2024

anikiki Oct 18, 2024

CrisBarreiro Oct 15, 2024

anikiki Oct 18, 2024

CrisBarreiro Oct 15, 2024

anikiki Oct 16, 2024

CrisBarreiro Oct 15, 2024

anikiki Oct 16, 2024

joshliebe Oct 21, 2024

anikiki Oct 22, 2024

Android: Migrate auto-complete to Flow and Coroutines #5123

Android: Migrate auto-complete to Flow and Coroutines #5123

Conversation

anikiki commented Oct 10, 2024 • edited Loading

Description

Steps to test this PR

NO UI changes

anikiki commented Oct 10, 2024 • edited Loading

CrisBarreiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anikiki commented Oct 10, 2024 •

edited

Loading

anikiki commented Oct 10, 2024 •

edited

Loading