Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: bitmap wallpaper setting on main thread #132

Merged
merged 6 commits into from
May 15, 2021

Conversation

vipulasri
Copy link
Contributor

Presently, when the Wallapaper Manager is unable to find the crop and set activity. App decodes the image and sets the bitmap on the main thread which should be avoided.

Comment on lines 292 to 295
viewModel.downloadUUID = DownloadWorker.enqueueDownload(
applicationContext,
DownloadAction.WALLPAPER, url, photo.fileName, photo.id
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
viewModel.downloadUUID = DownloadWorker.enqueueDownload(
applicationContext,
DownloadAction.WALLPAPER, url, photo.fileName, photo.id
)
viewModel.downloadUUID = DownloadWorker.enqueueDownload(
applicationContext,
DownloadAction.WALLPAPER,
url,
photo.fileName,
photo.id
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crazyhitty code formatting has been applied.

vipulasri and others added 2 commits May 13, 2021 20:07
Co-authored-by: Kartik Sharma <[email protected]>
Co-authored-by: Kartik Sharma <[email protected]>
private fun setWallpaperWithBitmap(bitmap: Bitmap) {
lifecycleScope.launch {
withContext((Executors.newSingleThreadExecutor().asCoroutineDispatcher())) {
wallpaperManager.setBitmap(bitmap)
Copy link
Contributor

Choose a reason for hiding this comment

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

setBitmap throws IOException in case if something goes wrong while setting wallpaper. Please handle the exception.
Reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the necessary changes.

Copy link
Contributor

@crazyhitty crazyhitty left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Owner

@b-lam b-lam left a comment

Choose a reason for hiding this comment

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

Overall looks great! Just left a few comments. Out of curiosity, is there a specific reason that you wanted to fix this?

@vipulasri
Copy link
Contributor Author

Overall looks great! Just left a few comments. Out of curiosity, is there a specific reason that you wanted to fix this?

Yes, I can see the freezing of the main thread when were preparing bitmap from URI and then setting it as wallpaper.

@b-lam
Copy link
Owner

b-lam commented May 15, 2021

Awesome work, thanks for doing this! If you're interested in working on other parts of Resplash just let me know :)

@b-lam b-lam merged commit b758a17 into b-lam:master May 15, 2021
@vipulasri
Copy link
Contributor Author

sure, @b-lam. Do we have any pending features or existing tasks?

@b-lam
Copy link
Owner

b-lam commented May 15, 2021

Currently I'm working on implementing the new-ish topics endpoint (https://unsplash.com/documentation#list-topics). The work I've done so far is on this branch https://github.com/b-lam/Resplash/tree/add-topics. Some of the other things I have on my list of things to do is migrating from Paging 2 to Paging 3, fixing a lockscreen wallpaper issue that some users have mentioned, adding support for auto wallpaper intervals less than 15 minutes. I should probably start adding some of my pending work to the Github issues section. If you are a user of Resplash, feel free to do anything that you think would make the app better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants