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

Don't crash during updateBlur() if viewRoot call save() and restore() an unequal amount of times (Fix #184) #187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Fox2Code
Copy link

@Fox2Code Fox2Code commented Oct 7, 2022

This match the behavior of sCompatibilityRestore for app targeting android 5.1 (sdk 22) and earlier.

Reference: Canvas.java from LineageOS 19.1 (Other ROMs should be the same)

This will also help to avoid being mislead that BlurView is the cause of issues that BlurView has nothing to do with.

I also updated the javadoc on BlurViewCanvas

public BlurViewCanvas(@NonNull Bitmap bitmap) {
super(bitmap);
}

public void saveRoot() {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think the custom method is a good idea.
Some other code might still call regular save on this canvas, invalidating the whole fix.

Copy link
Author

Choose a reason for hiding this comment

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

saveRoot is only used to get the save index, in fact there is no need to change the save function behavior.

If there is more save than restore the restoreRoot function will restore to the correct index.
If there is more restore than save, we need to keep the original transform intact, so this is why restore need to be overridden.

In fact, code calling the regular save is intended behavior of this PR as there is no need to change how it works.
In fact the saveRoot and restoreRoot should only be called by BlurView.

I will update the javadoc, sorry it is my fault for not making the code more understandable.

Copy link
Author

Choose a reason for hiding this comment

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

Thank for reviewing the patch, I added the javadoc to make it easier to understand.

The initial version of the patch hooked save and restore, but it doesn't work because of restoreToCount.

If you have any more questions please feel free to ask.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright, could you please explain how this happens exactly?
Because I'm struggling to understand how to reproduce this crash

Copy link
Author

Choose a reason for hiding this comment

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

It's just bad code calling save() or restore() too much times, this commit goal is to make sure the crash does not come from BlurView when it is not BlurView fault.

This patch doesn't prevent crashes, it just make the stacktrace show the actual error instead of just showing it comes from BlurView when it has nothing to do with BlurView.

To reproduce this crash, it just require to create a View that call restore() in it's draw function without calling save()
(Aka. making bad code)

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.

2 participants