Improve media displayed in widgets [BarcodeWidget + ArbitraryFileWidget]

[grzesiek2010]

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:

• added or modified tests for any new or changed behavior
• run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
• added a comment above any new strings describing it for translators
• added any new strings with date formatting to DateFormatsTest
• 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

[grzesiek2010]

@seadowg
please take a quick look to see if the general approach looks good to you.

[seadowg]

I definitely like the general direction of pulling out a view to specifically represent the answer here. I had in mind that we could go a bit further and have a generalized AnswerView with a similar structure that where setAnswer takes an IAnswerData (I think the widget would handle the hidden appearance in that setup). That would get us something that's simple to then use in the hierarchy later.

[grzesiek2010]

@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?

[seadowg]

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.

[grzesiek2010]

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.

[seadowg]

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.

That sounds good! You're right that it makes more sense to introduce the generalization after 2 or 3 examples rather than trying to plan it ahead of time.

[seadowg]

@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.

[grzesiek2010]

@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?

[seadowg]

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.

[grzesiek2010]

@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]

@grzesiek2010 I had been thinking we'd basically replace the onCreateAnswerView implementation with the new WidgetAnswer concept. Do you think that's another step or do you disagree?

[grzesiek2010]

@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.

[seadowg]

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]

#6534 (comment)

[grzesiek2010]

@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.

[seadowg]

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.

[grzesiek2010]

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.

[seadowg]

Did a first pass and some comments on some structural things

[seadowg]

For right now is there any reason to have WidgetAnswerView passed in like this? It seems like we'd only ever want ArbitraryFileWidget to be using ArbitraryFileWidgetAnswerView.

[seadowg]

I'd love to avoid using Dagger here. Could we add a setFontSize to WidgetAnswerView instead?

[seadowg]

While we're here, it would be great to try and avoid propagating the pattern of storing state in the views themselves. Would we be able to remove getAnswer from WidgetAnswerView and just store the answer in a field in these widgets?

[seadowg]

WIDGETS.md will need an update