onRecordedMovies should be optional

[interim17]

Review Time

Small

Problem

Closes #368 onRecordedMovies should be optional for implementations where movie recording is not desired.

Note: snuck in a commit in the prettier config to prevent {" "} < ---- those from appearing, I think it works to prevent them showing up when not needed, can add to sim-website too

Solution

Made the recording and onRecordedMovie props optional. If onRecordedMovie is undefined, set recorder to null, and adjusted references to account for that.

When a callback is provided, the feature will be enabled, and if the prop is undefined then no FrameRecorder functions should be called even if recording is set to true.

Added a function in test-bed to check if recording is enabled or not and display a message if so.

To test you can comment out the prop assigment: // onRecordedMovie={this.onRecordedMovie} Recording buttons should be disabled but viewer should work.

For a further test provide a true value to isRecordingEnabled

                 <RecordMovieComponent
                    isRecording={this.state.isRecording}
                    setIsRecording={this.setIsRecording}
                    isRecordingEnabled={true}
                />

Buttons will be clickable but should have no effect.

• New feature (non-breaking change which adds functionality)

[github-actions[bot]]

jest coverage report 🧪

Total coverage

Status	Category	Percentage	Covered / Total
🔴	Statements	40.39%	2046/5065
🔴	Branches	43.22%	845/1955
🔴	Functions	36.77%	417/1134
🔴	Lines	40.61%	1959/4823

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show files with reduced coverage 🔻

### Reduced coverage | Status | Filename | Statements | Branches | Functions | Lines | | :----: | :--------------- | :----------------- | :----------------- | :-------- | :----------------- | | 🔴 | src/controller | 29.95% | 15.1% | 16.66% | 30.28% | | 🟡 | src/simularium | 60.26% | 57.88% | 49.01% | 61.13% | | 🔴 | FrameRecorder.ts | 27.58% (-1.33% 🔻) | 29.16% (-4.17% 🔻) | 25% | 27.38% (-1.37% 🔻) | | 🔴 | src/viewport | 14.94% | 10.95% | 15% | 14.49% | | 🔴 | index.tsx | 14.94% | 10.95% | 15% | 14.49% | > Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

[toloudis]

Oops my thing says "requested changes" but I meant to do a "general comments".

[interim17]

ok @toloudis this may constitute mission creep but I couldn't bring myself to write the comments on the recording flag because its just not how we handle this type of thing in the rest of the repo.

New changes:

• no more recording prop
• sim controller has startRecording and stopRecording noop methods for the front end to call
• viewport provides controller the functions it needs
• FrameRecorder watches the internal isRecording state more closely since we have removed the flags in the higher components
• RecordMoviesComponent handles isRecording state because Viewer.tsx doesn't need to know it anymore

[meganrm]

looks good, just FYI, there seems to be a slightly different behavior for me on safari than before: I can still hit the record button on safari in the test app but the resulting file won't play.

Hm, that is unfortunate, Safari on my machine still disables the feature. I think I'll merge this PR and maybe write an issue for Safari issues.