Improve media displayed in widgets [BarcodeWidget + ArbitraryFileWidget + ExArbitraryFileWidget] #6964
Conversation
grzesiek2010
force-pushed
the
COLLECT-6234_barcode2
branch
from
f187aad to
1a7aeac
Compare
grzesiek2010
force-pushed
the
COLLECT-6234_barcode2
branch
from
1a7aeac to
c4d4e48
Compare
grzesiek2010
changed the title
seadowg
left a comment
seadowg
left a comment
There was a problem hiding this comment.
Some general comments on the structure before going any deeper!
| onView(withTagValue(Matchers.`is`("ArbitraryFileWidgetAnswer"))) | ||
| .check(matches(CoreMatchers.not(isDisplayed()))) | ||
| composeRule | ||
| .onNodeWithClickLabel(ApplicationProvider.getApplicationContext<Application>().getString(R.string.open_file)) |
There was a problem hiding this comment.
Maybe we could add an overload for onNodeWithClickLabel that takes a @StringRes Integer to avoid this noise in the tests?
| } | ||
|
|
||
| if (!isAnswerHidden) { | ||
| widgetAnswer() |
There was a problem hiding this comment.
I'm wondering after seeing a few of these now if just calling WidgetAnswer here directly would actually make more sense given we're always passing that. What reason do you see right now for the answer component being a "dependency" of the content components as opposed to just a part of their hierarchy?
| import org.odk.collect.android.utilities.MediaUtils | ||
| import org.odk.collect.android.utilities.QuestionMediaManager | ||
|
|
||
| class ArbitraryFileWidgetAnswerViewModel( |
There was a problem hiding this comment.
I'm not sure about having a ViewModel and a Delegate here for arbitrary file widgets - a ViewModel is really just a delegate that specifically handles state and events as well as having a special lifecycle (in Android). I think we should only have a ViewModel here (as that's more canonical in Android) and maybe just make getAnswer a static helper or extenson somewhere (as it's stateless).
Also, is there any scope for having one view model that both the video and file widgets could share? There seems to be some shared dependencies and interactions.
2 participants
Work towards #6234
Blocked by #6925Why is this the best possible solution? Were any other approaches considered?
It's the continuation of #6925
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?
Please perform full regression testing on the three updated widgets:
BarcodeWidget,ArbitraryFileWidget, andExArbitraryFileWidget. They have been reworked, so they need to be tested thoroughly.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:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest