For #21045 - Categories support #21104

Mugurell · 2021-09-01T13:13:23Z

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

Mugurell · 2021-09-01T13:14:59Z

~~Based my work on the not yet merge #21043 commit, that's why I still have it in history. Will rebase after that is merged.~~ Rebased. Only the expected 3 commits ready for review.

~~Still need to find the most efficient way to filter items.~~
Found a clean way to get the number of stories we need from each category for filtering.
Calculating only the counts in one step and then actually creating the items list result in another allows to order or group the results by category properties (like oldest selected) or item properties.

Mugurell · 2021-09-01T13:19:40Z

app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt

@@ -341,7 +341,13 @@ class SessionControlAdapter(

    override fun onViewRecycled(holder: RecyclerView.ViewHolder) {
        when (holder) {
-            is PocketStoriesViewHolder -> holder.composeView.disposeComposition()


I was seeing as much as 300ms needed for composeView.onAttachedToWindow everytime the holder was scrolled off and on screen again.
I don't think we need the composable destroyed while the user is still on homescreen and keeping it around definitely means a smoother UX.

Mugurell · 2021-09-01T13:21:40Z

...in/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt

+            }
+        }
+
+        // Populate each row with as many items as possible combining wider with narrower items.


Without this there would be a lot of unused space.

...in/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt

app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt

pocmo

First pass. Haven't read all of it yet. Especially the grid code not yet.

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

pocmo · 2021-09-15T11:27:33Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+            }
+        }
+
+        homeFragmentStore.dispatch(HomeFragmentAction.PocketStoriesCategoriesChange(updatedCategories))


The actual creation of the new state (the new categories) should preferably happen in the reducer. That way you are guaranteed to always end up with a consistent state. Imagine the processing being slow and the user clicking categories fast: You may generate states here while the new state is not available in the store yet, which will cause inconsistent states. This doesn't not happen in the reducer where the actions will be processed sequentially.

Remembered that I tried that first but the reducer seemed a bit big and it seemed to now contain the logic on for how to handle categories being clicked so I then moved that code in composable's parent - this fragment.
#21104 (comment)

I think reducers and state updates happen only on the main thread?
If so, instead of me launching coroutines to calculate the updated state and dispatch that I could have all this running on main.
That would avoid reducers becoming too complex while the new state can be calculated a bit closer to the user interaction.

I think actions like "SelectCategory" or "DeselectCategory" make totally sense to me - and I don't think it's a problem if the reducer is complex, if the calculation of the new state is complex (as long it's still a function that just transforms input into an output without side effects). If I remember correctly the first approach had something like a "Clicked" action and that felt wrong because that is too close to the UI layer, the fragment handles the click and would dispatch an actual state changing action (not an UI event like "click").

pocmo · 2021-09-15T11:28:47Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+ * This only impacts filtered results guaranteeing an even spread of stories from each category.
+ */
+@VisibleForTesting
+internal fun getFilteredStories(


It looks like those helpers here are independent from the HomeFragment class. Maybe we can move them somewhere else. There's a high risk that the HomeFragment will grow a lot. :)

Maybe into a Controller 😅?
Was thinking about an intermediate big interface for all domain logic needed for home composables, something like HomeComposablesLogic?(please suggest a better name) with a default implementation that can be delegated to by the HomeFragment?
This just while migrating current views to composables, though even after fully migrating this screen to Compose, if bringing back all this logic in one place will have that again being immense?

I don't have a strong opinion on the naming. It just seems like some utility functions (or maybe they could even be extension functions?) that could move to some related utilities file. Just to keep the fragment focused.

Mugurell · 2021-09-17T09:44:29Z

Sorry for not keeping this up-to-date with main.
Rebased now.
Don't really know where to put that code from HomeFragment though.

pocmo

Approving, but think the actual state creation should move into the reducer - that's it's job. :)

mergify · 2021-09-22T14:26:25Z

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

Mugurell · 2021-09-22T14:59:32Z

created a new HomeFragmentState.getFilteredStories() that could be called from anybody with access to the state, incl the fragment or the reducer
HomeFragment will now emit SelectPocketStoriesCategory and DeselectPocketStoriesCategory for the reducer to actually update the selection in BrowserState.

Mugurell · 2021-09-22T15:43:35Z

 SUITE: org.mozilla.fenix.share.ShareControllerTest
   TEST: handleShareToApp should start a new sharing activity and close this
     I/glean/Dispatchers: Task queued for execution and delayed until flushed
   FAILURE
 io.mockk.MockKException: Can't instantiate proxy for class kotlin.Function1

Restarting CI.

mergify · 2021-09-22T21:34:45Z

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

- stick to one naming scheme: rename articles to stories and use this all throughout the app. - add some spacing above the new section (as per the current design)

pocmo · 2021-09-27T07:58:28Z

app/src/main/java/org/mozilla/fenix/home/PocketUpdatesMiddleware.kt

+                        context.state.getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT)
+                    )
+                )
+            }


