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

RecordingControls should use TimeControls #30

Closed
Tracked by #52
ShrimpCryptid opened this issue Jun 20, 2023 · 3 comments · Fixed by #102
Closed
Tracked by #52

RecordingControls should use TimeControls #30

ShrimpCryptid opened this issue Jun 20, 2023 · 3 comments · Fixed by #102
Assignees
Labels
internals Tech debt, refactoring, dependencies, etc.

Comments

@ShrimpCryptid
Copy link
Contributor

Use Case

Internal-only, non-user affecting. Reduce code complexity by having TimeControls and RecordingControls' advancement loops use the same underlying logic, just with separate callback behavior.

Acceptance Criteria

  • When recording, the interval loop should be completely contained in TimeControls.
  • Any side-effects during recording should be done as callbacks.
  • Wrapping or termination behavior should be handled via callbacks.

Details

e.g.:
private playTimeSeries(onNewFrameCallback: () => void, onCompleted: () => void, intervalMs: number): void {}

  • Note: ok to have separate loops for timeout vs. interval if needed
  • separate public entrypoint for recording
@ShrimpCryptid ShrimpCryptid self-assigned this Jun 20, 2023
@ShrimpCryptid ShrimpCryptid added the new feature New feature or request label Jun 20, 2023
@toloudis
Copy link
Contributor

toloudis commented Jun 20, 2023

also to consider:
TimeControls was originally intended to encapsulate all time handling. It also encapsulates the user controls and the handlers we create. Consider a separate module called TimeController that just has the playback loop logic (with some convenient api on top of it) and TimeControls is ONLY a container for the controls and setting up their handlers, which then manipulate the TimeController.

@toloudis
Copy link
Contributor

toloudis commented Jun 22, 2023

Echoing a comment from Cameron, this ticket is also a good opportunity to pull out the code that gets DOM controls and binds handlers, and put it back into main, so that TimeControls and RecordingControls just contain the handlers themselves. In which case I would change the class names to Controller instead of Controls. Similar in spirit to the above comment.

@toloudis toloudis added internals Tech debt, refactoring, dependencies, etc. and removed new feature New feature or request labels Jun 22, 2023
@ShrimpCryptid
Copy link
Contributor Author

Can we use just one timeout? (timeout instead of setInterval?)

@toloudis toloudis mentioned this issue Aug 30, 2023
11 tasks
@ShrimpCryptid ShrimpCryptid linked a pull request Sep 28, 2023 that will close this issue
ShrimpCryptid added a commit that referenced this issue Oct 2, 2023
Adds functionality for the Export feature, allowing image sequences to be exported from the currently loaded dataset.

Closes #79, #30, and #72.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internals Tech debt, refactoring, dependencies, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants