Simplify InterruptReader facade and specify it as Input interface for ansipixels, with better name than ReadDirect. Fix windows every other frame read delay in FPSTicks#203
Conversation
… ansipixels, with better name than ReadDirect
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
==========================================
- Coverage 18.06% 17.31% -0.76%
==========================================
Files 23 23
Lines 4129 4308 +179
==========================================
Hits 746 746
- Misses 3352 3531 +179
Partials 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the InterruptReader facade by renaming DirectRead to ReadWithTimeout and removing the now-duplicate ReadWithTimeout method that had wait logic. It also introduces an Input interface in ansipixels to abstract input reading capabilities, making the code more modular and allowing alternative implementations (e.g., for SSH-based input).
- Renamed
DirectReadtoReadWithTimeoutwith clearer documentation about its behavior - Removed duplicate
ReadWithTimeoutmethod that included buffering logic - Introduced the
Inputinterface inansipixelsto abstract input reading requirements - Updated
AnsiPixels.SharedInputfield type from*terminal.InterruptReadertoInput
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| interrupt.go | Renamed DirectRead to ReadWithTimeout, removed duplicate ReadWithTimeout method with wait logic, updated internal calls |
| ansipixels/ansipixels.go | Added Input interface, changed SharedInput field type to use interface, updated method calls and documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…FDs, the select based one is still used/selected
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
timeoutreader.go:24
- Fixed typo: "inpout" corrected to "input"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…er tags mode: split immediate read into 2 phases (#205)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
timeoutreader.go:23
- Spelling error: 'loosing' should be 'losing' (meaning 'to lose', not 'to make loose').
timeoutreader.go:24 - Fixed typo: 'inpout' corrected to 'input'.
timeoutreader.go:56 - The inputChan buffer size was changed from 1 to 0 (unbuffered). This makes the channel synchronous, which means PrimeReadImmediate will now block until the reader goroutine receives from the channel. This change should be called out in the PR description or documentation, as it affects the behavior and could introduce blocking where it didn't exist before, particularly if PrimeReadImmediate is called while the reader goroutine is blocked on a read.
timeoutreader.go:226 - The comment says "If called currently it will block until the current read completes" but the implementation only briefly locks the mutex to update the timeout value. It does not wait for any read operation to complete. The comment should be corrected to accurately reflect that the method only updates the timeout for subsequent reads and may block briefly to acquire the mutex, but does not wait for in-progress reads.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (tr *TimeoutReaderUnixFD) ReadImmediate() (int, error) { | ||
| if tr.blocking { | ||
| return tr.ostream.Read(buf) | ||
| return tr.ostream.Read(tr.buf) | ||
| } | ||
| var zeroTv unix.Timeval | ||
| return ReadWithTimeout(tr.fd, &zeroTv, buf) | ||
| return ReadWithTimeout(tr.fd, &zeroTv, tr.buf) | ||
| } |
There was a problem hiding this comment.
ReadImmediate doesn't check if PrimeReadImmediate was called first. If tr.buf is nil (because PrimeReadImmediate was never called), calling ReadWithTimeout with a nil buffer could cause undefined behavior or a crash. Consider adding a check similar to the Windows implementation:
if tr.buf == nil {
panic("ReadImmediate called without prior PrimeReadImmediate")
}
works with https://github.com/ldemailly/go-scratch/blob/main/sshdtui/sshd.go