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

add autoStop #9

Merged
merged 2 commits into from Aug 15, 2019

Conversation

@Jiawen-Zhang
Copy link
Contributor

commented Aug 12, 2019

I have added a feature named "autoStop". This will stop recording automatically according to the average power. If the user turns on this feature, the recording process will stop automatically after the power is continuously lower than -10.


This change is Reviewable

@codecov

This comment has been minimized.

Copy link

commented Aug 12, 2019

Codecov Report

Merging #9 into master will decrease coverage by 0.98%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #9      +/-   ##
=========================================
- Coverage   39.49%   38.5%   -0.99%     
=========================================
  Files           4       4              
  Lines         314     322       +8     
=========================================
  Hits          124     124              
- Misses        190     198       +8
Impacted Files Coverage Δ
SpeechRecognizerButton/SFButton.swift 26.69% <0%> (-0.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 118a856...26c93aa. Read the comment docs.

@alexruperez
Copy link
Owner

left a comment

@Jiawen-Zhang thank you very much for your contribution, if you can change these comments and we can merge it.

@@ -212,6 +215,16 @@ open class SFButton: UIButton {
}
let normalizedValue = pow(10, averagePower / 20)
waveformView?.updateWithLevel(CGFloat(normalizedValue))

if(averagePower < -10){

This comment has been minimized.

Copy link
@alexruperez

alexruperez Aug 14, 2019

Owner

Can you add this spaces? if (averagePower < -10) {


if(averagePower < -10){
counter += 1
if(counter / 60 >= 2){

This comment has been minimized.

Copy link
@alexruperez

alexruperez Aug 14, 2019

Owner

Can you add this spaces? if (counter / 60 >= 2) {

endRecord()
}
}
else{

This comment has been minimized.

Copy link
@alexruperez

alexruperez Aug 14, 2019

Owner

Can you write this else like:
} else {
Instead of:

}
else{

please?

@@ -64,7 +65,8 @@ open class SFButton: UIButton {
@IBInspectable public var highlightedColor: UIColor?
@IBInspectable public var disabledColor: UIColor?
@IBInspectable public var highlightedAlpha: CGFloat = 0.5


private var counter:NSInteger = 0

This comment has been minimized.

Copy link
@alexruperez

alexruperez Aug 14, 2019

Owner

Can you add this space? counter: NSInteger

if(averagePower < -10){
counter += 1
if(counter / 60 >= 2){
endRecord()

This comment has been minimized.

Copy link
@alexruperez

alexruperez Aug 14, 2019

Owner

Can you add a unit test that covers this code please?

This comment has been minimized.

Copy link
@Jiawen-Zhang

Jiawen-Zhang Aug 15, 2019

Author Contributor

I have added the spaces and changed the style for the "if" condition, but I have no idea to do the unit test if I don't introduce any new function since these variables are all private. Could you please give me some hint for adding a unit test to cover this part of code since I am new to swift developing. In my opinion, it is needless if we introduce some additional function in order to do the unit test. Thanks.

This comment has been minimized.

Copy link
@alexruperez

alexruperez Aug 15, 2019

Owner

I agree with you, we will solve coverage with a UI Test in the future that waits for the auto stop or something like that. Push the spaces changes and we can merge. 👌 Thanks for the effort! 🙌

@alexruperez alexruperez merged commit af896f6 into alexruperez:master Aug 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.