Feat: Display Estimated Recorded Time and Add FPS Input#4
Open
vandan0101 wants to merge 1 commit into
Open
Conversation
Introduces a new input field for 'Output FPS' in the main menu. This allows users to specify the desired frames per second for the output timelapse video, which is crucial for estimating the 'Recorded Time' accurately. - Added 'fps' textinput.Model to the application model struct. - Initialized 'fps' input with a default value of 30 in initialModel(). - Updated inputs slice size and focus handling in initialModel(). - Modified viewMenu() to render the 'Output FPS' input field.
Reviewer's GuideAdds an FPS input to the main menu, wires it through to the timelapse subprocess, stores parsed interval/FPS values on the model, and uses them to display an estimated recorded video duration during recording in the TUI. Sequence diagram for starting recording with FPS and displaying recorded timesequenceDiagram
actor User
participant TUIApp
participant TimelapseProcess
User->>TUIApp: Navigate menu and enter interval, duration, fps, output
User->>TUIApp: Press Start Recording
TUIApp->>TUIApp: startRecording()
TUIApp->>TUIApp: Parse interval and duration
TUIApp->>TUIApp: Parse fps input
TUIApp->>TUIApp: Validate fpsFloat > 0
alt invalid fps
TUIApp->>User: Show error state "Invalid FPS value"
else valid fps
TUIApp->>TUIApp: Set stateRecording, startTime
TUIApp->>TUIApp: Store intervalValue, fpsValue
TUIApp->>TimelapseProcess: runTimelapse(interval,duration,fps,output)
end
loop During recording
TimelapseProcess-->>TUIApp: Progress update (frames captured)
TUIApp->>TUIApp: Update m.progress.current
TUIApp->>TUIApp: Compute recordedTime = frames / fpsValue
TUIApp->>User: Render viewRecording with Elapsed and Recorded times
end
Class diagram for updated model struct and related methodsclassDiagram
class model {
%% Fields
textinput.Model interval
textinput.Model duration
textinput.Model output
textinput.Model fps
int focusIndex
[]textinput.Model inputs
spinner.Model spinner
bool recordingDone
string finalMessage
error err
float64 intervalValue
float64 fpsValue
%% Methods (key ones touched by this PR)
startRecording() (tea.Model, tea.Cmd)
runTimelapse(interval float64, duration float64, fps float64, output string) tea.Cmd
updateMenu(msg tea.KeyMsg) (tea.Model, tea.Cmd)
viewMenu() string
viewRecording() string
}
class initialModel {
+initialModel() model
}
initialModel ..> model : constructs
%% Show relationships to external types (simplified)
class textinputModel {
+New() textinput.Model
+Placeholder string
+CharLimit int
+Width int
+Prompt string
+Focus() tea.Cmd
+Blur()
+View() string
+Value() string
}
class spinnerModel {
+New() spinner.Model
+Tick tea.Cmd
}
model *-- textinputModel : uses interval,duration,output,fps
model *-- spinnerModel : uses spinner
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The input handling relies heavily on hard-coded indices (e.g.,
inputs[2]as FPS,inputs[3]as output, andfocusIndex == 4for the button); consider introducing named constants or small helper methods for these positions to make future changes less error-prone. intervalValueis stored on the model but not used anywhere in the updated code; if it's not needed for the recorded time calculation or other logic yet, removing it would simplify the model and reduce confusion.- FPS is parsed as a float and passed to
timelapse.pywith two decimal places even though the UI suggests integer values (e.g., 30 FPS); you may want to either restrict this to integers or clarify and consistently treat FPS as a float across the UI and CLI call.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The input handling relies heavily on hard-coded indices (e.g., `inputs[2]` as FPS, `inputs[3]` as output, and `focusIndex == 4` for the button); consider introducing named constants or small helper methods for these positions to make future changes less error-prone.
- `intervalValue` is stored on the model but not used anywhere in the updated code; if it's not needed for the recorded time calculation or other logic yet, removing it would simplify the model and reduce confusion.
- FPS is parsed as a float and passed to `timelapse.py` with two decimal places even though the UI suggests integer values (e.g., 30 FPS); you may want to either restrict this to integers or clarify and consistently treat FPS as a float across the UI and CLI call.
## Individual Comments
### Comment 1
<location> `main.go:51-54` </location>
<code_context>
interval textinput.Model
duration textinput.Model
output textinput.Model
+ fps textinput.Model
focusIndex int
inputs []textinput.Model
</code_context>
<issue_to_address>
**suggestion:** The hardcoded indices for inputs (0,1,2,3,4) make the menu brittle to future changes.
Multiple places now assume specific positions in `inputs` (e.g. FPS at 2, output at 3, button at 4 via `m.inputs[3].Value()`, `if m.focusIndex == 4`, etc.). This will be fragile if the number or order of inputs changes.
Defining named index constants (e.g. `const idxInterval = iota; idxDuration; idxFPS; idxOutput; idxButton`) and deriving the button index from `len(m.inputs)` where possible would keep the logic consistent as the slice evolves.
Suggested implementation:
```golang
// buttonIndex returns the index representing the submit/confirm button.
// It is derived from the current number of inputs to avoid hardcoding.
func (m model) buttonIndex() int {
return len(m.inputs)
}
// Styles
func initialModel() model {
```
```golang
m.inputs[idxInterval]
```
```golang
m.inputs[idxDuration]
```
```golang
m.inputs[idxFPS]
```
```golang
m.inputs[idxOutput]
```
```golang
if m.focusIndex == m.buttonIndex() {
```
```golang
m.focusIndex = m.buttonIndex()
```
You should also:
1. Search the rest of `main.go` for any remaining hardcoded uses of indices:
- `m.focusIndex == 0`, `== 1`, `== 2`, `== 3` and replace them with `idxInterval`, `idxDuration`, `idxFPS`, `idxOutput` respectively.
- Any `m.focusIndex == 4`, `m.focusIndex = 4`, or comparisons like `> 4`, `< 4` should be revised to use `m.buttonIndex()` or `len(m.inputs)` as appropriate.
- Any other occurrences of `m.inputs[0]`, `m.inputs[1]`, `m.inputs[2]`, `m.inputs[3]` that are not caught by the simple patterns above (e.g. inside composite expressions) should be updated manually to use `idxInterval`, `idxDuration`, `idxFPS`, and `idxOutput`.
2. If you ever add more inputs or change their order in the `inputs` slice, only the slice construction and the index constants (if the semantic mapping changes) should need to be updated; the button will remain correctly derived from `len(m.inputs)`.
</issue_to_address>
### Comment 2
<location> `main.go:324-327` </location>
<code_context>
+ if fps == "" {
+ fps = "30"
+ }
+ fpsFloat, err := strconv.ParseFloat(fps, 64)
+ if err != nil || fpsFloat <= 0 {
+ m.state = stateError
+ m.finalMessage = "Invalid FPS value (use a positive number)"
+ return m, nil
+ }
</code_context>
<issue_to_address>
**suggestion:** Using a float for FPS and formatting with `%.2f` may be unnecessarily permissive and could lead to surprising values.
Right now FPS is parsed as `float64` and formatted to two decimals before passing to the Python script, so inputs like `29.9999` end up as `30.00` without the user realizing. If FPS should be integral, parsing as `int` (e.g. `strconv.Atoi`) would clarify intent and avoid rounding. If fractional FPS is expected, consider either preserving the original validated string when building the command or tightening validation to constrain allowed decimal places so the rounding behavior is explicit and predictable.
Suggested implementation:
```golang
if fps == "" {
fps = "30"
}
fpsInt, err := strconv.Atoi(fps)
if err != nil || fpsInt <= 0 {
m.state = stateError
m.finalMessage = "Invalid FPS value (use a positive integer)"
return m, nil
}
```
1. Anywhere `fpsFloat` is used later in this file (e.g. when building the Python command), update the code to use either:
* `fpsInt` (and format it with `%d`), or
* the validated `fps` string directly without `"%.2f"` formatting.
2. Remove any `"%.2f"` formatting of the FPS value to avoid hidden rounding and keep the behavior aligned with the integer validation.
3. If there are function signatures or struct fields that currently use `float64` for FPS, consider changing them to `int` to keep the type consistent across the codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
54
| fps textinput.Model | ||
| focusIndex int | ||
| inputs []textinput.Model | ||
| spinner spinner.Model |
There was a problem hiding this comment.
suggestion: The hardcoded indices for inputs (0,1,2,3,4) make the menu brittle to future changes.
Multiple places now assume specific positions in inputs (e.g. FPS at 2, output at 3, button at 4 via m.inputs[3].Value(), if m.focusIndex == 4, etc.). This will be fragile if the number or order of inputs changes.
Defining named index constants (e.g. const idxInterval = iota; idxDuration; idxFPS; idxOutput; idxButton) and deriving the button index from len(m.inputs) where possible would keep the logic consistent as the slice evolves.
Suggested implementation:
// buttonIndex returns the index representing the submit/confirm button.
// It is derived from the current number of inputs to avoid hardcoding.
func (m model) buttonIndex() int {
return len(m.inputs)
}
// Styles
func initialModel() model {m.inputs[idxInterval]m.inputs[idxDuration]m.inputs[idxFPS]m.inputs[idxOutput]if m.focusIndex == m.buttonIndex() {m.focusIndex = m.buttonIndex()You should also:
-
Search the rest of
main.gofor any remaining hardcoded uses of indices:m.focusIndex == 0,== 1,== 2,== 3and replace them withidxInterval,idxDuration,idxFPS,idxOutputrespectively.- Any
m.focusIndex == 4,m.focusIndex = 4, or comparisons like> 4,< 4should be revised to usem.buttonIndex()orlen(m.inputs)as appropriate. - Any other occurrences of
m.inputs[0],m.inputs[1],m.inputs[2],m.inputs[3]that are not caught by the simple patterns above (e.g. inside composite expressions) should be updated manually to useidxInterval,idxDuration,idxFPS, andidxOutput.
-
If you ever add more inputs or change their order in the
inputsslice, only the slice construction and the index constants (if the semantic mapping changes) should need to be updated; the button will remain correctly derived fromlen(m.inputs).
Comment on lines
+324
to
+327
| fpsFloat, err := strconv.ParseFloat(fps, 64) | ||
| if err != nil || fpsFloat <= 0 { | ||
| m.state = stateError | ||
| m.finalMessage = "Invalid FPS value (use a positive number)" |
There was a problem hiding this comment.
suggestion: Using a float for FPS and formatting with %.2f may be unnecessarily permissive and could lead to surprising values.
Right now FPS is parsed as float64 and formatted to two decimals before passing to the Python script, so inputs like 29.9999 end up as 30.00 without the user realizing. If FPS should be integral, parsing as int (e.g. strconv.Atoi) would clarify intent and avoid rounding. If fractional FPS is expected, consider either preserving the original validated string when building the command or tightening validation to constrain allowed decimal places so the rounding behavior is explicit and predictable.
Suggested implementation:
if fps == "" {
fps = "30"
}
fpsInt, err := strconv.Atoi(fps)
if err != nil || fpsInt <= 0 {
m.state = stateError
m.finalMessage = "Invalid FPS value (use a positive integer)"
return m, nil
}- Anywhere
fpsFloatis used later in this file (e.g. when building the Python command), update the code to use either:fpsInt(and format it with%d), or- the validated
fpsstring directly without"%.2f"formatting.
- Remove any
"%.2f"formatting of the FPS value to avoid hidden rounding and keep the behavior aligned with the integer validation. - If there are function signatures or struct fields that currently use
float64for FPS, consider changing them tointto keep the type consistent across the codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Statement:
Users lacked direct feedback on the estimated duration of their final timelapse video during recording, making it difficult to
plan and assess progress, especially with varying capture intervals and output FPS.
Solution:
This PR introduces a "Recorded Time" indicator to the live progress output in the TUI, which estimates the final video
duration based on the number of frames captured and the specified output FPS. To facilitate this, a new input field for
"Output FPS" has been added to the main menu.
Key Changes:
main.go:video's frames per second.
frames_captured / output_fps.
timelapse.py:How to Test:
number of captured frames and the FPS value you entered.
ffprobe or a media player).
Use Cases Addressed:
Summary by Sourcery
Add an output FPS configuration to the TUI and surface an estimated final video duration during recording.
New Features:
Enhancements:
Fixes: [Feature]: Display the recorded video time in TUI #3