closes #23504: download focus wallpapers at runtime #23505

MatthewTighe · 2022-01-31T22:37:53Z

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

Amejia481

Did a first pass it looks good in general, just added some suggestions

Amejia481 · 2022-02-01T16:30:34Z

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

+
+    @OptIn(DelicateCoroutinesApi::class)
+    private fun downloadWallpapers() {
+        GlobalScope.launch {


I think we should be explicit about the dispatcher GlobalScope.launch(Dispatchers.IO), is there any specific reason for using the default value? :)

I like to switch the dispatcher closest to the code that actually needs to be on the different dispatcher. That way, calling code doesn't need to introspect the async code to discover it needs to change dispatchers. This is a suggested practice, and is something that can be observed in Room and in Retrofit I believe.

That is, it's safe to call suspend functions defined in Room and Retrofit from any Dispatcher, because the libraries ensure they switch to IO before performing async tasks. I like to mirror that behavior.

We have some previous discussion about using the Dispatchers.Default when was strictly needed you could read more #19574 @mcomella and @csadilek could provide more pointers as they were more involved on the ticket.

app/src/main/java/org/mozilla/fenix/wallpapers/Wallpaper.kt

Amejia481 · 2022-02-01T16:48:26Z

app/src/main/java/org/mozilla/fenix/wallpapers/Wallpaper.kt

+fun Wallpaper.getLocalPath(orientation: String, theme: String): String =
+    "$orientation/$theme/${themeCollection.directoryName()}/$name.png"
+
+private fun Context.isLandscape(): Boolean {
+    return resources.configuration.orientation == Configuration.ORIENTATION_LANDSCAPE
+}
+
+private fun Context.isDark(): Boolean {
+    return resources.configuration.uiMode and
+        Configuration.UI_MODE_NIGHT_MASK == Configuration.UI_MODE_NIGHT_YES
 }


This look more like responsibilities that could be on the Wallpaper manager

I could move these extensions into that file if that makes more organizational sense, but Wallpaper.getLocal path is also used by WallpaperDownloader. WallpaperDownloader is a child of WallpaperManager, and so should not receive it as a dependency.

I could also just copy the functions into both, if that makes more sense. What do you think?

We could also have a Extensions.kt file under the wallpapers package similar to what we do here

https://github.com/mozilla-mobile/android-components/blob/main/components/feature/addons/src/main/java/mozilla/components/feature/addons/ui/Extensions.kt

that could be covered in follow up, let's leave it as it's for now.

app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperDownloader.kt

app/src/main/java/org/mozilla/fenix/settings/wallpaper/WallpaperSettings.kt

app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperManager.kt

app/src/main/java/org/mozilla/fenix/settings/wallpaper/WallpaperSettings.kt

app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperDownloader.kt

MatthewTighe · 2022-02-01T19:07:04Z

Added a commit addressing feedback, will squash them before landing. @Amejia481 and I decided that we'll land a separate PR for switching the local wallpapers from assets to drawables, and that will include the discussed changes to the Wallpaper type.

Amejia481 · 2022-02-01T20:26:33Z

app/src/main/java/org/mozilla/fenix/wallpapers/Wallpaper.kt

+fun WallpaperThemeCollection.directoryName(): String = when (this) {
+    WallpaperThemeCollection.NONE,
+    WallpaperThemeCollection.FIREFOX -> ""
+    WallpaperThemeCollection.FOCUS -> "focus"
+}


nit: This could be a property on the WallpaperThemeCollection :)
We could do that in follow up.

A better name might actually be what we want here. This function returns the remote directory name, so I don't think it's necessarily an applicable property for all theme collections

Amejia481 · 2022-02-01T20:33:56Z

app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperDownloader.kt

+                }
+            }.onFailure {
+                logger.error(it.message ?: "Download failed: no throwable message included.", it)
+                context.components.analytics.crashReporter.submitCaughtException(it)


nit: For future testing purpose we could the reference of crashReporter to WallpaperDownloader, we could do this in a follow up.

Amejia481 · 2022-02-01T20:58:59Z

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

@@ -127,6 +127,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider {

        setupInMainProcessOnly()

+        downloadWallpapers()


I almost missed this one, this should be under a feature validation FeatureFlags.showWallpapers + FeatureFlags.isThemedWallpapersFeatureEnabled.

Maybe we could add FeatureFlags.isThemedWallpapersFeatureEnabled later

🙏 good catch, TYVM

Amejia481

Tested locally and it's working like a charm!

…zilla-mobile#23505) * closes mozilla-mobile#23504: download focus wallpapers at runtime * address pr feedback * only download wallpapers if feature flag is set

MatthewTighe requested a review from Amejia481 January 31, 2022 22:37

MatthewTighe requested review from a team as code owners January 31, 2022 22:37

MatthewTighe force-pushed the wallpaper-network branch from a16e40f to 2e6e3fc Compare January 31, 2022 22:54

Amejia481 reviewed Feb 1, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperManager.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Feb 1, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/wallpaper/WallpaperSettings.kt Outdated Show resolved Hide resolved

gabrielluong reviewed Feb 1, 2022

View reviewed changes

app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperDownloader.kt Outdated Show resolved Hide resolved

closes mozilla-mobile#23504: download focus wallpapers at runtime

730a7f7

MatthewTighe force-pushed the wallpaper-network branch 2 times, most recently from c4a0d07 to 213575e Compare February 1, 2022 19:24

address pr feedback

e99a283

MatthewTighe force-pushed the wallpaper-network branch from 213575e to e99a283 Compare February 1, 2022 19:37

Amejia481 reviewed Feb 1, 2022

View reviewed changes

only download wallpapers if feature flag is set

a6595eb

MatthewTighe force-pushed the wallpaper-network branch from 3fbf45d to a6595eb Compare February 1, 2022 21:20

Amejia481 approved these changes Feb 1, 2022

View reviewed changes

MatthewTighe added the pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] label Feb 1, 2022

mergify bot merged commit 9dc0506 into mozilla-mobile:main Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

closes #23504: download focus wallpapers at runtime #23505

closes #23504: download focus wallpapers at runtime #23505

MatthewTighe commented Jan 31, 2022

Amejia481 left a comment

Amejia481 Feb 1, 2022

MatthewTighe Feb 1, 2022

Amejia481 Feb 1, 2022

Amejia481 Feb 1, 2022

MatthewTighe Feb 1, 2022

Amejia481 Feb 1, 2022 •

edited

Loading

MatthewTighe commented Feb 1, 2022

Amejia481 Feb 1, 2022

MatthewTighe Feb 1, 2022

Amejia481 Feb 1, 2022

Amejia481 Feb 1, 2022

Amejia481 Feb 1, 2022

MatthewTighe Feb 1, 2022

Amejia481 left a comment

		@@ -127,6 +127,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider {

		setupInMainProcessOnly()

		downloadWallpapers()

closes #23504: download focus wallpapers at runtime #23505

closes #23504: download focus wallpapers at runtime #23505

Conversation

MatthewTighe commented Jan 31, 2022

To download an APK when reviewing a PR:

Amejia481 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

Amejia481 Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

MatthewTighe commented Feb 1, 2022

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

Amejia481 left a comment

Choose a reason for hiding this comment

Amejia481 Feb 1, 2022 •

edited

Loading