Closed bcicen closed 6 years ago
Looks fantastic
I want to play with it just a bit before merging. I assume merging this one and closing initial one will do
A question, how NewFromStream is used? I assume you already integrated in some app of yours? Or just a common-sense method?
Wanted to check how far it's from making search non blocking (at least cancellable), on top of this branch.
Found few issues so far:
q
in the middle - on master
q stays in termbox events loop and term exits once search is done, on this branch - it's kinda discarded and does nothing later on (since event loop is already break
-en) Looks like problem with interrupt termbox.Interrupt() within updateLastLine
Changing it back to following fixes both issues
it's always should be interrupt followed by call to actual action(which will block until processed)
Otherwise every next loop of line scan was blocked by 10ms select-sleep within new event loop(since termbox.Interrupt() was blocked until it is picked up)
go termbox.Interrupt()
select {
case requestStatusUpdate <- lastLine.Line:
f.fetcher.updateMap(dataLine)
case <-ctx.Done():
return
}
pushed followups to bcicen-fix-blocking-conditions
, it sounds like more general approach for handling events should be taken, i.e wrap termbox.PollEvent() (and?) interrupt, instead of processing multiple requests inside single interrupt event.
(i.e pre-fetch and process from own queue, with ability to flush all blocked on-interrupt go-routines or stop buffer.fill() context if some control sequences(ctrl+c?) received while processing previous event) I'd say it big enough to be outside of scope of this PR.
What was the problem you was fixing with this change? (i've basically reverted it in followups...). Maybe it can be solved separately, but differently
Opened https://github.com/tigrawap/slit/issues/53 to follow up on blocking issue with general idea for solving. I hope to find a time to work on it this week, if you will want to take over please LMK
See follow up branch(bcicen-fix-blocking-conditions
), repush it here if it's fine with you(in case i've missed something why this change was essential)
Merged manually, with comments above
This is a follow-up to (and depends on) https://github.com/tigrawap/slit/pull/51.