Skip to content
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

Show recording duration and waveform #4241

Merged
merged 17 commits into from Dec 2, 2020
Merged

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Nov 24, 2020

Closes #4152
Closes #4153
Blocked by #4222

What has been done to verify that this works as intended?

New tests and verified manually.

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

Comments inline.

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?

I think we want to skip QA on this as it really only touches the new features.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • 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

@seadowg seadowg changed the title Show recording duration Show recording duration and waveform Nov 25, 2020
@@ -57,6 +57,9 @@ jobs:
- run:
name: Run async unit tests
command: ./gradlew -PdisablePreDex --no-daemon --max-workers=2 async:testDebug
- run:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have a test file in strings we should really be running it on CI.

import java.util.function.Supplier
import kotlin.coroutines.CoroutineContext

open class CoroutineScheduler(private val foregroundContext: CoroutineContext, private val backgroundContext: CoroutineContext) : Scheduler {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't always need deferred tasks so thought I'd split the implementations. I think long term it might make sense to split the interfaces but lets see how it evolves.

@@ -7,12 +7,12 @@ import java.io.File

/**
* Interface for a ViewModel that records audio. Can only record once session
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're now in a better place with this abstraction. Adding time/amplitude helped it make more sense as it forced the key data exposed to be the current "session" details.

@@ -320,6 +320,8 @@ dependencies {
// Better "Subjects" for Rx:
implementation "com.jakewharton.rxrelay2:rxrelay:2.1.1"

implementation 'com.github.Armen101:AudioRecordView:1.0.2'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the simplest waveform library I could find that still looked supported. I think we may want to improve this in the future or add our own flair.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a size increase of .2mb from 1.28.4 to this branch. Not sure how much is from this lib but either way it seems ok.

@@ -128,7 +128,7 @@ public void externalApp_ShouldPopulateFields() throws IOException {
resultIntent.setClipData(clipData);
resultIntent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION);
intending(not(isInternal())).respondWith(new Instrumentation.ActivityResult(Activity.RESULT_OK, resultIntent));
onView(withText("This is buttonText")).perform(click());
onView(withText("This is buttonText")).perform(nestedScrollTo(), click());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests were failing due to the audio widget getting slightly taller and it forcing other items in the test to be scrolled off screen.

@@ -3,58 +3,72 @@ package org.odk.collect.testshared
import org.odk.collect.async.Cancellable
import org.odk.collect.async.Scheduler
import org.odk.collect.async.TaskSpec
import java.util.LinkedList
import java.util.function.Consumer
import java.util.function.Supplier

class FakeScheduler : Scheduler {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some of the tests this needed to be a bit more advanced so it now actually keeps scheduled tasks in queues/lists so it runs them in the correct order and can run repeated tasks.

@seadowg seadowg marked this pull request as ready for review December 1, 2020 12:56
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

There's a rotation issue that's deeper than I thought -- after rotating during recording, it's not possible to see the amplitude before exiting the form. Even starting a new recording doesn't reset things. I think that should be addressed before merge.

Regarding QA, I think we should do a pass over the amplitude/time functionality before freeze next Weds even if small visual tweaks will be done after that. There might be other weird state things I didn't notice and it'd be better to get those addressed before regression testing begins. Are you thinking you'd like to do that after your design pairing on Friday?

@@ -320,6 +320,8 @@ dependencies {
// Better "Subjects" for Rx:
implementation "com.jakewharton.rxrelay2:rxrelay:2.1.1"

implementation 'com.github.Armen101:AudioRecordView:1.0.2'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a size increase of .2mb from 1.28.4 to this branch. Not sure how much is from this lib but either way it seems ok.

@seadowg
Copy link
Member Author

seadowg commented Dec 2, 2020

@lognaturel fixed! It looks like it's a bug/quirk in the library.

I see a size increase of .2mb from 1.28.4 to this branch. Not sure how much is from this lib but either way it seems ok.

Yeah would be really nice if we could replace with something simple we write ourselves.

@lognaturel lognaturel merged commit fa36423 into getodk:master Dec 2, 2020
@seadowg seadowg deleted the timecode branch December 2, 2020 17:41
@lognaturel
Copy link
Member

lognaturel commented Dec 2, 2020

@getodk/testers Heads up that we'll get these recording visualization features to you after a few more visual tweaks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can see a waveform while recording Can see how long a recording has been going on
2 participants