Add button to toggle flash on/off #59

Didier116 · 2018-02-02T17:53:26Z

This will allow the user to activate flash when recording by adding a flash toggle button on the top left (See the screenshot below).

The developer can also hide flash toggle using builder.noFlashToggle(); while creating CaptureConfiguration. By default, flash toggle switch is visible and off.

It's my first pull request, so I must admit that I was very inspired by Kevalpatel's work done on the front and rear facing camera switch and on his PR message ;)

Refs Pull#34 and Issue#13

codecov-io · 2018-02-02T18:06:45Z

Codecov Report

Merging #59 into master will decrease coverage by 2.39%.
The diff coverage is 41.17%.

@@             Coverage Diff             @@
##             master      #59     +/-   ##
===========================================
- Coverage     71.05%   68.66%   -2.4%     
- Complexity      127      131      +4     
===========================================
  Files            15       15             
  Lines           722      785     +63     
  Branches         49       64     +15     
===========================================
+ Hits            513      539     +26     
- Misses          184      214     +30     
- Partials         25       32      +7

Impacted Files	Coverage Δ	Complexity Δ
...ile/landscapevideocapture/camera/NativeCamera.java	`6.25% <0%> (-1.6%)`	`1 <0> (ø)`
...le/landscapevideocapture/camera/CameraWrapper.java	`68% <0%> (-1.39%)`	`19 <0> (ø)`
.../landscapevideocapture/recorder/VideoRecorder.java	`95.27% <100%> (+0.03%)`	`26 <1> (ø)`	⬇️
.../landscapevideocapture/preview/CapturePreview.java	`96.61% <100%> (+0.05%)`	`12 <1> (+1)`	⬆️
...configuration/PredefinedCaptureConfigurations.java	`96.29% <100%> (+0.29%)`	`1 <0> (ø)`	⬇️
...le/landscapevideocapture/VideoCaptureActivity.java	`27.52% <22.22%> (+0.33%)`	`10 <1> (ø)`	⬇️
...e/landscapevideocapture/view/VideoCaptureView.java	`60.18% <23.52%> (-6.85%)`	`14 <1> (+1)`
...deocapture/configuration/CaptureConfiguration.java	`85.07% <76.19%> (-1.66%)`	`22 <2> (+2)`

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 5c90229...45736ab. Read the comment docs.

kheirus · 2018-02-04T23:40:27Z

Good job guy @Didier116 , hope that will be validate soon, it will be nice that we can use this feature without re-implement it from scratch.
Thank u for your implication anyway ;)

…g methods

…Plus)

JeroenMols

Looks good, please have a look at the comments.

Build is failing because of timeout on Android emulators in Travis, let's wait for a rebuild (and get lucky) or else I'll fix it structurally

JeroenMols · 2018-03-18T20:06:40Z

...y/src/androidTest/java/com/jmolsmobile/landscapevideocapture/recorder/VideoRecorderTest.java

+
+    // name attribute is optional, provide an unique name for test
+    // multiple parameters, uses Collection<Object[]>
+    @Parameterized.Parameters()


This is overkill, there is no need to check that every interaction with the recorder uses the right value for the flash. Better just to always pass in false, and to add one extra test that validates that true is properly propagated. (or multiple ones if there are different propagations)

Doing it this way basically defocusses every existing test and adds an extra thing for them to implicitly test. That is what I want to avoid: every test should only test for exactly one thing.

So remove parametrized, always use flash = false and add one (or more) tests to only check if a true value is properly propagated,

JeroenMols · 2018-03-18T20:11:47Z

...ava/com/jmolsmobile/landscapevideocapture/configuration/CaptureConfigurationBuilderTest.java

+        CaptureConfiguration configuration = new Builder(MOCK_RESOLUTION, MOCK_QUALITY).setFlashOption(PredefinedCaptureConfigurations.FlashOption.HIDE).build();
+
+        assertThat(configuration.getAllowFlashToggle()).isFalse();
+        assertThat(configuration.getIfFlashStartOn()).isFalse();


Please split each and every one of those tests in two. Every test should only have exactly one assertion/verification.

Reason is that with multiple asserts: tests can fail for more than one reason. So if they fail you need to actually do an investigation to see what went wrong. With only one assert the root cause for failure is crystal clear (especially if you gave the test a great name). Second benefit is that the tests stops running after the first failure which could be annoying if you actually fixed bug number 1, but there is still a second one.

JeroenMols · 2018-03-18T20:12:12Z

...ava/com/jmolsmobile/landscapevideocapture/configuration/CaptureConfigurationBuilderTest.java

 import com.jmolsmobile.landscapevideocapture.configuration.PredefinedCaptureConfigurations.CaptureResolution;

 import org.junit.Test;
 import org.junit.runner.RunWith;

+import android.support.test.runner.AndroidJUnit4;


Unneccessary import?

I think it's on other " committing reformatting changes" error.

JeroenMols · 2018-03-18T20:13:53Z

