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

Conversation

MatthewTighe
Copy link
Contributor

@MatthewTighe MatthewTighe commented Jul 29, 2022

This is part of #26034. The main point of this PR was to remove updateWallpaper, which included the side-effect of updating the currentWallpaper, which should instead now be handled by dispatching the UpdateCurrentWallpaper.

The next step will likely be to refactor the WallpaperManager into a WallpaperUseCase, as filed in #26245. That patch will probably introduce the largest number of changes in this initiative.

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.

QA

  • [] QA Needed

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-debug task.
  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

Fixes #26052

@MatthewTighe MatthewTighe requested review from a team as code owners July 29, 2022 22:49
@MatthewTighe MatthewTighe force-pushed the remove-update-wallpaper branch 2 times, most recently from 24bf695 to 8c1f6a5 Compare August 1, 2022 17:50
Copy link
Contributor

@MozillaNoah MozillaNoah left a comment

Choose a reason for hiding this comment

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

couple nits. Unrelated follow-up, I also noticed that the WallpaperThumbnailsPreview doesn't render, probably because it's using the .components extension.

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.

* 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

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.

@MatthewTighe
Copy link
Contributor Author

couple nits. Unrelated follow-up, I also noticed that the WallpaperThumbnailsPreview doesn't render, probably because it's using the .components extension.

Oh thanks, that would definitely explain it! Are there any easy ways around that or would I need to manually instantiate anything I use in previews?

@MozillaNoah
Copy link
Contributor

couple nits. Unrelated follow-up, I also noticed that the WallpaperThumbnailsPreview doesn't render, probably because it's using the .components extension.

Oh thanks, that would definitely explain it! Are there any easy ways around that or would I need to manually instantiate anything I use in previews?

You would need to either manually instantiate the real-world use-case or just abstract-out that dependency so either only a lambda or a list<> can be passed-in. If something further upstream needs the context, then we have to do an if with inComposePreview (see: Favicon) to just provide an override for @Previews

@MatthewTighe
Copy link
Contributor Author

Note: the test that was removed in the diff was failing, but that functionality will be removed soon anyway in #25985. I was having trouble getting the UI tests to run locally, and there's quite a lot of follow-up work for this PR so I decided to go with the fastest option. I'll squash before I land

@MatthewTighe MatthewTighe force-pushed the remove-update-wallpaper branch from 9a0dbb3 to ecbf9bc Compare August 1, 2022 23:06
@gabrielluong gabrielluong added the pr:approved PR that has been approved label Aug 2, 2022
@MatthewTighe MatthewTighe force-pushed the remove-update-wallpaper branch from ecbf9bc to 9f9ead2 Compare August 2, 2022 20:14
@MatthewTighe MatthewTighe added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Aug 2, 2022
@mergify mergify bot merged commit 9fb6eeb into mozilla-mobile:main Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove WallpaperManager::updateWallpaper
3 participants