Remove Internal Ticker | Simplify presentation

cmd/subcommands/acci-ping/acci-ping.go

@@ -27,7 +27,6 @@ type Config struct {
 	*application.SharedFlags
 	*tabflags.FlagSet
 
-	debugFps           *int
 	debuggingTermSize  *string
 	filePath           *string
 	followingOnStart   *bool
@@ -69,7 +68,6 @@ func GetFlags(info *application.BuildInfo) *Config {
 		debuggingTermSize: tf.String("debug-term-size", "", "switches the terminal to fixed mode and no iteractivity",
 			tabflags.AutoComplete{Choices: []string{"15x80", "20x85", "HxW"}}),
 		followingOnStart:   tf.Bool("follow", false, "if this flag is used the graph will be shown in following mode immediately"),
-		debugFps:           tf.Int("debug-fps", 240, "configures the internal tickrate for the graph re-paint look (in FPS)"),
 		logarithmicOnStart: tf.Bool("logarithmic", false, "if this flag is used the graph will be shown in logarithmic mode immediately"),
 	}
 	*ret.pingBufferingLimit = 10

cmd/subcommands/acci-ping/application.go

@@ -37,10 +37,8 @@ type Application struct {
 	g    *graph.Graph
 	term *terminal.Terminal
 
-	toUpdate *os.File
-	config   Config
-	// this doesn't need a mutex because we ensure that no two threads have access to the same byte index (I
-	// think this is fine when the slice doesn't grow).
+	toUpdate   *os.File
+	config     Config
 	drawBuffer *draw.Buffer
 
 	errorChannel      chan error
@@ -108,7 +106,7 @@ func (app *Application) Run(
 	defer close(guiControlChannel)
 	defer close(guiSpeedChange)
 	// Very high FPS is good for responsiveness in the UI (since it's locked) and re-drawing on a re-size.
-	graph, cleanup, terminalSizeUpdates, err := app.g.Run(ctx, cancelFunc, *app.config.debugFps, app.listeners(), app.fallbacks)
+	graph, cleanup, terminalSizeUpdates, err := app.g.Run(ctx, cancelFunc, app.listeners(), app.fallbacks)
 	termRecover := func() {
 		_ = app.term.ClearScreen(terminal.UpdateSize)
 		cleanup()

cmd/tab_completion/tab_completion_test.go

@@ -9,7 +9,9 @@ package tabcompletion
 
 import (
 	"flag"
+	"fmt"
 	"log"
+	"log/slog"
 	"os"
 	"path/filepath"
 	"slices"
@@ -37,14 +39,7 @@ func TestGetChoices(t *testing.T) {
 		actual, err := runGetChoices("acci-ping")
 		assert.NilError(t, err)
 
-		expectedFlags := []string{}
-		accipingFlags.Fs.VisitAll(func(f *flag.Flag) {
-			if strings.HasPrefix(f.Name, "debug") {
-				return
-			}
-			expectedFlags = append(expectedFlags, "-"+f.Name)
-		})
-
+		expectedFlags := acciPingNonDebugFlags()
 		assertEqual(t, actual, slices.Concat([]string{"drawframe", "version"}, expectedFlags))
 	})
 	t.Run("start drawframe", func(t *testing.T) {
@@ -70,27 +65,15 @@ func TestGetChoices(t *testing.T) {
 		actual, err := runGetChoices("acci-ping", "-")
 		assert.NilError(t, err)
 
-		expectedFlags := []string{}
-		accipingFlags.Fs.VisitAll(func(f *flag.Flag) {
-			if strings.HasPrefix(f.Name, "debug") {
-				return
-			}
-			expectedFlags = append(expectedFlags, "-"+f.Name)
-		})
+		expectedFlags := acciPingNonDebugFlags()
 		assertEqual(t, actual, expectedFlags)
 	})
-	t.Run("start drawframe ", func(t *testing.T) {
+	t.Run("start drawframe tab", func(t *testing.T) {
 		t.Parallel()
 		actual, err := runGetChoices("acci-ping", "drawframe", "")
 		assert.NilError(t, err)
 
-		expectedFlags := []string{}
-		drawframe.GetFlags(nil).VisitAll(func(f *flag.Flag) {
-			if strings.HasPrefix(f.Name, "debug") {
-				return
-			}
-			expectedFlags = append(expectedFlags, "-"+f.Name)
-		})
+		expectedFlags := drawframeNonDebugFlags()
 		expectedFlags = slices.Concat(expectedFlags, filesByExt(".go"))
 		assertEqual(t, actual, expectedFlags)
 	})
@@ -112,16 +95,17 @@ func TestGetChoices(t *testing.T) {
 		t.Parallel()
 		starting := []string{}
 		accipingFlags.Fs.VisitAll(func(f *flag.Flag) {
-			if strings.HasPrefix(f.Name, "debug") {
+			if strings.HasPrefix(f.Name, "debug") || strings.HasPrefix(f.Name, "file") || strings.HasPrefix(f.Name, "theme") {
 				return
 			}
 			starting = append(starting, "-"+f.Name)
 		})
 		start := sliceutils.TakeRandom(starting)
-		expectedFlags := sliceutils.Remove(starting, start)
+		expectedFlags := sliceutils.Remove(acciPingNonDebugFlags(), start)
 
 		actual, err := runGetChoices("acci-ping", start, "")
 		assert.NilError(t, err)
+		slog.Info(fmt.Sprintf("random selection %q %q", start, starting))
 		assertEqual(t, actual, expectedFlags)
 	})
 	t.Run("start -file", func(t *testing.T) {
@@ -204,7 +188,6 @@ func make_acciping_Flags() Command {
 	_ = tf.String("file", "", "skipped for test",
 		tabflags.AutoComplete{WantsFile: true, FileExt: ".go"})
 	_ = tf.Bool("hide-help", false, "skipped for test")
-	_ = tf.Float64("pings-per-minute", 60.0, "skipped for test")
 	_ = tf.Bool("debug-error-creator", false, "skipped for test")
 	_ = tf.String("url", "www.google.com", "skipped for test", tabflags.AutoComplete{})
 	_ = tf.String("theme", "", "skipped for test",
@@ -250,3 +233,25 @@ func assertEqual(t *testing.T, expected, actual []string) {
 	slices.Sort(expected)
 	assert.DeepEqual(t, actual, expected)
 }
+
+func acciPingNonDebugFlags() []string {
+	expectedFlags := []string{}
+	accipingFlags.Fs.VisitAll(func(f *flag.Flag) {
+		if strings.HasPrefix(f.Name, "debug") {
+			return
+		}
+		expectedFlags = append(expectedFlags, "-"+f.Name)
+	})
+	return expectedFlags
+}
+
+func drawframeNonDebugFlags() []string {
+	expectedFlags := []string{}
+	drawframe.GetFlags(nil).VisitAll(func(f *flag.Flag) {
+		if strings.HasPrefix(f.Name, "debug") {
+			return
+		}
+		expectedFlags = append(expectedFlags, "-"+f.Name)
+	})
+	return expectedFlags
+}

cmd/tab_completion/tabflags/tabflags.go

@@ -81,6 +81,7 @@ func (f *FlagSet) GetAutoCompleteFor(flagName string) *AutoComplete {
 func (f *FlagSet) GetNames(includeDebug bool, toSkip []string) []string {
 	f.syncFlagSet()
 	keys := maps.Keys(f.nameToAc)
+	// Add the dash first so that we remove items matching [toSkip]
 	withDash := iterutils.Map(keys, func(n string) string {
 		return "-" + n
 	})

draw/draw.go

@@ -23,9 +23,6 @@ type Buffer struct {
 }
 
 // TODO paint buffer should be application level and agnostic to the draw buffer itself.
-//
-// TODO this buffering has race conditions, data can be written to the paint buffer while it's being read for
-// painting. A full investigation is required to determine if this is actually a problem.
 func NewPaintBuffer() *Buffer {
 	return newBuffer(int(indexCount.Load()))
 }

graph/drawing.go

@@ -28,10 +28,9 @@ import (
 )
 
 type computeFrameConfig struct {
-	timeBetweenFrames time.Duration
-	followLatestSpan  bool
-	drawSpinner       bool
-	yAxisScale        YAxisScale
+	followLatestSpan bool
+	drawSpinner      bool
+	yAxisScale       YAxisScale
 }
 
 func (c computeFrameConfig) Match(cfg computeFrameConfig) bool {
@@ -41,14 +40,6 @@ func (c computeFrameConfig) Match(cfg computeFrameConfig) bool {
 
 const drawingDebug = false
 
-func getTimeBetweenFrames(fps int, pingsPerMinute ping.PingsPerMinute) time.Duration {
-	if fps == 0 {
-		return ping.PingsPerMinuteToDuration(pingsPerMinute)
-	} else {
-		return time.Duration(1000/fps) * time.Millisecond
-	}
-}
-
 var noFrame = func(w io.Writer) error { return nil }
 
 func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error {

graph/export_test.go

@@ -24,7 +24,7 @@ func (g *Graph) ComputeFrame() string {
 	painter := g.computeFrame(computeFrameConfig{
 		followLatestSpan: false,
 		drawSpinner:      false,
-		yAxisScale:       g.presentation.YAxisScale,
+		yAxisScale:       g.presentation.Get().YAxisScale,
 	})
 	err := painter(&b)
 	check.NoErr(err, "While painting frame to string buffer")

graph/graph.go

@@ -21,17 +21,19 @@ import (
 	"github.com/Lexer747/acci-ping/gui"
 	"github.com/Lexer747/acci-ping/ping"
 	"github.com/Lexer747/acci-ping/terminal"
+	"github.com/Lexer747/acci-ping/utils/atomic"
 	"github.com/Lexer747/acci-ping/utils/check"
 )
 
 type Graph struct {
-	ui             gui.GUI
-	Term           *terminal.Terminal
-	dataChannel    <-chan ping.PingResults
-	data           *graphdata.GraphData
-	frameMutex     *sync.Mutex
-	drawingBuffer  *draw.Buffer
-	presentation   *controlState
+	ui            gui.GUI
+	Term          *terminal.Terminal
+	dataChannel   <-chan ping.PingResults
+	data          *graphdata.GraphData
+	frameMutex    *sync.Mutex
+	drawingBuffer *draw.Buffer
+
+	presentation   atomic.Of[Presentation]
 	controlChannel <-chan Control
 	lastFrame      frame
 	initial        ping.PingsPerMinute
@@ -104,7 +106,7 @@ func NewGraph(ctx context.Context, cfg GraphConfiguration) *Graph {
 		ui:             cfg.Gui,
 		debugStrict:    cfg.DebugStrict,
 		controlChannel: cfg.ControlPlane,
-		presentation:   &controlState{Presentation: cfg.Presentation, m: &sync.Mutex{}},
+		presentation:   atomic.Init(cfg.Presentation),
 	}
 	if ctx != nil {
 		// A nil context is valid: It means that no new data is expected and the input channel isn't active
@@ -128,12 +130,9 @@ func NewGraph(ctx context.Context, cfg GraphConfiguration) *Graph {
 func (g *Graph) Run(
 	ctx context.Context,
 	stop context.CancelCauseFunc,
-	fps int, // this isn't really an FPS given how the GUI is setup and paint buffers, more like a max re-paint delay timer thing.
 	listeners []terminal.ConditionalListener,
 	fallbacks []terminal.Listener,
 ) (func() error, func(), <-chan terminal.Size, error) {
-	timeBetweenFrames := getTimeBetweenFrames(fps, g.initial)
-	frameRate := time.NewTicker(timeBetweenFrames)
 	cleanup, err := g.Term.StartRaw(ctx, stop, listeners, fallbacks)
 	if err != nil {
 		return nil, cleanup, nil, err
@@ -143,12 +142,14 @@ func (g *Graph) Run(
 		size := g.Term.GetSize()
 		defer close(terminalUpdates)
 		slog.Info("running acci-ping")
+		// The main loop of acci-ping, await on the context (will be fired by the terminal)
 		for {
 			select {
 			case <-ctx.Done():
 				return context.Cause(ctx)
-			case <-frameRate.C:
-				err = g.Term.UpdateSize()
+			default:
+				// The main body of acci-ping, get the terminal size and send on the channel if changed
+				err := g.Term.UpdateSize()
 				if err != nil {
 					return err
 				}
@@ -157,14 +158,17 @@ func (g *Graph) Run(
 					terminalUpdates <- size
 					size = g.Term.GetSize()
 				}
-				g.presentation.m.Lock()
+				// Lock the presentation layer (this ensures we don't get GUI tearing - as in one component
+				// has data from last frame and another component has data from this frame), compute a new
+				// frame. Note that the optimisations to do no drawing are all in this function.
+				p := g.presentation.Get()
 				toWrite := g.computeFrame(computeFrameConfig{
-					timeBetweenFrames: timeBetweenFrames,
-					followLatestSpan:  g.presentation.Following,
-					drawSpinner:       true,
-					yAxisScale:        g.presentation.YAxisScale,
+					followLatestSpan: p.Following,
+					drawSpinner:      true,
+					yAxisScale:       p.YAxisScale,
 				})
-				g.presentation.m.Unlock()
+				// Now that we have a frame to draw (may just be a spinner update), execute this function
+				// writing the painted frame to the terminal.
 				err = toWrite(g.Term)
 				if err != nil {
 					return err
@@ -188,13 +192,12 @@ func (g *Graph) OneFrame() error {
 	if err != nil {
 		return err
 	}
-	g.presentation.m.Lock()
+	p := g.presentation.Get()
 	toWrite := g.computeFrame(computeFrameConfig{
-		followLatestSpan: g.presentation.Following,
+		followLatestSpan: p.Following,
 		drawSpinner:      false,
-		yAxisScale:       g.presentation.YAxisScale,
+		yAxisScale:       p.YAxisScale,
 	})
-	g.presentation.m.Unlock()
 	return toWrite(g.Term)
 }
 
@@ -216,8 +219,6 @@ func (g *Graph) Summarise() string {
 }
 
 func (g *Graph) ClearForPerfTest() {
-	g.presentation.m.Lock()
-	defer g.presentation.m.Unlock()
 	g.lastFrame = frame{spinnerData: spinner{timestampLastDrawn: time.Now()}}
 	g.drawingBuffer = draw.NewPaintBuffer()
 }
@@ -249,22 +250,18 @@ func (g *Graph) handleControl(ctx context.Context) {
 			if !ok {
 				return
 			}
-			// Note we don't need the mutex while reading the values.
 			guiChanged := p.FollowLatestSpan.DidChange || p.YAxisScale.DidChange
-			if guiChanged {
-				// But we do need it while writing
-				g.presentation.m.Lock()
-			}
+			pr := Presentation{}
 			if p.FollowLatestSpan.DidChange {
-				g.presentation.Following = p.FollowLatestSpan.Value
+				pr.Following = p.FollowLatestSpan.Value
 				slog.Info("switching to:", "FollowLatestSpan", p.FollowLatestSpan.Value)
 			}
 			if p.YAxisScale.DidChange {
-				g.presentation.YAxisScale = p.YAxisScale.Value
+				pr.YAxisScale = p.YAxisScale.Value
 				slog.Info("switching to:", "YAxisScale", p.YAxisScale.Value)
 			}
 			if guiChanged {
-				g.presentation.m.Unlock()
+				g.presentation.Set(pr)
 			}
 		}
 	}
@@ -296,8 +293,3 @@ func (f frame) Match(s terminal.Size, cfg computeFrameConfig) bool {
 func (f frame) Size() terminal.Size {
 	return terminal.Size{Height: f.xAxis.size, Width: f.yAxis.size}
 }
-
-type controlState struct {
-	m *sync.Mutex
-	Presentation
-}

graph/graphdata/graphdata.go

@@ -8,7 +8,6 @@ package graphdata
 
 import (
 	"fmt"
-	"io"
 	"sync"
 	"time"
 
@@ -67,12 +66,6 @@ func (gd *GraphData) Summary() string {
 	return gd.data.Summary()
 }
 
-func (gd *GraphData) AsCompact(w io.Writer) error {
-	gd.m.Lock()
-	defer gd.m.Unlock()
-	return gd.data.AsCompact(w)
-}
-
 func (gd *GraphData) Lock() {
 	gd.m.Lock()
 }