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

Support implicit intents using any kind of uri #2386

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented Jul 19, 2018

Closes #2385

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

Used the form in the issue (https://github.com/opendatakit/collect/files/2207874/dial-number.xml.txt) as well as the one from #2351.

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

I once again considered changing populateParameters to a method that returns a map of resolved parameters. I decided against it because the method is also used in ODKView to handle external apps that populate field lists. Instead, I extracted the functionality to resolve a value so it could be used just on the uri_data value.

Are there any risks to merging this code? If so, what are they?

This does affect any parameter passed into an external app so it would be good to do a quick regression check with something like https://github.com/opendatakit/counter.

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

https://github.com/opendatakit/collect/files/2207874/dial-number.xml.txt

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

getodk/docs#775

Before submitting this PR, please make sure you have:

  • run ./gradlew pmd checkstyle lintDebug spotbugsDebug and confirmed all checks still pass.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

The app type to launch is determined by the uri scheme. Prior to this change, only apps that respond to smsto: URIs could be launched.
@lognaturel lognaturel requested a review from grzesiek2010 July 19, 2018 01:47
@lognaturel lognaturel added needs testing high priority Should be looked at before other PRs/issues labels Jul 19, 2018
@grzesiek2010
Copy link
Member

It looks ok but generally, I wonder whether we should add a new widget for such actions. ExStringWidget was created to receive an answer but sms/email/dial is just to perform an action. It's more similar to ExPrinterWidget.

@mmarciniak90
Copy link
Contributor

Tested with success!

Verified on Android: 7.0, 6.0, 5.1, 4.4, 4.2, 4.1
Verified:

  • dial-number form -> Clicking on Launch button moves the user to phone app. A phone number is auto-populated if the user selects a number in the first question.
  • launching counter app

@opendatakit-bot label "behavior verified"
@opendatakit-bot unlabel "needs testing"

@yanokwa yanokwa merged commit f479445 into getodk:master Jul 19, 2018
@lognaturel
Copy link
Member Author

lognaturel commented Jul 19, 2018

@grzesiek2010 I agree with you that it feels really weird to have to specify a bind type and instance element for actions that don't result in values being saved. All of this external app functionality is a huge abuse of input and appearances. Printer is the same -- you still have to specify a bind type. The only difference is that in that case the print appearance hides the intent details from the user whereas the generic external app widgets allow the user to specify their own intent configuration.

In this case, what we really want is a control that's only in the body and doesn't necessarily have to reference a node in the instance. Really, this is what trigger is for. But guess what?! ODK already has a notion of trigger and it's totally weird and does reference an instance node. With a proper trigger, we could introduce an action that opens Android apps and has attributes to configure it.

Another option would be something like an intent body element that has attributes for data, type maybe even button name and can have child extra elements.

All this would be nice but we still would need to support the existing appearance behavior and it wouldn't provide more functionality to users so I don't see it as a big priority. It would be a big lift because we don't have any notion of body-only elements in XLSForm.

All that said, if you do have a proposal that you think would be better, do share it and we can keep discussing so it's ready when the time comes.

@lognaturel lognaturel deleted the collect-2385 branch July 19, 2018 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In external app widgets, only implicit intents that can use an smsto: uri are supported
5 participants