Bug 1876594 - Refactor ThumbnailStorage to be an implicit dependency

[MozillaNoah]

A change to ThumbnailStorage was breaking previews anywhere the ThumbnailImage composable was being used.

Until we can figure out a long term strategy for properly handling our components singleton within previews (which do not have the application context), we'll simply have an inComposePreview check within ThumbnailImage

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

1. Click on Checks at the top of the PR page.
2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
4. Click on View task in Taskcluster in the new DETAILS section.
5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1876594

[MozillaNoah]

Writing the info comments, I'm realizing I forgot to implement the factory usage inside of ThumbnailCard as well. I'll add this tomorrow

[gabrielluong]

Instead of having this ComposableFactory, is there any reason why this shouldn't just be THE ThumbnailImage

[MatthewTighe]

Noah and I were brainstorming about this yesterday. Here's some of the reasoning for something along this route:

1. Modularization. Modules won't have access to components, so they won't be able to have implicit, hidden dependencies on it like is in the ThumbnailImage child Composable below. This can result in "prop drilling", where we pass these dependencies down the entire Composable stack.
2. Even before modularization, the components dependency is quite hidden in the stack here. I am not strictly against Composables that can retrieve their own dependencies, but they should then fully own those dependencies.

Using a factory pattern seemed like it might help reveal the hidden dependency a bit by requiring a step "outside" of the usual Compose world, but I don't think this is the only solution to these problems. In fact, there is probably quite a bit of prior art from the React and Redux communities that could inform our solution. @boek may have more insight there.

Ultimately, I would like to see us come to a team agreement on how to solve this problem as I think it would help the team iterate faster and avoid disparate novel solutions. That said, I am in no way married to anything as presented in this patch.

[MatthewTighe]

Jeff and I spoke offline about this, and I have a different suggestion to share when I get some time tomorrow

[rahulsainani]

Tangential on modularization in case we haven't thought of this already, compose/designsystem package should not contain any feature specific components like TabThumbnail to not have dependency on feature models.

[MatthewTighe]

Apologies for coming back a little late to this.

My short-term suggestion to avoid the preview breakage would be to pass the dependency explicitly.

Longer-term, the strategy I think we should investigate but would require team buy-in

FakeComponents

1. Create an abstract supertype of Components
2. Create fakes for each component type
3. update the globally accessible @Composable components to be something like: get() = if (inComposePreview) LocalContext.current.components else FakeComponents()

I also think we should consider moving the components from the LocalContext to the CompositionLocal to distance their association from the application context. It would also give us more control over local overrides, which is risky but potentially useful.

Then, I think it is fine for some Composables to retrieve dependencies for their children from the local composition. This would be required for Composables defined in modules that won't have knowledge of Components, and also keeps dependencies more explicit as well as practicing a bit of inversion of control. We would still have implicit dependencies hidden in the tree in the Composables that retrieved components for their children, but I think that is an okay trade-off as long as we limit it to our normally globally-accessible types for now.

A fair amount of stuff is touched on in the docs for CompositionLocal. For example, we could use the fake types as sane defaults.

As an aside: an interesting case is that they suggest not using CompositionLocal to hold ViewModels, which makes sense as they are screen-specific. However, if we move to a full Redux model with a single store, I think that we could get away with injecting that through CompositionLocal, which means we could make more interesting decisions about what level of the tree we observed Store properties and define action dispatches at.

[MatthewTighe]

Ah, Noah reminded me that we could just do the inComposePreview check in TabThumbnail directly and access components there. I think that's fine for now, but that we'll want a better long-term solution that is well-defined and can be re-used across the team.

Tangential on modularization in case we haven't thought of this already, compose/designsystem package should not contain any feature specific components like TabThumbnail to not have dependency on feature models.

TabThumbnail in particular looks to only have dependencies on lower-level types like ThumbnailStorage and TabSessionState. Given our domain, I imagine we could potentially include things like a TabThumbnail in our design system, or at the very least in a module between the design system and the app module. I admit it's been a while since I've looked over our design system tokens, so are we imagining a module that contains only components that are agnostic of the browser domain?

[MatthewTighe]

LGTM. I'll take a note to write a follow-up for investigating a more substantial strategy for these situations

[MatthewTighe]

Is this just some unrelated bookkeeping? Fine with me if so, just checking

[MozillaNoah]

Yes, Modifiers are supposed to be optional, so I opted to include that minor change now