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

adapt Android10/Q #606

Closed
wants to merge 2 commits into from
Closed

adapt Android10/Q #606

wants to merge 2 commits into from

Conversation

joker-fu
Copy link

As the title

}
Bitmap decodeSampledBitmap = null;
ContentResolver resolver = mContext.getContentResolver();
if (mBeforeAndroidQ) {
Copy link

Choose a reason for hiding this comment

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

It's unnecessary check, it's not really needed to use file descriptor. We fixed this in our fork, you can take a look

bandlab@6ffe481

Copy link
Author

@joker-fu joker-fu Jan 2, 2020

Choose a reason for hiding this comment

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

Thanks, I just considered Android10 compatibility. I looked at your fork and did a simple test, and it worked fine. Why is pr closed?

Copy link

Choose a reason for hiding this comment

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

We just didn't have time to prepare proper PR, but we plan to do that. Also those changes changed behavior, image is not saved anymore to Downloads, it should be documented

Copy link

Choose a reason for hiding this comment

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

One more thing, to make a proper fix, you should send PR not to master, but to develop AND to feature/non_native branches, because non-native and native versions of the lib are essentially forks

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I found UCrop's jni in my project. I haven’t had time to modify it. I will close this pr and use your solution in my project.

Copy link
Author

Choose a reason for hiding this comment

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

After several hours of incomplete testing, I came to the conclusion that your plan is more time-consuming. There are two main points: 1. CopyFile is executed regardless of the version, which increases the time 2. decodeStream is more time-consuming than decodeFileDescriptor

image

Copy link

Choose a reason for hiding this comment

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

This is the only correct way anyway. copyFile is required because it's used by other parts of the app. Using file descriptor is just incorrect

Copy link

Choose a reason for hiding this comment

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

Also, are you sure that your measurement is correct and you don't forget to include copyFile function, because it's required and for old and for new code

Copy link
Author

Choose a reason for hiding this comment

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

I don't quite agree with FileDescriptor's incorrect statement (I want to know why), because UCrop was at least available before Android10. Then there is the issue of time. Even if you add the copy application time to the version that does not use copyFile, it still saves a little time. Of course, I only tested 2 phones.

Copy link

@gildor gildor Jan 2, 2020

Choose a reason for hiding this comment

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

UCrop was at least available before Android10

I don't say that it will not work, it's just too hacky, and broken work with Android 10 proofs this. ContentResolve is the only correct and intended by Android framework way to do that

@joker-fu
Copy link
Author

joker-fu commented Jan 2, 2020

As discussed above

@joker-fu joker-fu closed this Jan 2, 2020
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