-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add question type that shows preview and prints to Android-connected printer #5854
Conversation
0697060
to
c471c67
Compare
collect_app/src/main/java/org/odk/collect/android/widgets/utilities/PrintableHtmlParser.kt
Outdated
Show resolved
Hide resolved
It's magical, @grzesiek2010! 🎉 I notice it's taking a very long time to load the preview screen after tapping the "Initiate printing" button (I think it should just be "Print"). I also keep getting "ODK Collect isn't responding" after tapping the button including when the preview screen is up and after I've exited out of printing. I think it's related to the image manipulation. It seems less bad when I don't take a picture (but I do still get not responding bottom sheets every few seconds after I've printed, even when on the main menu screen). |
2b61f5f
to
6f6ba4c
Compare
I've added a progress dialog so that it's clear that something is happening (the HTML is being parsed).
Ok, done.
I've updated the way images are added to the HTML. Now I don't convert them to bitmaps and include data URLs but passing a path so maybe it will be faster. @lognaturel please test if now it works faster and does not cause any ANRs. |
Can't reproduce any of the issues I'd highlighted. 👍 |
906a0d3
to
3ea3a16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so cool! I've only had a chance to run through the general structure, and that's mostly looking good to me! A couple of things though:
- I think it'd be nicer to have
PrinterWidgetViewModel
passed into the widget via the constructor (probably as an interface defined inwidgets
that the ViewModel implements) so that the Activity is in control of constructing it and making observations on its data. This limits the amount of dependencies going into the purely view layer and also means the widget doesn't need to understand anything about the lifecycle it's part of. - I'd be interested in if it's possible to do the HTML parsing/manipulation without adding a new dependency (and not increasing the APK size as much) using some of the XML parsing libraries we already use (like
KXML2
for instance).
Do you mean building the ViewModel in
I tried kxml2 but it is primarily designed for parsing XML and it didn't work well with HTML. I think we should use a reliable library like Jsoup to make sure even complex HTML constructions work well. We don't want to test such cases manually. |
Yes. We already use this approach for a few ViewModels - we pass the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1aafbd5
to
bc817ff
Compare
collect_app/src/main/java/org/odk/collect/android/widgets/PrinterWidget.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/widgets/PrinterWidget.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/widgets/PrinterWidget.kt
Outdated
Show resolved
Hide resolved
b5e9a71
to
2fac81b
Compare
2fac81b
to
3cb9f90
Compare
collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java
Outdated
Show resolved
Hide resolved
@@ -52,6 +52,10 @@ public static String getAnswerText(FormEntryPrompt fep, Context context, FormCon | |||
IAnswerData data = fep.getAnswerValue(); | |||
final String appearance = fep.getQuestion().getAppearanceAttr(); | |||
|
|||
if (appearance != null && appearance.equals(Appearances.PRINTER)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test for this change? Maybe we should rename this method to something like getHierarchySummary
and write a unit test for it that makes sure the printer questions always return ""
. It'd be nice for the helper to live in the new hierarchy module, but I realise that was actually created in a different PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge #5885 first then and I can add a new test in the same package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to merge this first so we can get it out as a beta ASAP. Are you able to add a test in #5885?
collect_app/src/main/java/org/odk/collect/android/widgets/interfaces/Printer.kt
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/PrinterWidgetViewModel.kt
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/widgets/PrinterWidgetTest.kt
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/widgets/PrinterWidget.kt
Outdated
Show resolved
Hide resolved
Everything is up to you. If you used the form I have attached the code that generates the document contains:
again if you used my form it's this part: you can add/remove something you can change the size of the code as well. |
I Noticed two strange behaviors:
|
Both those issues seem like something not directly related to Collect. If the preview is generated correctly that's where our job ends. Could you try to do the same with other apps that print documents and compare the behavior? |
@grzesiek2010 Checked on different apps and it works in similar way. Tested with Success! Verified on device with Android 12,13 Verified cases:
|
Tested with Success! Verified on device with Android 10 |
Closes #5831
Why is this the best possible solution? Were any other approaches considered?
We have decided to use the Android printing API because it's versatile and should work with different printers. The API offers three different ways of printing and the one we have chosen is with HTML because in our case we can relatively easily build such HTML docs using the
calculation
column and the concat function. The sample from the form attached below looks like:I said it's relatively easy because the more complex the expected output is the more args the function will have. The problem with this function is that every time we want to add an expression that should be evaluated like
${first_name}
,${last_name}
etc. above we need to add them in as separate args because that's how this function works what makes creating bigger documents difficult and error-prone.It would be good to have a new function like:
That would allow us to have one string, like in kotlin for example. However, this is something we can add separately later.
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?
This introduces a new question type, so the changes seem to be isolated and should not affect other functionalities. Given that, we can (almost) fully focus on testing what's new. The only thing that I would verify to avoid regression is the old printer question type. The old one should be created if the appearance starts with
printer
and the new one if it is justprinter
:printer
then start the new printer displayedprinter:printer:org.opendatakit.sensors.ZebraPrinter
the old printer widget should be displayedDo we need any specific form for testing your changes? If so, please attach one.
I've created one sample form:
Printing.xlsx
Does this change require updates to documentation? If so, please file an issue here and include the link below.
Yes getodk/docs#1720
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.