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

Refactor FilePicker.java intent result handling #5851

Conversation

parneet-guraya
Copy link
Contributor

@parneet-guraya parneet-guraya commented Oct 14, 2024

Fixes #5850

What changes did you make and why?

  • Removed CAPTURE_VIDEO request code and it's checks, while from what I can understand we do not handle video mime type. Though if I missed something and I'm wrong please let me know, happy to make changes.

  • Removed Video's picker test cases.

  • Only Gallery picker's callback was handling changes for both Document picker and Gallery picker. So, passed appropriate flag to make sure respective callbacks get invoked

EDIT:

  • Removed below code
 else if (isPhoto(data)) {
                        onPictureReturnedFromCamera(activity, callbacks);
                    } else {
                        onPictureReturnedFromDocuments(data, activity, callbacks);
                    }

isPhoto(data) method checks if the returned data is null or not. Now, it for Document and Gallery picker results we rely on returned results so we are making sure it's not null. But, later after all the specific checks we had the above code.

To understand the problem, let's say we got the result of Document picker which also checks for !isPhoto() to make sure result is not null. But let's say it is then the above mentioned check would invoke the callback for handling camera result but didn't we returned the result for document picker (however very unlikely to get null from document picker we either get cancelled or result ok, which we already take care of before making these checks).

My understanding is this the check was added under the assumption that we store the image in the persistent storage when using camera's result. So it just made simple check if the result is null then it must be camera's result otherwise document's result. But from above we understand that it would wrong to use camera's image (will return the last stored image which is wrong) when user has chosen Document/Gallery one (less likely but still we shouldn't have this check)

Tests performed (required)

  • Ran test for FilePicker.java file.

  • manually tested. below is this demo.

Screencast.from.2024-10-15.02-07-13.webm

Tested {build variant, e.g. ProdDebug} on {name of device or emulator} with API level {API level}.
Build variant: betaDebug
API level: 34 (Emulator) & 34 Oneplus 9RT 5G

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Sounds good to me, as we most probably won't support video any time soon, even less video capture directly from the app.
I will run some tests.

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Tested uploads with various pickers (including document-based) and via sharing.
Worked fine for all uploads, coordinates preserved.

@nicolas-raoul nicolas-raoul merged commit f889ed1 into commons-app:main Oct 16, 2024
1 check passed
@parneet-guraya parneet-guraya deleted the refactor-file-picker-result-handling branch October 16, 2024 02:25
@parneet-guraya
Copy link
Contributor Author

Wow, that was fast! Thanks @nicolas-raoul 👍

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.

Remove unnecessary request code checks while handling file picker results
2 participants