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
Reworking Audio widget #4006
Reworking Audio widget #4006
Conversation
abb55c3
to
289cc79
Compare
@SaumiaSinghal do you think it'd be plausible to look at fixing #4005 as part of this? If it seems like a lot of extra work then no worries but thought it'd be good to look at while you're in the "neighbourhood". |
yes sure @seadowg I will look at it |
Ah I'm so sorry @SaumiaSinghal I meant #4008. There were two issues but only one was a regression. I see you've fixed #4005 now which is good but if you can look at #4008 it is higher priority. |
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.
Just done a first pass but this looks ace 🙌. Liking the use of fakes and pulling out interfaces to deal with the file stuff.
Just a few comments/tweaks inline.
destinationName = RandomString.make(); | ||
@Test | ||
public void getAnswer_whenPromptHasAnswer_returnsAnswer() { | ||
AudioWidget widget = createWidget(promptWithAnswer(new StringData(destinationPath))); |
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 better to just have the values inline (destinationPath
) rather than defined anywhere. Easier to see in the test that the value is just a string.
@@ -0,0 +1,7 @@ | |||
package org.odk.collect.android.widgets.interfaces; | |||
|
|||
public interface MediaManagerListener { |
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.
Not sure the naming is right here. It's not really a "listener". Maybe the interface is QuestionMediaManager
and the implementation is something else?
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.
Yes right the implementation mostly concerns with the deletion or replacing the file at the current index. But I think I might need to extend this implementation for working on other widgets
binding.captureButton.widgetButton.setTextSize(TypedValue.COMPLEX_UNIT_DIP, answerFontSize); | ||
binding.chooseButton.widgetButton.setTextSize(TypedValue.COMPLEX_UNIT_DIP, answerFontSize); | ||
|
||
binding.captureButton.widgetButton.setText(getContext().getString(R.string.capture_audio)); |
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.
Feels like the button text should just live in a layout?
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 use
`
<include
android:id="@+id/choose_button"
layout="@layout/widget_button"/>`
in audio_widget.xml file. So I have added the button text here.
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 I'd be happier duplicating the layout here than having hardcoded text set programmatically. My thinking is that it's good to be able to see the layout as it will appear (with the text) in the design preview for it.
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.
For the other file widgets, I was using the same widget_button.xml
layout. So, I should have a separate layout for all of them too?
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 instead of sharing an include
maybe we could create a custom button class with the shared values. Or, create a style that can be shared. These would let us avoid duplication but avoid having to set the text programmatically (which I'd like to avoid as much as possible).
collect_app/src/main/java/org/odk/collect/android/audio/AudioPlayerViewModel.java
Show resolved
Hide resolved
@@ -13,6 +13,7 @@ | |||
private Runnable foregroundTask; | |||
private Runnable backgroundTask; | |||
private Boolean cancelled = false; | |||
private Boolean isProgressUpdating = false; |
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 like what's happening here but I think the variable/method should be named something more related to the scheduler than the test here. Maybe isRepeatRunning
?
@@ -6,6 +6,15 @@ | |||
<item name="android:textStyle">normal</item> | |||
</style> | |||
|
|||
<style name="Widget.Collect.Button.Layout" parent="Widget.Collect.Button.Custom"> |
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.
The naming scheme here is pretty confusing as by convention we use Widget
at the beginning but that's referring to the general definition of the word rather than a "question widget". Maybe the name here could be Widget.Collect.Button.WidgetAnswer
?
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.
Oh I thought the same initially.
@@ -24,7 +24,7 @@ | |||
* remember an original media file answer (no matter how many times this answer is replaced), in order | |||
* to be able to restore the original answer in case of ignoring changes. | |||
*/ | |||
public enum MediaManager { | |||
public enum MediaManager implements QuestionMediaManager { |
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 don't think this is quite right after a second look. As much as we can we want to avoid static singletons. If there is some state that we want to keep shared at an application level (i.e. between multiple activities or services) then we should use Dagger to setup and inject a singleton (using @Singleton
).
In this case I don't believe the state needs to persist outside of form entry so it really only needs to last as long as the Activity. This means we could confuse ourselves by ending up with state hanging around between form entry "sessions". A good way to share state like this would be to use a view model where the ViewModelProvider
uses the activity as its owner. Does that make sense as a way forward?
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.
If so, it might be good to discuss the plans/approach here so we can come up with something that makes sense as it's a little awkward I think!
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.
@seadowg If I use the ViewModel
then I would be creating it using the widget context, and it will be there for all the widgets on the screen?
I feel like avoiding using QuestionMediaManager
too and using the previous implementation. That would mean there would be additional unit tests for the widgets. Will that be fine?
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 if that's simpler to do right now that's a good plan. Maybe we can move the logic to a viewmodel in the future.
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.
@SaumiaSinghal and I talked about this and agreed it made sense to experiment with using a view model to handle files.
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.
Getting closer and closer. This is a really hard one to strucutre...
@@ -370,7 +377,7 @@ public Factory(Analytics analytics) { | |||
@NonNull | |||
@Override | |||
public <T extends ViewModel> T create(@NonNull Class<T> modelClass) { | |||
return (T) new FormSaveViewModel(System::currentTimeMillis, new DiskFormSaver(), analytics); | |||
return (T) new FormSaveViewModel(System::currentTimeMillis, new DiskFormSaver(), analytics, MediaManager.INSTANCE); |
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.
Maybe this hasn't come through in previous comments and conversations but I'm very against the MediaManager
being a singleton. Just to make it very clear I think the MediaManager
should be a class
(not an enum
) and that this line should be:
return (T) new FormSaveViewModel(System::currentTimeMillis, new DiskFormSaver(), analytics, new MediaManager());
As we talked about before we could get in trouble with the enum
singleton implementation as its state will be carried between different instances of the FormEntryActivity
. Also in tests the singleton would not be recreated for each test (as the static state is retained throughout the process) so any state in the MediaManager
set up in one test might inadvertently affect another.
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.
Oh right! I didn't think about it. It will cause problems when writing tests for other file widgets.
@@ -98,6 +102,12 @@ public void setUp() { | |||
when(activityAvailability.isActivityAvailable(any())).thenReturn(true); | |||
} | |||
|
|||
@After | |||
public void tearDown() { | |||
widgetActivity.originalFiles.clear(); |
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.
You shouldn't need to do this as each test will get a new instance of TestWidgetActivity
.
@@ -2674,6 +2675,16 @@ public void onCancelFormLoading() { | |||
finish(); | |||
} | |||
|
|||
@Override | |||
public void deleteOriginalFile(String questionIndex, String fileName) { |
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 we're getting close with the structure but I don't like that the FormEntryActivity
implements the QuestionMediaManager
. I think instead:
- the
AudioWidget
should take aQuestionMediaManager
in as a dependency at it's constructor rather than casting thecontext
to one. - the
WidgetFactory
should also take aQuestionMediaManager
in as one of the args tocreateWidgetFromPrompt
so it can pass it to any widget that needs it. FormSaveViewModel#getMediaManager
should return aQuestionMediaManager
not aMediaManager
FormSaveViewModel#getMediaManager
should be passed as theQuestionMediaManager
argument toWidgetFactory#createWidgetFromPrompt
QuitFormDialogFragment
should callviewModel.getMediaManager().revertChanges
rather than relying on the static singleton instance
I'm not sure if I like that currently the QuestionMediaManager
is simply owned by the FromSaveViewModel
and then exposes it. I'm thinking that potentially the FormSaveViewModel
could implement the QuestionMediaManager
interface and then there would be no need for the MediaManager
class. In that world the "revert" call (in QuiteFormDialogFragment
could just be part of the logic FormSaveViewModel#removeTempInstance
.
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.
Nice idea @seadowg, and Thanks for explaining so well. I implemented all the functionality of MediaManager
in FormSaveViewModel
, as you said if we have the FormSaveViewModel
implements QuestionMediaManager
, then MediaManager
class is no longer required.
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.
@SaumiaSinghal looks like unit tests are failing
af09cd9
to
fcfb6b9
Compare
Sorry for that @seadowg |
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.
Like where this is going! It feels like this is working towards a better pattern for how we deal with files, external apps etc with widgets (basically listener interfaces with android arch components handling the actual implementations).
@@ -126,6 +127,7 @@ public void removeTempInstance() { | |||
FileUtils.purgeMediaPath(instanceFolder); | |||
} | |||
} | |||
releaseMediaManager(); |
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.
Do we need to do this in onCleared()
as well? Have you played around with the behaviour when "Don't save activities" is enabled? It might be good to give that a go and see if we need to handle the case when we leave the app, Android kills the process, and then we return to the app. Would files get left undeleted but on in the form in this case? Would things break?
Let me know if you have questions about trying that out!
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 an interesting case. I will try that out with all the media widgets.
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 tried clearing the originalFiles
and recentFiles
HashMap in onCleared()
, when Don't keep activities
is turned on. I didn't find any anomaly in saving answers to the widgets, as I think, the answer is already saved with the widget, so clearing the files won't cause any break. Am I missing some test case here?
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.
Here's the problem I found:
- Enabled "Don't keep activities"
- Open the "All widgets form"
- Go to the file widget
- Select a file
- Select a different file
- Go to the hierarchy view with the arrow button
- Press "Go To End"
- Save and exit the form
If you now go to the "Device File Explorer" and check the instance dir (sdcard/odk/instances
and then the newest dir) you'll see both the files you've selected are still in the directory. I'm guessing this is because when the activity is killed the view model state is wiped with the activity and the originalFiles
list will be empty when we return to the FormEntryActivity
from the hierarchy screen. Before (on master
) this worked because the MediaManager
was a process level singleton and so state would be retained.
It feels to me like it would be a lot nicer if when the form was saved or (or ignore changes is hit) it just deleted any files not being used in the form but that could require a lot of rework. I'm thinking we should use the new Saved State module to persist originalFiles
between activity instances (as opposed to going back to a process singleton) and then file an issue around carrying out the rework.
I'd like to pull in @lognaturel and @grzesiek2010 for sanity checks here though!
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 case does seem possible if someone needs to get clarification on what file to attach, for example. Certainly it'd be best not to have a known regression in behavior.
We haven't used the saved state module yet. It might be simpler to use onSavedInstanceState
. But yes, saving originalFiles
between activity instances for now seems good.
collect_app/src/main/java/org/odk/collect/android/formentry/saving/FormSaveViewModel.java
Outdated
Show resolved
Hide resolved
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.
Needs discussion as mentioned in comment
59dafaa
to
e06f970
Compare
a4b6c91
to
35225c3
Compare
collect_app/src/main/java/org/odk/collect/android/formentry/saving/FormSaveViewModel.java
Outdated
Show resolved
Hide resolved
releaseMediaManager(); | ||
originalFiles.clear(); | ||
recentFiles.clear(); | ||
|
||
stateHandle.set(ORIGINAL_FILES, originalFiles); |
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.
Do we need to do this if we're also handling it in markOriginalFileOrDelete
(same for the RECENT_FILES
)?
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.
Hi @seadowg! Do you mean if I need the onCleared
method or not?
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 don't think we need anything in here.
collect_app/src/test/java/org/odk/collect/android/formentry/audit/FormSaveViewModelTest.java
Outdated
Show resolved
Hide resolved
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.
Still think the tests could be reworked a little further. Again, I think the tests for driving out the use of SavedStateHandle shouldn't have to check what's actually in the instance being used by the view models. The test should just check that the viewmodel behaves as expected (i.e. clearing up files on save) when it is recreated.
collect_app/src/test/java/org/odk/collect/android/formentry/audit/FormSaveViewModelTest.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/saving/FormSaveViewModel.java
Outdated
Show resolved
Hide resolved
releaseMediaManager(); | ||
originalFiles.clear(); | ||
recentFiles.clear(); | ||
|
||
stateHandle.set(ORIGINAL_FILES, originalFiles); |
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 don't think we need anything in here.
@getodk-bot label "needs testing" I think we're just in a back and forth here around some tweaks to the code but would be good to get started testing this as it'd be nice to include in 1.28. |
} | ||
|
||
@Test | ||
public void whenFormSaverFinishes_onRecreatingFormSaveViewModel_mediaManagerIsCleared() { |
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'm not sure you need this test and whenFormSaverFinishes_mediaManagerIsCleared
. Could you explain it or do you think we should just delete this one?
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 we can delete whenFormSaverFinishes_mediaManagerIsCleared
. We do not have any other test to check whether mediaManager is released when form save finishes. I'm not sure is it is really important to check whether all files get deleted when form save task is finished.
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.
Yup I agree. I think my comment below lays out one test that would capture what whenFormSaverFinishes_mediaManagerIsCleared
is trying to test.
Map<String, String> originalFiles = savedStateHandle.get(FormSaveViewModel.ORIGINAL_FILES); | ||
Map<String, String> recentFiles = savedStateHandle.get(FormSaveViewModel.RECENT_FILES); | ||
|
||
assertThat(originalFiles.isEmpty(), equalTo(true)); |
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.
Instead of verifying the files here I think this test should run saveForm
again and check that deleteImageFileFromMediaProvider
is only called the first time. Does that make sense? Again, this is to test the intended behaviour rather than an implementation detail of that behaviour.
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.
To verify calls to deleteImageFileFromMediaProvider()
, I should use actual new DisFormSaver()
instead of FormSaver
, right?
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.
Oh I just meant like you did in the other test and call saveForm
on the view model. Does that make sense. The test is to verify that if we call save once and then call save again then we don't try and delete the files from the first save. That's my understanding anyway. Does that make sense?
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.
So pseudo code would be:
- set up files to be deleted/saved
- call save on view model
- check files are saved/deleted correctly (using mock of
MediaUtils
) - call save again
- check no files are saved/deleted (you can pass
once()
toverify
to check it method has only been called once)
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.
Hi! the unit test saveForm_runsWith_correctFiles()
is failing. Sorry I meant to name it saveForm_savesCorrectFiles()
. I confirmed the failure is bacause of calling clearMediaFiles()
in OnComplete, but that should not effect the FakeFormSaver, right?
Analytics analytics = mock(Analytics.class); | ||
|
||
when(formController.getAuditEventLogger()).thenReturn(logger); | ||
when(logger.isChangeReasonRequired()).thenReturn(false); | ||
|
||
viewModel = new FormSaveViewModel(() -> CURRENT_TIME, formSaver, analytics); | ||
Map<String, String> originalFiles = new HashMap<>(); |
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.
It looks like you don't need these 4 lines for anything?
|
||
private void clearMediaFiles() { |
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.
When saveToDisk()
task is complete, I just call clearMediaFiles()
. So when I try to verify call to mediaUtils.deleteImageFileFromMediaProvider()
, the test fails.
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.
Ah sorry because the formSaver
is mocked! My bad. The way to verify things are set up correctly then is to assert on the files passed to the fromSaver
(as part of verify
) instead of the call the mediaUtils
.
Tested with success Verified on Android: 7.0, 8.1, 9.0 Verified cases:
|
@mmarciniak90's pass looks quite thorough. I'm going to merge this because it's going to make tracking what's in the final beta a little easier. If any other problem is found, please file an issue. 🙏 Feel free to shake your fist in my general direction if I've caused trouble here. |
@getodk-bot unlabel "needs testing" |
Closes #4005 and Closes #4008
This PR includes rewriting the test coverage and class of Audio Widget
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 PR removes
MediaManager
enum class and the functionality is shifted toFormSaveViewModel
. Hence, it changes the structure ofFileWidgets
and scenarios involving rotation of activities and killing of activities in the middle of saving, replacing or deleting answers in file widgets would be good to test.Do we need any specific form for testing your changes? If so, please attach one.
Any form having Audio Widget like AllWidgetsForm
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 checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.