Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Fixes: #26052 remove WallpaperManager::updateWallpaper #26249

Merged
merged 2 commits into from
Aug 2, 2022
Merged

Fixes: #26052 remove WallpaperManager::updateWallpaper #26249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f9ead2
Fixes: #26052 remove WallpaperManager::updateWallpaper
MatthewTighe Jul 29, 2022
6323146
Merge branch 'main' into remove-update-wallpaper
mergify[bot] Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions app/src/androidTest/java/org/mozilla/fenix/ui/HomeScreenTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import androidx.test.uiautomator.Until
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.mozilla.fenix.customannotations.SmokeTest
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.helpers.HomeActivityTestRule
import org.mozilla.fenix.helpers.RetryTestRule
Expand Down Expand Up @@ -154,19 +153,4 @@ class HomeScreenTest {
verifyWelcomeHeader()
}
}

@SmokeTest
@Test
fun tapLogoToChangeWallpaperTest() {
homeScreen {
clickFirefoxLogo()
verifyWallpaperImageApplied(true)
clickFirefoxLogo()
verifyWallpaperImageApplied(true)
clickFirefoxLogo()
verifyWallpaperImageApplied(true)
clickFirefoxLogo()
verifyWallpaperImageApplied(false)
}
}
}
23 changes: 12 additions & 11 deletions app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ import org.mozilla.fenix.tabstray.TabsTrayAccessPoint
import org.mozilla.fenix.utils.Settings.Companion.TOP_SITES_PROVIDER_MAX_THRESHOLD
import org.mozilla.fenix.utils.ToolbarPopupWindow
import org.mozilla.fenix.utils.allowUndo
import org.mozilla.fenix.wallpapers.WallpaperManager
import org.mozilla.fenix.wallpapers.Wallpaper
import java.lang.ref.WeakReference
import kotlin.math.min

