From 4c078d6505076d377caadb56bc2ea9ecf382a234 Mon Sep 17 00:00:00 2001 From: Daisuke Maki Date: Mon, 8 Sep 2014 14:41:06 +0900 Subject: [PATCH] Fix a race condition that existed between clear status and print status For whatever reason I was thinking at the time, I had split the channels to receive request to print to and clear the status message. This basically means that, even if you meant to print X and then clear it in 500 milliseconds, the clear request could actually arrive BEFORE the print message. This change basically puts all the status related request into one channel, so that all requests can come sequentially --- hub.go | 13 +++---------- hub_test.go | 13 +++---------- keymap.go | 3 +-- layout.go | 32 ++++++++++++++++++++------------ layout_test.go | 8 +------- view.go | 21 +++++++++++---------- 6 files changed, 39 insertions(+), 51 deletions(-) diff --git a/hub.go b/hub.go index d26e24b..0c091dc 100644 --- a/hub.go +++ b/hub.go @@ -141,18 +141,11 @@ func (h *Hub) StatusMsgCh() chan HubReq { // SendStatusMsg sends a string to be displayed in the status message func (h *Hub) SendStatusMsg(q string) { - send(h.StatusMsgCh(), HubReq{q, nil}, h.isSync) + h.SendStatusMsgAndClear(q, 0) } -func (h *Hub) ClearStatusCh() chan HubReq { - return h.clearStatusCh -} - -// SendClearStatus sends a request to clear the status message in -// `d` duration. If a new status message is sent before the clear -// request is executed, the clear instruction will be canceled -func (h *Hub) SendClearStatus(d time.Duration) { - send(h.ClearStatusCh(), HubReq{d, nil}, h.isSync) +func (h *Hub) SendStatusMsgAndClear(q string, clearDelay time.Duration) { + send(h.StatusMsgCh(), HubReq{StatusMsgRequest{q, clearDelay}, nil}, h.isSync) } // PagingCh returns the channel to page through the results diff --git a/hub_test.go b/hub_test.go index 2d1a6d1..7cf6593 100644 --- a/hub_test.go +++ b/hub_test.go @@ -27,19 +27,14 @@ func TestHub(t *testing.T) { }() go func() { hr := <-h.StatusMsgCh() - if hr.DataString() != "Hello, World!" { - t.Errorf("Expected data to be 'Hello World!', got '%s'", hr.DataString()) + r := hr.DataInterface().(StatusMsgRequest) + if r.message != "Hello, World!" { + t.Errorf("Expected data to be 'Hello World!', got '%s'", r.message) } time.Sleep(100 * time.Millisecond) done["status"] = time.Now() hr.Done() }() - go func() { - hr := <-h.ClearStatusCh() - time.Sleep(100 * time.Millisecond) - done["clearStatus"] = time.Now() - hr.Done() - }() go func() { hr := <-h.PagingCh() time.Sleep(100 * time.Millisecond) @@ -51,7 +46,6 @@ func TestHub(t *testing.T) { h.SendQuery("Hello World!") h.SendDraw(nil) h.SendStatusMsg("Hello, World!") - h.SendClearStatus(time.Second) h.SendPaging(ToLineAbove) }) @@ -59,7 +53,6 @@ func TestHub(t *testing.T) { "query", "draw", "status", - "clearStatus", "paging", } diff --git a/keymap.go b/keymap.go index 6f69ea0..0681df0 100644 --- a/keymap.go +++ b/keymap.go @@ -63,11 +63,10 @@ func wrapClearSequence(a Action) Action { } if len(i.currentKeySeq) > 0 { - i.SendStatusMsg(strings.Join(i.currentKeySeq, " ")) + i.SendStatusMsgAndClear(strings.Join(i.currentKeySeq, " "), 500 * time.Millisecond) i.currentKeySeq = []string{} } - i.SendClearStatus(500 * time.Millisecond) a.Execute(i, ev) }) } diff --git a/layout.go b/layout.go index d3060f6..b2cc102 100644 --- a/layout.go +++ b/layout.go @@ -2,6 +2,7 @@ package peco import ( "fmt" + "sync" "time" "unicode/utf8" @@ -42,8 +43,7 @@ func IsValidVerticalAnchor(anchor VerticalAnchor) bool { // Layout represents the component that controls where elements are placed on screen type Layout interface { - ClearStatus(time.Duration) - PrintStatus(string) + PrintStatus(string, time.Duration) DrawPrompt() DrawScreen([]Match) MovePage(PagingRequest) @@ -188,6 +188,7 @@ type StatusBar struct { *Ctx *AnchorSettings clearTimer *time.Timer + timerMutex *sync.Mutex } // NewStatusBar creates a new StatusBar struct @@ -196,29 +197,26 @@ func NewStatusBar(ctx *Ctx, anchor VerticalAnchor, anchorOffset int) *StatusBar ctx, NewAnchorSettings(anchor, anchorOffset), nil, + &sync.Mutex{}, } } func (s *StatusBar) stopTimer() { + s.timerMutex.Lock() + defer s.timerMutex.Unlock() if t := s.clearTimer; t != nil { t.Stop() + s.clearTimer = nil } } -// ClearStatus clears the string displayed in the status bar area -// after `d time.Duration`. -func (s *StatusBar) ClearStatus(d time.Duration) { - s.stopTimer() - s.clearTimer = time.AfterFunc(d, func() { - s.PrintStatus("") - }) -} - // PrintStatus prints a new status message. This also resets the // timer created by ClearStatus() -func (s *StatusBar) PrintStatus(msg string) { +func (s *StatusBar) PrintStatus(msg string, clearDelay time.Duration) { s.stopTimer() + s.timerMutex.Lock() + location := s.AnchorPosition() w, _ := screen.Size() @@ -248,6 +246,16 @@ func (s *StatusBar) PrintStatus(msg string) { printScreen(w-width, location, fgAttr|termbox.AttrReverse|termbox.AttrBold, bgAttr|termbox.AttrReverse, msg, false) } screen.Flush() + + s.timerMutex.Unlock() + + // if everything is successful AND the clearDelay timer is specified, + // then set a timer to clear the status + if clearDelay != 0 { + s.clearTimer = time.AfterFunc(clearDelay, func() { + s.PrintStatus("", 0) + }) + } } // ListArea represents the area where the actual line buffer is diff --git a/layout_test.go b/layout_test.go index 2f22ceb..e400813 100644 --- a/layout_test.go +++ b/layout_test.go @@ -2,7 +2,6 @@ package peco import ( "testing" - "time" "unicode/utf8" "github.com/mattn/go-runewidth" @@ -111,16 +110,11 @@ func TestStatusBar(t *testing.T) { defer guard() st := NewStatusBar(NewCtx(nil), AnchorBottom, 0) - st.PrintStatus("Hello, World!") + st.PrintStatus("Hello, World!", 0) events := i.events if l := len(events["Flush"]); l != 1 { t.Errorf("Expected 1 Flush event, got %d", l) return } - - // XXX This is not a test... :/ - // need to think of a way to verify that we're actually doing something - st.ClearStatus(500 * time.Millisecond) - <-time.After(time.Second) } diff --git a/view.go b/view.go index e133812..28afaf6 100644 --- a/view.go +++ b/view.go @@ -22,6 +22,14 @@ const ( ToScrollPageUp ) +// StatusMsgRequest specifies the string to be drawn +// on the status message bar and an optional delay that tells +// the view to clear that message +type StatusMsgRequest struct { + message string + clearDelay time.Duration +} + // Loop receives requests to update the screen func (v *View) Loop() { defer v.ReleaseWaitGroup() @@ -30,10 +38,7 @@ func (v *View) Loop() { case <-v.LoopCh(): return case m := <-v.StatusMsgCh(): - v.printStatus(m.DataString()) - m.Done() - case m := <-v.ClearStatusCh(): - v.clearStatus(m.DataInterface().(time.Duration)) + v.printStatus(m.DataInterface().(StatusMsgRequest)) m.Done() case r := <-v.PagingCh(): v.movePage(r.DataInterface().(PagingRequest)) @@ -54,12 +59,8 @@ func (v *View) Loop() { } } -func (v *View) printStatus(m string) { - v.layout.PrintStatus(m) -} - -func (v *View) clearStatus(d time.Duration) { - v.layout.ClearStatus(d) +func (v *View) printStatus(r StatusMsgRequest) { + v.layout.PrintStatus(r.message, r.clearDelay) } func (v *View) drawScreenNoLock(targets []Match) {