From 049c72393a26806f2e7408075a48849467262c00 Mon Sep 17 00:00:00 2001 From: Tim Culverhouse Date: Thu, 6 Oct 2022 11:46:44 -0500 Subject: [PATCH] invalidatable: always mark ui as dirty OnInvalidate The Invalidatable struct is designed so that a widget can have a callback function ran when it is Invalidated. This is used to cascade up the widget tree, marking things as Invalid along the way so that only Invalid widgets are drawn. However, this is only implemented at the grid cell level for checks if the cell is invalidated -- and the grid cells are never set back to a "valid" state. The effect of this is that no matter what is invalidated, the entire UI gets drawn again. The calling through the Invalidate callbacks creates *several* race conditions, as Invalidate is called from several different goroutines, and many widgets call invalidate on their parent or children. Tcell has optimizations to only rerender screen cells that have changed their rune and style. The only performance penalty by redrawing the entire screen for aerc is the operations *within the aerc draw methods*. Most of these are not expensive and have relatively no impact on performance. Skip all of the OnInvalidates, and directly invalidate the UI when DoInvalidate is called by a widget. This reduces data races, and simplifies the widget redraw logic signficantly. Signed-off-by: Tim Culverhouse Acked-by: Robin Jarry --- CHANGELOG.md | 1 + lib/ui/invalidatable.go | 9 +-------- lib/ui/ui.go | 26 +++++++++++++++++++------- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0739dcd..f3ab262 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). `accounts.conf`. - Mouse support for embedded editors when `mouse-enabled=true`. - Numerous race conditions related to event handling order +- Numerous race conditions related to OnInvalidate calls ## [0.12.0](https://git.sr.ht/~rjarry/aerc/refs/0.12.0) - 2022-09-01 diff --git a/lib/ui/invalidatable.go b/lib/ui/invalidatable.go index 9275712..fc354bd 100644 --- a/lib/ui/invalidatable.go +++ b/lib/ui/invalidatable.go @@ -13,12 +13,5 @@ func (i *Invalidatable) OnInvalidate(f func(d Drawable)) { } func (i *Invalidatable) DoInvalidate(d Drawable) { - v := i.onInvalidate.Load() - if v == nil { - return - } - f := v.(func(d Drawable)) - if f != nil { - f(d) - } + atomic.StoreInt32(&dirty, DIRTY) } diff --git a/lib/ui/ui.go b/lib/ui/ui.go index 42d584f..073684c 100644 --- a/lib/ui/ui.go +++ b/lib/ui/ui.go @@ -6,6 +6,11 @@ import ( "github.com/gdamore/tcell/v2" ) +const ( + DIRTY int32 = iota + NOT_DIRTY +) + var MsgChannel = make(chan AercMsg, 50) // QueueRedraw sends a nil message into the MsgChannel. Nothing will handle this @@ -14,13 +19,23 @@ func QueueRedraw() { MsgChannel <- nil } +// dirty is the dirty state of the UI. Any value other than 0 means the UI is in +// a dirty state. Dirty should only be accessed via atomic operations to +// maintain thread safety +var dirty int32 + +// Invalidate marks the entire UI as invalid. Invalidate can be called from any +// goroutine +func Invalidate() { + atomic.StoreInt32(&dirty, DIRTY) +} + type UI struct { Content DrawableInteractive exit atomic.Value // bool ctx *Context screen tcell.Screen popover *Popover - invalid int32 // access via atomic } func Initialize(content DrawableInteractive) (*UI, error) { @@ -47,10 +62,7 @@ func Initialize(content DrawableInteractive) (*UI, error) { state.exit.Store(false) - state.invalid = 1 - content.OnInvalidate(func(_ Drawable) { - atomic.StoreInt32(&state.invalid, 1) - }) + Invalidate() if beeper, ok := content.(DrawableInteractiveBeeper); ok { beeper.OnBeep(screen.Beep) } @@ -80,8 +92,8 @@ func (state *UI) Close() { } func (state *UI) Render() { - wasInvalid := atomic.SwapInt32(&state.invalid, 0) - if wasInvalid != 0 { + dirtyState := atomic.SwapInt32(&dirty, NOT_DIRTY) + if dirtyState == DIRTY { // reset popover for the next Draw state.popover = nil state.Content.Draw(state.ctx)