Improve media displayed in widgets [VideoWidget + ExVideoWidget]

[grzesiek2010]

Work towards #6234

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

As discussed in the comments, using Jetpack Compose appears to be the right approach for refactoring here.

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?

We need to test the whole Video and ExVideo widgets for regression, plus their new way of displaying the answer.

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

No.

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 Not long ago, we merged the first PR towards #6234 - a refactoring of the Barcode and File widgets (#6534). Although that wasn’t too recent, the pull request had been open for quite a while and was originally created when we thought adopting Jetpack Compose wouldn’t be possible anytime soon. Things have changed since then, and we’ve now started using Jetpack Compose.

I was eager to work with Jetpack Compose, and I’m really happy that it’s now feasible. The ongoing widget rework (#6234 and #6084) will introduce many changes, and I don’t think continuing to use the old view system makes sense. On the contrary, this is a great opportunity to implement everything with Jetpack Compose.

Here’s what I’ve done so far here:

1. Reworked the widget content using Jetpack Compose while keeping the existing architecture of views that extend QuestionWidget.
2. Extracted an Answer composable that can be used in both the widget and the summary view.
3. Demonstrated that we can test UI elements built with Jetpack Compose in both unit and UI tests.

I hope you’ll like this direction as much as I do. Although it might take a bit more time to build everything this way compared to the old Android views, I’m confident it’s the right move and will pay off in the long run.

The PR is actually ready for review, but I’m keeping it as a draft for now to give you a chance to get familiar with the idea first.

[seadowg]

Reworked the widget content using Jetpack Compose while keeping the existing architecture of views that extend QuestionWidget.
Extracted an Answer composable that can be used in both the widget and the summary view.

I just wanted to clarify this quickly! So basically, if we were building a widget from scratch you'd still create a QuestionWidget implementation, but onCreateWidgetView would return a ComposeView and the majority of the UI would be implemented in Compose and then be hooked up to the QuestionWidget API. That way, we can simply have an @Composable for the "answer" part of the view (like you've done with VideoWidgetAnswer) that can be shared between the widget and the hierarchy. Correct? If that's the case I think we'll also want to go back and rework the widgets we changed in #6534 as it seems like we'd be getting rid of the WidgetAnswerView concept all together.

Also, do you think we should add a Composable that shows a different "answer" Composable based on the answer data? Obviously the widgets know exactly which kind of data they deal with, but the hierarchy will probably need something like that. It might be nice to prove that out now.

I hope you’ll like this direction as much as I do. Although it might take a bit more time to build everything this way compared to the old Android views, I’m confident it’s the right move and will pay off in the long run.

I do like it! I am though wondering if, to minimize the amount of work we have to do to get to having components that can be shared with the hierarchy, we should just create Composables for the "answers" in the media widgets and hold off on reworking the rest of each widget's UI for now? So here for this example we'd just create and use the VideoWidgetAnswer, but still use View based implementations for the rest of the UI. Obviously here, we could probably just keep VideoWidgetContent as I don't want to throw it away - I just mean for the other widgets we extract the answer UI from.

[grzesiek2010]

I just wanted to clarify this quickly! So basically, if we were building a widget from scratch you'd still create a QuestionWidget implementation, but onCreateWidgetView would return a ComposeView and the majority of the UI would be implemented in Compose and then be hooked up to the QuestionWidget API. That way, we can simply have an @composable for the "answer" part of the view (like you've done with VideoWidgetAnswer) that can be shared between the widget and the hierarchy. Correct?

Correct. It’s great that we can do it this way because it lets us introduce Jetpack Compose step by step. Otherwise, we’d have to rewrite the entire widget architecture, which we’d probably never do since it would be too big of a task.

If that's the case I think we'll also want to go back and rework the widgets we changed in #6534 as it seems like we'd be getting rid of the WidgetAnswerView concept all together.

That’s also correct, and that’s why I mentioned that PR in my comment. It’s a step back, but considering that we’ll probably be reworking many widgets (maybe even all of them), it still makes sense.

Also, do you think we should add a Composable that shows a different "answer" Composable based on the answer data? Obviously the widgets know exactly which kind of data they deal with, but the hierarchy will probably need something like that. It might be nice to prove that out now.

Not based on the answer data, because for example, the VideoWidget uses StringData, just like simple text widgets. We’ll need to determine that using FormEntryPrompt, similar to how WidgetFactory works. What I have in mind right now is a similar factory, but for composables.

I am though wondering if, to minimize the amount of work we have to do to get to having components that can be shared with the hierarchy, we should just create Composables for the "answers" in the media widgets and hold off on reworking the rest of each widget's UI for now?

I understand that I was also torn between those two solutions, but the temptation to rewrite the entire widget using Jetpack Compose turned out to be too strong 😃. I’ll keep that in mind, so if doing the same for other widgets turns out to be too time-consuming, I can always limit it to the answer view.

[seadowg]

Not based on the answer data, because for example, the VideoWidget uses StringData, just like simple text widgets. We’ll need to determine that using FormEntryPrompt, similar to how WidgetFactory works. What I have in mind right now is a similar factory, but for composables.

Good point: question type is the important thing really. And like everything in Compose, that would just end up being another Composable!

I understand that I was also torn between those two solutions, but the temptation to rewrite the entire widget using Jetpack Compose turned out to be too strong 😃. I’ll keep that in mind, so if doing the same for other widgets turns out to be too time-consuming, I can always limit it to the answer view.

Agreed. Doesn't need to be a rule, just something to keep in mind.

[seadowg]

I've got some initial thoughts on the Compose code that's probably worth discussing before reviewing any further: I imagine it might lead to structure changes.

[seadowg]

Just some more Compose structural things to discuss!

[seadowg]

#6925 (comment)

[seadowg]

Just a couple of tweaks it would be cool to make here, but looks great!

One thought to discuss: I think as we move forward with Compose we should try and push as much testing as reasonable into the component ("unit") level. I think not reworking all of that here and keeping the testing structure as-is was the right call for this change though.

[seadowg]

One quick tweak that we could make here and in VideoWidgetContent: you could place this call in a Box (or some other simple container) that adds the margin so that can be a concern of the layout code here rather than in the passed Composable declaration.

[dbemke]

It seems that the preview image isn't generated on older devices- on Android 8.1 and 7.1.1 there's the preview but it is gray (no picture).

[grzesiek2010]

It seems that the preview image isn't generated on older devices- on Android 8.1 and 7.1.1 there's the preview but it is gray (no picture).

This should work fine now.

[WKobus]

Tested with success

Verified on Android 16, 11, 10

Verified cases:

• Big sized attachment in video widget
• Video widget on field list
• Media displayed in video widget after savepoint
• Opening video preview using different apps
• Regression check on Video and External video widgets

[dbemke]

Tested with success

Verified on Android 10, 8.1, 7.1.1