collect
collect copied to clipboard
getodk
Improve media displayed in widgets [BarcodeWidget + ArbitraryFileWidget]
Closes #6234
Why is this the best possible solution? Were any other approaches considered?
The structure of this pull request reflects the discussion below. It’s the first step toward creating shareable answer views that can be used across all question types and eventually in the summary view as well. The changes are introduced for BarcodeWidget, ArbitraryFileWidget, and ExArbitraryFileWidget, but later other question types will be reworked in the same 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?
The only visible change is that answers in the barcode question will now include an icon in addition to the text. This is something we plan to introduce later for other question types as well, since a shared style should be used across both the questions and the summary view.
Aside from that, this is purely a refactoring and shouldn't introduce any functional changes. However, we should still test the BarcodeWidget, ArbitraryFileWidget, and ExArbitraryFileWidget to ensure that no regressions have been introduced.
Do we need any specific form for testing your changes? If so, please attach one.
The All question types form is fine.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
- [x] added or modified tests for any new or changed behavior
- [x] run
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still pass - [x] added a comment above any new strings describing it for translators
- [x] added any new strings with date formatting to
DateFormatsTest - [x] verified that any code or assets from external sources are properly credited in comments and/or in the about file.
- [x] verified that any new UI elements use theme colors. UI Components Style guidelines
@seadowg please take a quick look to see if the general approach looks good to you.
@seadowg could you clarify your feedback because I'm not sure if I understand?
Do you want to have one AnswerView for all types of questions? or maybe AnswerView should be an interface and implementations would be created based on data type as we do for the whole widgets in WidgetFactory?
Do you want to have one AnswerView for all types of questions? or maybe AnswerView should be an interface and implementations would be created based on data type as we do for the whole widgets in WidgetFactory?
I was thinking one AnswerView that can handle any IAnswerData, but I'd imagine that'd be implemented using multiple different child views. My thinking is that we currently make the decision about how to display an answer in two places (in each widget and in the hierarchy list items) and we could move that to one place.
I was thinking one AnswerView that can handle any IAnswerData, but I'd imagine that'd be implemented using multiple different child views.
That makes sense.
My thinking is that we currently make the decision about how to display an answer in two places (in each widget and in the hierarchy list items) and we could move that to one place.
The view I factored out will be one that can be shared between the question layout and the hierarchy layout.
As I rework other questions, I’ll likely introduce some form of abstraction. Later, when we work on the hierarchy view, we'll need to implement a factory to construct the appropriate view but that can wait for now.
Let's maybe continue with other question types in this pull request to see how it goes, unless you have something against the current changes.
@grzesiek2010 I've been running into frustrations trying to add an arg to the QuestionWidget constructor, and it led me to thinking a bit more about the eventual structure we want.
Basically I think we need to get to a point where we can modify QuestionWidget (the thing responsible for the shared view logic between every widget) without needing to then modify all the subclasses. I reckon we'll need to deploy some "composition over inheritance" to achieve this. To go with the example we already have here I reckon instead of a BearingWidget we really just want to have a BearingWidgetAnswer and then in WidgetFactory be constructing our bearing question like this (or something similar):
QuestionWidget(context, questionDetails, BearingWidgetAnswer())
That would let us easily modify QuestionWidget's constructor in the future (and remove things like Dagger). I think getting to that sort of place for at least one widget here would be great! Sadly I don't think that kind of restructure is a good idea in one go.
@seadowg
As discussed earlier, I've reworked another question type (actually four, since there are multiple image questions) to see how it goes. I believe we can have a single interface implemented for all widget answer classes, like BarcodeWidgetAnswer, ImageWidgetAnswer, and so on. However, different implementations require different dependencies. For example, ImageWidgetAnswer needs imageLoader, questionMediaManager, etc. Given that, I think we'll need a factory similar to the one we use for entire widgets.
QuestionWidget(context, questionDetails, BearingWidgetAnswer())
The current implementation adds the answers using XML, but this won’t be possible later in the summary view, where we’ll need to generate answers dynamically. So, it would probably make more sense to add that view programmatically in the widgets as well. Is that what you meant? Any other thoughts?
The current implementation adds the answers using XML, but this won’t be possible later in the summary view, where we’ll need to generate answers dynamically. So, it would probably make more sense to add that view programmatically in the widgets as well. Is that what you meant? Any other thoughts?
Right it feels like we need to move to the views being added programmatically for this to work. So ideally QuestionWidget has a new constructor arg for the widget answer view (like we've been describing) that is a View and also implements our new interface (WidgetAnswer for example). In Kotlin, we could do this:
class QuestionWidget<A>(private val widgetAnswerView: A) where A : View, A : WidgetAnswer {
I think in Java we just need to use an abstract to combine the types:
public abstract class WidgetAnswerView extends View implements WidgetAnswer {
}
Then we'd be able to add that view into the layout for the QuestionWidget programmatically and interact with it using the interface. Given it's going to be hard to convert everything at once, we probably want to deprecate, but continue to support QuestionWidget#onCreateAnswerView for a while longer.
@seadowg
The current version of this PR demonstrates how this could look using the BarcodeWidget and ArbitraryFileWidget. I think it's close to what we need. Please take a look. If it looks good, I can prepare it for review and then continue working on other widgets.
@seadowg onCreateAnswerView is responsible for setting up the entire question view, not just the answer part. For example, in BarcodeWidget, the view it returns includes both the button to trigger scanning and the real answer part. The current naming is misleading and should be changed, but we cannot remove this method.
Just to expand on my earlier comment, I wanted to be more explicit about where I saw this eventually going. My thinking was that in the future (🔮) we'd end up switching away from an inheritance based model for widgets to a composition based one. The way I'd see that working is that we'd have a QuestionWidget class that's final (no subclasses) that has a constructor like this (with probably a couple of extra args for things like AudioPlayer):
class QuestionWidget(context: Context, questionDetails: QuestionDetails, widgetAnswer: WidgetAnswer)
Then when we're displaying a question we are always constructing a QuestionWidget with the only difference being which WidgetAnswer implementation we pass it and that WidgetAnswer would handle all the type specific stuff so it would have an interface more similar to the current QuestionWidget's abstract methods.
I was thinking a way to transition to that would be to have QuestionWidget take an optional WidgetAnswer for the moment and have QuestionWidget#onCreateAnswerView use that if it's non-null (along with other methods like clearAnswer etc). In that world, we'd end up with no BarcodeWidget at all, we'd just pass BarcodeWidgetAnswer to a QuestionWidget. I think where the PR is right now is pretty similar, I'm just proposing making it a more general change and expanding it so we can kill the specific widget classes we're updating here (I'd suggest we limit it to the ones we've already got to) to set a direction.
As an aside, there's been some back and forth about Compose. I think that this direction is exactly what we'd want to do if we were looking to build widgets with Composables (or similar frameworks like React/JSX):
@Composable
fun QuestionWidget(questionDetails: QuestionDetails) {
BarcodeAnswer(
questionDetails: QuestionDetails,
onScanBarcode = {
// Launch barcode scanner
},
onLongPress = onLongPressListener
)
}
Obviously we're pretty far off from that kind of thing as widgets hold state (which wouldn't work in a Compose setup), but I think shooting for that kind of structure makes sense.
@seadowg The structure I started building (to be used in the hierarchy as well) consists of WidgetAnswerViews that contain only the answer part - for example, just the image for image questions or just the text for barcode questions. This aligns with what we previously agreed on, as I recall. Based on your description, it seems like you envision WidgetAnswerView as the entire question view, including buttons and other elements. Am I misunderstanding something? The structure you described isn’t compatible with the one I started working on. Of course, in the future, we can introduce another set of views that build on WidgetAnswerView and incorporate the rest of the layout.
Based on your description, it seems like you envision WidgetAnswerView as the entire question view, including buttons and other elements. Am I misunderstanding something? The structure you described isn’t compatible with the one I started working on. Of course, in the future, we can introduce another set of views that build on WidgetAnswerView and incorporate the rest of the layout.
I've had a bit of a think about it, and I agree. I'm jumping the gun a bit! Down the line, I'd really like to get to a place where QuestionWidget has no subclasses, and we just create widgets via composition, but you're right that WidgetAnswer can be a building block in that rather than providing the whole thing.
How about we rename WidgetAnswer to AnswerView to make it really clear that this is a component for rendering answer data. We should rename onCreateAnswerView to something else here as well as (as you point out), it's really more than just the "answer" - it's the full type specific controls and answer for the widget.
Thanks for back and forth on this with me! I'm liking the direction we're moving in.
How about we rename WidgetAnswer to AnswerView to make it really clear that this is a component for rendering answer data. We should rename onCreateAnswerView to something else here as well as (as you point out), it's really more than just the "answer" - it's the full type specific controls and answer for the widget.
Done.
Icon is to close to the text and I don't know if it's just me but it looks bit weird if there is longer text? Also you might notice that the text is cut off and it's not possible to move it
Icon to close
Long and cut off text
Tested with success
Verified on Android 16
Verified cases:
- Scanning different barcodes
- Different type of media in file widget
- Regression check on media, file and barcode widget