Expand Down Expand Up @@ -754,10 +754,6 @@ class HomeFragment : Fragment() {
themeCollection = newWallpaper::class.simpleName
)
)
manager.updateWallpaper(
wallpaperContainer = binding.wallpaperImageView,
newWallpaper = newWallpaper
)
}
}
}
Expand Down Expand Up @@ -967,12 +963,17 @@ class HomeFragment : Fragment() {
.onEach { state ->
// We only want to update the wallpaper when it's different from the default one
// as the default is applied already on xml by default.
val currentWallpaper = state.wallpaperState.currentWallpaper
if (currentWallpaper != WallpaperManager.defaultWallpaper) {
requireComponents.wallpaperManager.updateWallpaper(
binding.wallpaperImageView,
currentWallpaper
)
when (val currentWallpaper = state.wallpaperState.currentWallpaper) {
is Wallpaper.Default -> {
binding.wallpaperImageView.visibility = View.GONE
}
else -> {
with(requireComponents.wallpaperManager) {
val bitmap = currentWallpaper.load(requireContext()) ?: return@onEach
bitmap.scaleBitmapToBottomOfView(binding.wallpaperImageView)
}
binding.wallpaperImageView.visibility = View.VISIBLE
}
}
}
.launchIn(viewLifecycleOwner.lifecycleScope)
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/java/org/mozilla/fenix/settings/wallpaper/WallpaperSettings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ private fun WallpaperThumbnailsPreview() {

WallpaperSettings(
defaultWallpaper = WallpaperManager.defaultWallpaper,
loadWallpaperResource = {
wallpaperManager.loadSavedWallpaper(context, it)
loadWallpaperResource = { wallpaper ->
with(wallpaperManager) { wallpaper.load(context) }
},
wallpapers = wallpaperManager.wallpapers,
selectedWallpaper = wallpaperManager.currentWallpaper,
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/java/org/mozilla/fenix/settings/wallpaper/WallpaperSettingsFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ class WallpaperSettingsFragment : Fragment() {
WallpaperSettings(
wallpapers = wallpaperManager.wallpapers,
defaultWallpaper = WallpaperManager.defaultWallpaper,
loadWallpaperResource = {
wallpaperManager.loadSavedWallpaper(requireContext(), it)
loadWallpaperResource = { wallpaper ->
with(wallpaperManager) { wallpaper.load(context) }
},
selectedWallpaper = currentWallpaper,
onSelectWallpaper = { selectedWallpaper: Wallpaper ->
Expand Down
43 changes: 13 additions & 30 deletions app/src/main/java/org/mozilla/fenix/wallpapers/WallpaperManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,30 +58,6 @@ class WallpaperManager(
fileManager.clean(currentWallpaper, wallpapers.filterIsInstance<Wallpaper.Remote>())
}

/**
* Apply the [newWallpaper] into the [wallpaperContainer] and update the [currentWallpaper].
*/
fun updateWallpaper(wallpaperContainer: ImageView, newWallpaper: Wallpaper) {
val context = wallpaperContainer.context
if (newWallpaper == defaultWallpaper) {
wallpaperContainer.visibility = View.GONE
logger.info("Wallpaper update to default background")
} else {
val bitmap = loadSavedWallpaper(context, newWallpaper)
if (bitmap == null) {
val message = "Could not load wallpaper bitmap. Resetting to default."
logger.error(message)
currentWallpaper = defaultWallpaper
wallpaperContainer.visibility = View.GONE
return
} else {
wallpaperContainer.visibility = View.VISIBLE
scaleBitmapToBottom(bitmap, wallpaperContainer)
}
}
currentWallpaper = newWallpaper
}

/**
* Download all known remote wallpapers.
*/
Expand All @@ -104,7 +80,7 @@ class WallpaperManager(
} else {
values[index]
}.also {
appStore.dispatch(AppAction.WallpaperAction.UpdateCurrentWallpaper(it))
currentWallpaper = it
}
}

Expand Down Expand Up @@ -139,10 +115,10 @@ class WallpaperManager(
/**
* Load a wallpaper that is saved locally.
*/
fun loadSavedWallpaper(context: Context, wallpaper: Wallpaper): Bitmap? =
when (wallpaper) {
is Wallpaper.Local -> loadWallpaperFromDrawables(context, wallpaper)
is Wallpaper.Remote -> loadWallpaperFromDisk(context, wallpaper)
fun Wallpaper.load(context: Context): Bitmap? =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should this extension instead live in a extension function file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one technically could, but structuring it this way was an intentional plan for refactoring the WallpaperManager as a WallpaperUseCase. It forces a client to have a reference to the UseCase to be able to load wallpapers. That way, we can treat the UseCase as an abstraction barrier. Right now, all the loading is done using implicit Context-dependent stuff, but I imagine the callstack would look more like this soon:

with (wallpaperUseCase) {
    wallpaper.load() -> wallpaperUseCase.fileManager.loadWallpaper()
}

Not sure that notation really makes sense lol. But basically we could inject the context (or a WallpaperLoader or whatever else we wanted to use to actually do disk reading) into the object hierarchy and hide it from call sites of wallpaper.load().

Sidenote: the pattern here might be more clear if we were on Kotlin 1.6.20 and could use context receivers.

when (this) {
is Wallpaper.Local -> loadWallpaperFromDrawables(context, this)
is Wallpaper.Remote -> loadWallpaperFromDisk(context, this)
else -> null
}

Expand All @@ -163,7 +139,14 @@ class WallpaperManager(
}
}.getOrNull()

private fun scaleBitmapToBottom(bitmap: Bitmap, view: ImageView) {
/**
* This will scale the received [Bitmap] to the size of the [view]. It retains the bitmap's
* original aspect ratio, but will shrink or enlarge it to fit the viewport. If bitmap does not
* correctly fit the aspect ratio of the view, it will be shifted to prioritize the bottom-left
* of the bitmap.
*/
fun Bitmap.scaleBitmapToBottomOfView(view: ImageView) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should this extension instead live in a extension function file?

Copy link
Contributor Author

@MatthewTighe MatthewTighe Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one would make more sense to move into an extension function file, IMO, because it isn't necessarily a strictly wallpaper-related function. I just always worry about polluting namespaces without an obvious need. I'd defer to your opinion if you think we should move it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel super strongly either way, so if you feel this will be used elsewhere, I'd lean that way. Otherwise, it can just live here for now.

val bitmap = this
view.setImageBitmap(bitmap)
view.scaleType = ImageView.ScaleType.MATRIX
val matrix = Matrix()
Expand Down