For #21900 - Converted the Tabs Tray Synced Tabs to Compose #22798

MozillaNoah · 2021-12-11T00:01:05Z

Fixes #21900

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:

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

app/src/main/java/org/mozilla/fenix/compose/Text.kt

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsComposables.kt

gabrielluong · 2021-12-11T00:57:33Z

Apologizes for the early drive by. I couldn't help myself to admiring some new Compose code.

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsComposables.kt

mergify · 2021-12-14T19:03:10Z

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

mergify · 2021-12-15T13:06:18Z

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

app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt

app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

jonalmeida · 2021-12-15T18:22:00Z

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

jonalmeida

Great! Just some nits and questions, but it looks good.

I'll leave the final review for gl though since I only focused on Synced Tabs feature and not the Compose code.

app/src/test/java/org/mozilla/fenix/tabstray/FloatingActionButtonBindingTest.kt

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsListItem.kt

jonalmeida · 2021-12-20T05:24:50Z

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

+ */
+@Composable
+fun SyncedTabsDeviceItem(deviceName: String) {
+    PrimaryText(


nit: in the designs, we needed to add a horizontal line above the device name. I think it's fine to leave out if you're planning on picking up #19942 next.

Good catch! I noticed that without sticky headers it did become hard to tell one section from the other. My intent was to immediately do the sticky header issue next.

MozillaNoah · 2021-12-20T22:47:57Z

Most recent push was to fix some of the previews. Fun fact: the Compose previews don't like using stuff in the Components class, so the preview builds error out since Components is dependent on FenixApplication. So the most recent changes were to move the URL parsing to make URLs text-friendly out of the Composables and into one of the helper functions.

mergify · 2021-12-22T08:15:55Z

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

app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt

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

app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt

app/src/main/java/org/mozilla/fenix/sync/ext/ErrorType.kt

gabrielluong · 2022-01-12T19:21:36Z

app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayStoreReducerTest.kt

@@ -69,4 +70,18 @@ class TabsTrayStoreReducerTest {

        assertEquals(expectedState, resultState)
    }
+
+    @Test
+    fun `WHEN UpdateSyncedTabs THEN synced tabs are added`() {


@jonalmeida Hm.. it's actually interesting to see that we have a Reducer and Store test separately. Usually I see we use the Store test to test the underlying Reducer. Is there any benefit from having them separate like this? I can imagine we end up picking and choosing which actions are dispatched and resulting state in different files.

I missed this comment.

What does the store add that needs to be tested? In most of the stores, the store doesn't do anything for us within the structure. The logic is within the state/reducer. Put state in, reduce, test state out.

The store holds an executor and provides single access to the state and to dispatch actions. Nothing worth testing in tabs tray.

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

gabrielluong · 2022-01-21T21:04:18Z

app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt

@@ -76,30 +70,38 @@ class TrayPagerAdapter(
            }
            PrivateBrowserPageViewHolder.LAYOUT_ID -> {
                PrivateBrowserPageViewHolder(
-                    itemView,
+                    LayoutInflater.from(parent.context).inflate(viewType, parent, false),
                    tabsTrayStore,
                    browserStore,
                    interactor
                )
            }
            SyncedTabsPageViewHolder.LAYOUT_ID -> {
                SyncedTabsPageViewHolder(


I am wondering if we need to do something similar to ComposeViewHolder to avoid #22383

gabrielluong · 2022-01-21T21:08:33Z

app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt

+        when (viewHolder) {
+            is NormalBrowserPageViewHolder -> viewHolder.bind(normalAdapter)
+            is PrivateBrowserPageViewHolder -> viewHolder.bind(privateAdapter)
+            is SyncedTabsPageViewHolder -> viewHolder.bind()


It might be worthwhile to align more closely with SessionControlAdapter. https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt#361

Also wondering if we need to do something like https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt#293.

align with which part? not needing bind() call?

Ya not needing to call bind(), I don't think we need to do this in the current PR. This can be a followup.

Reading back through this, we also may not even need to file a separate follow-up, since the real follow-up is converting the rest of TabsTray to Compose. I'm happy to still do so though, since the timeline for the rest of the stuff is fluctuating.

app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt

gabrielluong · 2022-01-21T21:23:18Z

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

@@ -191,7 +191,7 @@
    <!-- Browser menu toggle that installs a Progressive Web App shortcut to the site on the device home screen. -->
    <string name="browser_menu_install_on_homescreen">Install</string>
    <!-- Menu option on the toolbar that takes you to synced tabs page-->
-    <string name="synced_tabs">Synced tabs</string>
+    <string name="synced_tabs" moz:removedIn="98" tools:ignore="UnusedResources">Synced tabs</string>


Just wondering about this removal. Are we removing view_synced_tabs_title because it is never used?

Yes. Based on some context clues of git annotations, I am under the impression that Sync previously lived under its own screen + screen title, and the layout + string are leftover from that era after synced tabs were moved to the tabs tray.

gabrielluong · 2022-01-21T21:28:20Z

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsIntegration.kt

+            accountManager = accountManager,
+            view = this,
+            lifecycleOwner = lifecycleOwner,
+            onTabClicked = {


Maybe file something for AC about cleaning this up.

I'd assume it'd be a part of this mozilla-mobile/android-components#11552

gabrielluong · 2022-01-21T21:30:23Z

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsIntegration.kt

+        SyncButtonBinding(store) { listener?.onRefresh() }
+    }
+
+    override var listener: SyncedTabsView.Listener? = null


This is a followup. Same as above about cleaning this up in AC. I would verify with @jonalmeida and make sure there's no other consumers here.

While I am here, is there an actual listener that is attached to this?

Yes, this is some of the weirdness of the Observable<SyncedTabsView.Listener> by ObserverRegistry(). From my understanding it's initialized at runtime but none of that should be necessary, hopefully, after mozilla-mobile/android-components#11552.

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsIntegration.kt

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

MozillaNoah · 2022-02-07T21:45:49Z

Most recent push simply increases the spacing between the error text and the error button from 8dp to 12dp.

MozillaNoah · 2022-02-07T22:23:56Z

Push to update from main to fix colors + build issue

mergify · 2022-02-08T12:46:27Z

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

gabrielluong · 2022-02-14T16:08:02Z

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

+                onClick = onClick
+            )
+            .padding(horizontal = 16.dp)
+            .defaultMinSize(minHeight = 56.dp),


General question outside of the review, I am wondering how we decide to uses defaultMinSize and height(IntrinsicSize.Min) (in general, nothing in particular about the current code).

I used defaultMinSize to be a more direct 1:1 to android:minHeight in XML (to help ensure with a default text size that there would be enough padding), and the height(IntrinsicSize.Min) was used when I was specifically trying out random things to get the dashed border for the error UI to show. The documentation for Modifier.height() and Modifier.defaultMinSize() aren't the clearest though on when to use either.

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt

mergify · 2022-02-14T18:44:34Z

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

gabrielluong reviewed Dec 11, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/compose/Text.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Dec 11, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsComposables.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Dec 11, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsComposables.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Dec 11, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsComposables.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Dec 11, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsComposables.kt Outdated Show resolved Hide resolved

gabrielluong added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Dec 15, 2021

jonalmeida reviewed Dec 15, 2021

View reviewed changes

gabrielluong reviewed Dec 15, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

MozillaNoah changed the title ~~WIP - For #21900 - Converted the Tabs Tray Synced Tabs to Compose~~ For #21900 - Converted the Tabs Tray Synced Tabs to Compose Dec 16, 2021

MozillaNoah marked this pull request as ready for review December 16, 2021 21:51

MozillaNoah requested review from a team as code owners December 16, 2021 21:51

MozillaNoah requested review from jonalmeida and gabrielluong December 17, 2021 17:38

jonalmeida reviewed Dec 20, 2021

View reviewed changes

gabrielluong removed the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Jan 12, 2022

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/sync/ext/ErrorType.kt Show resolved Hide resolved

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 12, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 21, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 21, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 21, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabsIntegration.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 21, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Jan 21, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

MozillaNoah added the needs:UX-review label Jan 21, 2022

gabrielluong self-assigned this Jan 21, 2022

MozillaNoah requested a review from cwzilla February 4, 2022 19:20

MozillaNoah requested a review from gabrielluong February 10, 2022 22:56

MozillaNoah removed the needs:UX-review label Feb 10, 2022

gabrielluong reviewed Feb 14, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Feb 14, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Feb 14, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/tabstray/syncedtabs/SyncedTabs.kt Outdated Show resolved Hide resolved

gabrielluong approved these changes Feb 14, 2022

View reviewed changes

MozillaNoah added 2 commits February 14, 2022 14:40

For #21900 - Delete files from Synced Tabs XML implementation

ad4780b

For #21900 - Delete files from Synced Tabs XML implementation

5d66a4c

MozillaNoah added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Feb 14, 2022

MozillaNoah closed this Feb 14, 2022

MozillaNoah reopened this Feb 14, 2022

mergify bot merged commit 48141b2 into mozilla-mobile:main Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #21900 - Converted the Tabs Tray Synced Tabs to Compose #22798

For #21900 - Converted the Tabs Tray Synced Tabs to Compose #22798

MozillaNoah commented Dec 11, 2021 •

edited

Loading

gabrielluong commented Dec 11, 2021 •

edited

Loading

mergify bot commented Dec 14, 2021

mergify bot commented Dec 15, 2021

jonalmeida commented Dec 15, 2021

jonalmeida left a comment •

edited

Loading

jonalmeida Dec 20, 2021

MozillaNoah Dec 20, 2021

MozillaNoah commented Dec 20, 2021 •

edited

Loading

mergify bot commented Dec 22, 2021

gabrielluong Jan 12, 2022

jonalmeida Jan 15, 2022

gabrielluong Jan 21, 2022

gabrielluong Jan 21, 2022

MozillaNoah Jan 21, 2022

gabrielluong Jan 21, 2022 •

edited

Loading

MozillaNoah Jan 24, 2022 •

edited

Loading

gabrielluong Jan 21, 2022

MozillaNoah Jan 21, 2022

gabrielluong Jan 21, 2022

MozillaNoah Jan 21, 2022

gabrielluong Jan 21, 2022

gabrielluong Jan 21, 2022

MozillaNoah Jan 21, 2022

MozillaNoah commented Feb 7, 2022 •

edited

Loading

MozillaNoah commented Feb 7, 2022

mergify bot commented Feb 8, 2022

gabrielluong Feb 14, 2022

MozillaNoah Feb 14, 2022

mergify bot commented Feb 14, 2022

For #21900 - Converted the Tabs Tray Synced Tabs to Compose #22798

For #21900 - Converted the Tabs Tray Synced Tabs to Compose #22798

Conversation

MozillaNoah commented Dec 11, 2021 • edited Loading

Pull Request checklist

To download an APK when reviewing a PR:

gabrielluong commented Dec 11, 2021 • edited Loading

mergify bot commented Dec 14, 2021

mergify bot commented Dec 15, 2021

jonalmeida commented Dec 15, 2021

jonalmeida left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MozillaNoah commented Dec 20, 2021 • edited Loading

mergify bot commented Dec 22, 2021

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

gabrielluong Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

MozillaNoah Jan 24, 2022 • edited Loading

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

MozillaNoah commented Feb 7, 2022 • edited Loading

MozillaNoah commented Feb 7, 2022

mergify bot commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 14, 2022

MozillaNoah commented Dec 11, 2021 •

edited

Loading

gabrielluong commented Dec 11, 2021 •

edited

Loading

jonalmeida left a comment •

edited

Loading

MozillaNoah commented Dec 20, 2021 •

edited

Loading

gabrielluong Jan 21, 2022 •

edited

Loading

MozillaNoah Jan 24, 2022 •

edited

Loading

MozillaNoah commented Feb 7, 2022 •

edited

Loading