I don't really understand the need for this middleware. This could happen directly in the reducer for those actions? 🤔

pocmo · 2021-09-27T08:04:03Z

...in/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt

-                    PocketStory(item, client)
+                    key(item.title) {
+                        PocketStory(item, client)
+                    }


Why do we use key() here? Is it to avoid recomposition of PocketStory unless the title changes? Why the title only though? 🤔

The title seemed like a small thing to check and enough to protect against recompositions. The stories are static in nature with no property changing.
Tested a bit now, don't see any recomposition. Same calls with or without key. Will remove it.

pocmo · 2021-09-27T08:06:11Z

...ain/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesController.kt

+        oldestCategoryToDeselect?.let {
+            homeStore.dispatch(HomeFragmentAction.DeselectPocketStoriesCategory(it.name))
+        }
+


Still think this should happen in the reducer since you are calculating a new state based on the current state here.

Mugurell · 2021-09-27T09:32:13Z

The review request from mozilla-mobile/performance was from me suppressing penaltyDeath. Updated the PR to remove that change.

Mugurell requested review from a team as code owners September 1, 2021 13:13

Mugurell marked this pull request as draft September 1, 2021 13:16

Mugurell commented Sep 1, 2021

View reviewed changes

Mugurell marked this pull request as ready for review September 2, 2021 09:42

Mugurell requested review from csadilek and pocmo September 2, 2021 09:42

Mugurell marked this pull request as draft September 6, 2021 13:59

Mugurell mentioned this pull request Sep 14, 2021

For #21275 - Sort items by how many times they were actually shown #21277

Merged

3 tasks

pocmo reviewed Sep 15, 2021

View reviewed changes

Mugurell marked this pull request as ready for review September 17, 2021 09:44

pocmo approved these changes Sep 21, 2021

View reviewed changes

Mugurell requested a review from pocmo September 22, 2021 15:15

Mugurell closed this Sep 22, 2021

Mugurell reopened this Sep 22, 2021

For #21045: Cleanup

144422c

- stick to one naming scheme: rename articles to stories and use this all throughout the app. - add some spacing above the new section (as per the current design)

pocmo reviewed Sep 27, 2021

View reviewed changes

Mugurell requested a review from a team as a code owner September 27, 2021 09:29

For #21045: Add categories support

ca8a598

Mugurell added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Sep 27, 2021

mergify bot merged commit ba4c44a into mozilla-mobile:main Sep 27, 2021

Mugurell deleted the 21045 branch September 27, 2021 10:00

Mugurell mentioned this pull request Sep 27, 2021

For #21391 - Updated composables #21392

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #21045 - Categories support #21104

For #21045 - Categories support #21104

Mugurell commented Sep 1, 2021

Mugurell commented Sep 1, 2021 •

edited

Mugurell Sep 1, 2021 •

edited

Mugurell Sep 1, 2021

pocmo left a comment

pocmo Sep 15, 2021

Mugurell Sep 17, 2021

Mugurell Sep 20, 2021

pocmo Sep 21, 2021

pocmo Sep 15, 2021

Mugurell Sep 17, 2021

pocmo Sep 21, 2021

Mugurell commented Sep 17, 2021 •

edited

pocmo left a comment

mergify bot commented Sep 22, 2021

Mugurell commented Sep 22, 2021

Mugurell commented Sep 22, 2021

mergify bot commented Sep 22, 2021

pocmo Sep 27, 2021

pocmo Sep 27, 2021

Mugurell Sep 27, 2021

pocmo Sep 27, 2021

Mugurell commented Sep 27, 2021

For #21045 - Categories support #21104

For #21045 - Categories support #21104

Conversation

Mugurell commented Sep 1, 2021

Pull Request checklist

To download an APK when reviewing a PR:

Mugurell commented Sep 1, 2021 • edited

Mugurell Sep 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocmo 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

Mugurell commented Sep 17, 2021 • edited

pocmo left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 22, 2021

Mugurell commented Sep 22, 2021

Mugurell commented Sep 22, 2021

mergify bot commented Sep 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

Mugurell commented Sep 27, 2021

Mugurell commented Sep 1, 2021 •

edited

Mugurell Sep 1, 2021 •

edited

Mugurell commented Sep 17, 2021 •

edited