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

Allow custom camera app to be specified in a question #5738

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Sep 14, 2023

Closes #5621

What has been done to verify that this works as intended?

I've tested the changes manually and added automated tests.

Why is this the best possible solution? Were any other approaches considered?

We have discussed other options (see in the issue) but decided that the issue should be fixed in this particular way.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

As described in the issue this should allow specifying custom camera apps in image/annotate widgets. We need to test carefully those two widgets and verify edge cases like:

  • what happens if a widget has a custom camera app specified but it's not installed
  • what if the package name is wrong
  • what if the package name is correct but points to an app tat is not a camera app

Do we need any specific form for testing your changes? If so, please attach one.

I've used:
image_questions.xlsx
image_questions.xml.txt

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes getodk/docs#1652

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@lognaturel
Copy link
Member

@seadowg will consider @lindsay-stevens's points at XLSForm/pyxform#651 (comment) when doing review. @grzesiek2010 and I still feel like it's the right way to go.

@seadowg
Copy link
Member

seadowg commented Oct 3, 2023

@lognaturel @grzesiek2010 I don't actually think it makes sense for me to jump into XLSForm/XForm design discussion here if you're both already convinced we're going in the right direction - I think I'm hitting my context switching limit. If there's still some doubt that'd be helpful for me to weight in on though, let me know and I can dive in. For the moment, I'll remove myself as a reviewer and you can add me back on if this is ready to go.

@seadowg seadowg removed their request for review October 3, 2023 12:17
@grzesiek2010
Copy link
Member Author

I think it's ready for review. We decided to stick to what we agreed on before and changes to pyxform will be merged soon.

@seadowg
Copy link
Member

seadowg commented Oct 30, 2023