library/src/main/java/com/jmolsmobile/landscapevideocapture/VideoCaptureActivity.java

 public class VideoCaptureActivity extends Activity implements RecordingButtonInterface, VideoRecorderInterface {

    public static final int RESULT_ERROR = 753245;
+


General tip: please avoid committing reformatting changes, they clutter you PR and make it harder to review + they clutter your git history.

You don't have to change this here

My bad, I did not pay enough attention. Sorry for that

JeroenMols · 2018-03-18T20:21:48Z

library/src/main/java/com/jmolsmobile/landscapevideocapture/camera/NativeCamera.java

+
+    public void setFlash(boolean isFlashOn){
+
+        if(camera != null) {


When setFlash() is being called the camera should already be open (camera != null) and params should already be accessed before (params != null) hence it doesn't make any sense to check for this.

I prefer to program offensively: if something happens that you didn't expect your code should crash so you are aware of that. (otherwise it fails silently) In this case a consumer of setFlash() could call it and expect it to work, but internally the method would fail. Not checking for null, makes the contract more strict and enforces a single way to use this api.

Please remove null check for camera and params, the null check for flashmodes is valid

Very interesting remark, I thought to do well, I take note and correct.

JeroenMols · 2018-03-18T20:23:17Z

.../src/main/java/com/jmolsmobile/landscapevideocapture/configuration/CaptureConfiguration.java

@@ -155,6 +158,20 @@ public boolean getAllowFrontFacingCamera() {
        return allowFrontFacingCamera;
    }

+    /**
+     * @return If front flash toggle must be displayed before capturing video


Does "front" need to be here? Flash also works for back camera, right?

JeroenMols · 2018-03-18T20:23:23Z

.../src/main/java/com/jmolsmobile/landscapevideocapture/configuration/CaptureConfiguration.java

+    }
+
+    /**
+     * @return If front flash must be start on


Does "front" need to be here? Flash also works for back camera, right?

...voiceless...

JeroenMols · 2018-03-18T20:25:45Z

...ava/com/jmolsmobile/landscapevideocapture/configuration/PredefinedCaptureConfigurations.java

@@ -103,4 +103,9 @@ public int getBitrate(CaptureQuality quality) {
        }

    }
+
+    public enum FlashOption {
+        HIDE,HIDE_ON, START_OFF, START_ON;


Wouldn't it be better to call: FlashOption: NO_FLASH, ALWAYS_ON, START_OFF, START_ON

JeroenMols · 2018-03-18T20:31:28Z

Sorry for my late review, things have been extremely hectic

JeroenMols · 2018-04-05T18:00:18Z

Any chance you could have a look at my comments?

Didier116 · 2018-04-06T14:34:16Z

Yes, sorry, I'm a little busy right now but I'll try to look it by next week.

Didier116 · 2018-04-30T13:57:15Z

I think I considered all your remarks.
So, what's the next step? Is there anything I've forgotten or can I do more?

Didier116 · 2018-09-28T14:20:05Z

What's wrong with this PL ?

Add button to toggle flash on/off

daf724a

Didier116 added 9 commits February 6, 2018 12:26

Add spinner to choose flash option

534ebba

Flash icone start on or off depending on isFlashStartOn Option

ad63247

setFlash call on startRecording()

4b51dcd

Rename allowFlash variable to allowFlashToggle

1d0544e

Add useFlash parameters to onRecordButtonClicked

91fbaf2

Add FlashStatus as Parameterized to toggleRecording and startRecordin…

38e50ea

…g methods

Add getAllowFlashToggle an getIFStartOn Tests

e0c832f

update with setFlashOption method used and description

2a2e885

Start Flash before recording (prevent crash with some device like One…

c3c583f

…Plus)

JeroenMols reviewed Mar 18, 2018

View reviewed changes

Didier116 added 5 commits April 13, 2018 21:06

Rename FlashOption

61c7366

Remove unused parameter (isFlashOn/useFlash)

985d70d

Remove null check for camera and params

6d3cbcc

Update Comments (Flash also works for back camera)

22f17d6

Remove unused parameter (isFlashOn/useFlash)

45736ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button to toggle flash on/off #59

Add button to toggle flash on/off #59

Didier116 commented Feb 2, 2018

codecov-io commented Feb 2, 2018 •

edited

Loading

kheirus commented Feb 4, 2018

JeroenMols left a comment

JeroenMols Mar 18, 2018

JeroenMols Mar 18, 2018

JeroenMols Mar 18, 2018

Didier116 Apr 13, 2018

JeroenMols Mar 18, 2018

Didier116 Apr 13, 2018

JeroenMols Mar 18, 2018

Didier116 Apr 13, 2018

JeroenMols Mar 18, 2018

Didier116 Apr 13, 2018 •

edited

Loading

JeroenMols Mar 18, 2018

Didier116 Apr 13, 2018

JeroenMols Mar 18, 2018

Didier116 Apr 13, 2018

JeroenMols commented Mar 18, 2018

JeroenMols commented Apr 5, 2018

Didier116 commented Apr 6, 2018

Didier116 commented Apr 30, 2018

Didier116 commented Sep 28, 2018

		public class VideoCaptureActivity extends Activity implements RecordingButtonInterface, VideoRecorderInterface {

		public static final int RESULT_ERROR = 753245;


		public void setFlash(boolean isFlashOn){

		if(camera != null) {

Add button to toggle flash on/off #59

Are you sure you want to change the base?

Add button to toggle flash on/off #59

Conversation

Didier116 commented Feb 2, 2018

codecov-io commented Feb 2, 2018 • edited Loading

Codecov Report

kheirus commented Feb 4, 2018

JeroenMols left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Didier116 Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeroenMols commented Mar 18, 2018

JeroenMols commented Apr 5, 2018

Didier116 commented Apr 6, 2018

Didier116 commented Apr 30, 2018

Didier116 commented Sep 28, 2018

codecov-io commented Feb 2, 2018 •

edited

Loading

Didier116 Apr 13, 2018 •

edited

Loading