@grzesiek2010 Would it be possible to include an example XLSForm (and XForm seeing as the pyxform changes won't have released yet) in the PR description. Would make reviewing/testing a lot easier.

Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

@grzesiek2010
Copy link
Member Author

@grzesiek2010 Would it be possible to include an example XLSForm (and XForm seeing as the pyxform changes won't have released yet) in the PR description. Would make reviewing/testing a lot easier.

Yes, I forgot to add it.

@dbemke
Copy link

dbemke commented Nov 9, 2023

After taking a photo in vertical position via Open Camera and Timestamp camera, the photo is shown in Collect in horizontal position. The issue doesn't always occur. Reproduced on Android 10 and 13 (the form from the PR).
The screenshot after taking a photo in a vertical position (shown in horizontal).
vertical

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Nov 10, 2023

After taking a photo in vertical position via Open Camera and Timestamp camera, the photo is shown in Collect in horizontal position. The issue doesn't always occur. Reproduced on Android 10 and 13 (the form from the PR).
The screenshot after taking a photo in a vertical position (shown in horizontal).

You can file issues like that separately after merging this pull request. It might be on our side but it also might be a bug in that external camera app. This is not directly related to letting users start particular camera apps.

@dbemke
Copy link

dbemke commented Nov 10, 2023

In Image and Annotate widget with "Choose image" when there's a wrong package or the external app isn't installed there's a toast on tapping "Take picture" but it's possible to add a photo via "Choose Image" button. I'm not sure what's the expected - "Choose Image" should be enabled or disabled?

@grzesiek2010
Copy link
Member Author

In Image and Annotate widget with "Choose image" when there's a wrong package or the external app isn't installed there's a toast on tapping "Take picture" but it's possible to add a photo via "Choose Image" button. I'm not sure what's the expected - "Choose Image" should be enabled or disabled?

This is expected we were thinking about hiding that button entirely and only allowing users to take new pictures but if that's users want they can mix it with the new appearance (see: https://docs.getodk.org/form-question-types/#image-widget-with-required-new-image)

@srujner
Copy link

srujner commented Nov 10, 2023

The Image Google Camera have a different behavior:

On Android 10:

  • "No activity found to handle: Take picture" toast is visible

On Android 13:

  • ODK opens built-in android camera app
    Screenshot_20231110-142126

@srujner
Copy link

srujner commented Nov 10, 2023

The Image Google Camera have a different behavior:

Are you sure that on both devices it's the same camera app? And the package name is correct? You should be able to verify the package name using https://play.google.com/store/apps/details?id=com.csdroid.pkg. What about other camera apps on that Android 10?

Now i see that Android 13 (Google Pixel 6a) is using app called Pixel Camera (it is built-in app), probably it is not available on Redmi 10

@grzesiek2010
Copy link
Member Author

The Image Google Camera have a different behavior:

This indicates that either the app is not installed or the package name is incorrect. But in this case the toast should be displayed in both cases. If the default camera app is started it might mean there is no package name specified at all. Are you sure that the question has it?

@grzesiek2010
Copy link
Member Author

Now i see that Android 13 (Google Pixel 6a) is using app called Pixel Camera (it is built-in app), probably it is not available on Redmi 10

It doesn't matter what the default camera app is. If you have a question with a package name if it's incorrect the built-in camera app should not be started and there should be a toast in both cases. Maybe you have two different forms?

@srujner
Copy link

srujner commented Nov 10, 2023

Now i see that Android 13 (Google Pixel 6a) is using app called Pixel Camera (it is built-in app), probably it is not available on Redmi 10

It doesn't matter what the default camera app is. If you have a question with a package name if it's incorrect the built-in camera app should not be started and there should be a toast in both cases. Maybe you have two different forms?

We've got the correct Package name in that case and we're using exactly the same form.
Android 10 is showing an error because the app is not installed (and the same app as in Google Pixel is not available on Redmi)

@grzesiek2010
Copy link
Member Author

Android 10 is showing an error because the app is not installed

That's ok

(and the same app as in Google Pixel is not available on Redmi)

it shouldn't matter... you have a question with com.google.android.GoogleCamera and it's not installed on both devices. Does the default camera on Android 13 have the same package name? That would explain why it's started but it shouldn't happen I guess.

@srujner
Copy link

srujner commented Nov 10, 2023

Android 10 is showing an error because the app is not installed

That's ok

(and the same app as in Google Pixel is not available on Redmi)

it shouldn't matter... you have a question with com.google.android.GoogleCamera and it's not installed on both devices. Does the default camera on Android 13 have the same package name? That would explain why it's started but it shouldn't happen I guess.

I checked package name using the app that You suggested and it turned out that it was actually the default app on Google Pixel.

So to sum up: On Android 13 device the app with the com.google.android.GoogleCamera package name was already installed that's why it was available and on Android 10 device the app with the same package was not installed that's why we've seen the error toast.

@dbemke
Copy link

dbemke commented Nov 13, 2023

In the Image and Annotate widget with OpenCamera there's an option to record a video. It's possible to record a video in the Image widget but the file won't be saved (there's no crash).
opencamera

@grzesiek2010
Copy link
Member Author

Triggering that action I set a flag that indicates it should be for image capturing. If there are apps that do not respect it that's not our fault.

@dbemke
Copy link

dbemke commented Nov 13, 2023

Tested with Success!

Verified on device with Android 8.1, 10

Verified cases:

  • criteria in Allow custom camera app to be specified in a question #5621
  • xml forms (Image and Annotate widget) with and without custom camera specified in the form
  • packages: OpenCamera, Timestamp camera, Google camera, Camera FV-5 Lite, Cymera
  • using annotate and image widget with a custom camera specified when the camera app is installed and not
  • wrong package in the form
  • correct name of a package the app isn’t a camera app (Youtube)
  • camera permissions granted/ not granted in custom cameras
  • taking, removing, editing photos via a custom camera
  • image widget without a custom camera specified in the form when other camera apps are installed and not installed
  • Image and Annotate widget with appearance new
  • regression checks in the Image widget

@srujner
Copy link

srujner commented Nov 13, 2023

Tested with Success!

Verified on device with Android 12, 13

@grzesiek2010 grzesiek2010 merged commit 388c30d into getodk:master Nov 13, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow custom camera app to be specified in a question
5 participants