Multiple issues in chapter 7 of the Egg timer tutorial

[hchargois]

Hi! First, let me say that I really like the tutorial so far, and I've not yet completed it but I absolutely intend to. Thank you for your work in making gio so accessible.

Even though I don't know a lot of gio yet, I do know Go sufficiently well to notice a few issues in chapter 7 (implementation of the progress bar). Some of these issues are very subjective and debatable, others much less so. I'll list them in increasing order of criticality.

Keeping state of the progress instead of the boil duration

In my opinion, state should be as close to business logic as possible. For an egg timer, the business logic is to keep track of elapsed time. The fact that you need a progress float in the 0-1 range to display a progress bar is just that, a display necessity. Therefore, the state variable that should be tracked is the elapsed boil time, from 0 to the total duration (which should be defined somewhere as a const for example). The progress float should only be scoped to the progress bar widget and be recomputed from the ground/business truth (the boil duration) every time we need to draw the progress bar.

Using global variables

Global variables are generally frowned upon, especially if there are easy ways to avoid them. Here, progress is only used in draw(), so it could just as well be a variable of that function. progressIncrementer should not exist (see next point) but if it did, it could be passed as an argument of the draw() function from the main.

Using a channel for no good reason

You are using a progressIncrementer channel which is produced to in a goroutine, and consumed in another:

// in main()...
progressIncrementer = make(chan float32)
go func() {
	for {
		time.Sleep(time.Second / 25)
		progressIncrementer <- 0.004
	}
}()

// in draw()...
go func() {
	for p := range progressIncrementer {
		if boiling && progress < 1 {
			progress += p
			// Force a redraw by invalidating the frame
			w.Invalidate()
		}
	}
}()

This achieves absolutely nothing more than what could be done in a single, much simpler goroutine, without any channel whatsoever:

// in draw()...
go func() {
	for {
		time.Sleep(time.Second / 25)
		if boiling && progress < 1 {
			progress += 0.004
			w.Invalidate()
		}
	}
}()

To reiterate, that channel's only job is to make the second goroutine wait on a Sleep... But that's already what Sleep does by itself. The channel doesn't add any useful feature to that, except maybe that it passes the increment value but since it's a literal constant I don't see a good reason to define it somewhere else than where it's needed.

When I first read chapter 7, I thought maybe this was some kind of over-engineering that was unnecessary but that could be some useful generalization, but I don't see it. It just seems to me like something overly complex for no good reason. I also fear this may convey, especially to novice Go developers, a false understanding about what a channel is useful for or when it just doesn't help. Speaking of which...

Race condition

That's the big one. Race conditions are not OK in Go and good Go code should not have any.

programmers should write Go programs without data races
[...]
insisting that races are errors
https://go.dev/ref/mem

By reading and writing to both progress and boiling in two distinct goroutines (the goroutine running the bulk of draw() and the anonymous one started inside draw()), you are creating a race condition.

Seeing that plain (bool and float32) variables are insecurely accessed and modified concurrently from multiple goroutines is enough to characterize these race conditions, but helpfully they are also reliably detected by Go's builtin race detector (at least on my machine), which can be activated by building the program with go build -race and running it:

==================
WARNING: DATA RACE
Write at 0x00c0002144bf by goroutine 8:
  main.draw()
      /home/gohu/tmp/gio/main.go:80 +0x471
  main.main.func2()
      /home/gohu/tmp/gio/main.go:35 +0x1bb

Previous read at 0x00c0002144bf by goroutine 9:
  main.draw.func1()
      /home/gohu/tmp/gio/main.go:63 +0x7d

Goroutine 8 (running) created at:
  main.main()
      /home/gohu/tmp/gio/main.go:30 +0x67

Goroutine 9 (running) created at:
  main.draw()
      /home/gohu/tmp/gio/main.go:61 +0x1ea
  main.main.func2()
      /home/gohu/tmp/gio/main.go:35 +0x1bb
==================
==================
WARNING: DATA RACE
Write at 0x000001cbc7d4 by goroutine 9:
  main.draw.func1()
      /home/gohu/tmp/gio/main.go:64 +0xdb

Previous read at 0x000001cbc7d4 by goroutine 8:
  main.draw.func2()
      /home/gohu/tmp/gio/main.go:91 +0x3e
  gioui.org/layout.Flex.Layout()
      /home/gohu/go/pkg/mod/[email protected]/layout/flex.go:99 +0x31b
  main.draw()
      /home/gohu/tmp/gio/main.go:88 +0x796
  main.main.func2()
      /home/gohu/tmp/gio/main.go:35 +0x1bb

Goroutine 9 (running) created at:
  main.draw()
      /home/gohu/tmp/gio/main.go:61 +0x1ea
  main.main.func2()
      /home/gohu/tmp/gio/main.go:35 +0x1bb

Goroutine 8 (running) created at:
  main.main()
      /home/gohu/tmp/gio/main.go:30 +0x67
==================
Found 2 data race(s)

I guess the easiest way to get rid of these races would be to add a big Mutex:

var mu sync.Mutex
// listen for events in the incrementer channel
go func() {
	for p := range progressIncrementer {
		mu.Lock()
		// ...
		mu.Unlock()
	}
}()

for {
	// listen for events in the window
	switch e := w.Event().(type) {

	// this is sent when the application should re-render
	case app.FrameEvent:
		mu.Lock()
		// ...
		mu.Unlock()

	// this is sent when the application is closed
	case app.DestroyEvent:
		return e.Err
	}
}

That should work.

However, I wonder if it's not just better to bite the bullet and do things the correct (I guess?) way. Having read this essay https://eliasnaur.com/blog/immediate-mode-gui-programming before starting the egg timer tutorial, I have the impression that the bouncing ball and the egg timer are very similar things, and I think the egg timer should work similarly than the bouncing ball. That is, it should use gtx.Now to keep track of time and increment the elapsed boiling duration on each frame.

Here's what I came up with:

const targetBoilingDuration = 3 * time.Second

func draw(w *app.Window) error {
	var ops op.Ops

	var startButton widget.Clickable

	var boiling bool
	var boilingElapsed time.Duration
	var prevTime time.Time

	th := material.NewTheme()

	for {
		switch e := w.Event().(type) {
		case app.FrameEvent:
			gtx := app.NewContext(&ops, e)

			if boiling {
				boilingElapsed += gtx.Now.Sub(prevTime)
				if boilingElapsed >= targetBoilingDuration {
					boilingElapsed = targetBoilingDuration
					boiling = false
				}
				gtx.Execute(op.InvalidateCmd{})
			}

			if startButton.Clicked(gtx) {
				boiling = !boiling
				if boiling && boilingElapsed == targetBoilingDuration {
					// Restart the timer after it has completed
					boilingElapsed = 0
				}
			}
			prevTime = gtx.Now

			layout.Flex{
				Axis:    layout.Vertical,
				Spacing: layout.SpaceStart,
			}.Layout(gtx,
				layout.Rigid(func(gtx C) D {
					progress := float32(boilingElapsed) / float32(targetBoilingDuration)
					pb := material.ProgressBar(th, progress)
					return pb.Layout(gtx)
				}),
				layout.Rigid(
					func(gtx C) D {
						return layout.Inset{
							Top:    unit.Dp(25),
							Bottom: unit.Dp(25),
							Right:  unit.Dp(35),
							Left:   unit.Dp(35),
						}.Layout(gtx,
							func(gtx C) D {
								var text string
								if !boiling {
									text = "Start"
								} else {
									text = "Stop"
								}
								btn := material.Button(th, &startButton, text)
								return btn.Layout(gtx)
							},
						)
					},
				),
			)
			e.Frame(gtx.Ops)

		case app.DestroyEvent:
			return e.Err
		}
	}
}

I'm sure it could be improved by splitting the business logic in a function, maybe in a method of a state struct as in the bouncing ball example.

Also, it would be much simpler if we didn't support the pausing of the timer. We could then just have a single startTime state variable indicated when the timer was last started, and the elapsed time would simply be the difference with now; no need to accumulate a duration in a variable.

Sorry for the long issue post, and feel free to disregard any or all of the points I mentioned. Thanks again for the tutorial.

[jonegil]

I love this, thank you for the thorough commentary and kind wording.

Would you be interested in having a go at recactoring the code and writeup?

If anything can be improved, let's improve :-)

[hchargois]

Thanks, that's very nice of you to ask! Unfortunately, learning gio was already a "side-quest" for me and I can't dedicate too much time into it. I know how much work goes into writing a good tutorial and I thank you for that!

I did spend some time refining a new implementation of the chapter 7 code, here it is on my fork:
3fc5568

You can notice a few things.

First, there is now a Timer object that holds the business logic of the timer. This better decouples the business logic and the UI. In production code, you would probably want to unit test the Timer without having it tied to the UI. The time of the timer must be explicitly advanced by calling its dedicated method, this makes it both easier to integrate with gio and to unit test if needed.

Second, we use gtx.Now to find out how much time has passed since the last frame; and InvalidateCmd to continually request new frames when the timer is running, as in my previous post.

Finally, I've split the single button state into three distinct button states. I think it's much cleaner that way. The fact that a single button is displayed at any one time does not mean that there is conceptually a single button (i.e. action), it's just a design choice to only show one of them. This may seem like some pedantic distinction, but it has 2 very concrete consequences:

• first, it's easier to react to the button clicks: if the "start" button is clicked, just call timer.Start(), etc. No need to check the current state of the timer again to decide what the click meant.
• second, this avoids a strange behavior that happens if you click the button "in between" states. For example, if you press on the "stop" button (while the timer is running) but don't release until after the timer has finished, it will register as if you clicked on the "reset" button. It's probably not what you want. By using distinct widget.Clickables, the clicks are only registered if the mouse-down and the mouse-up happen on the same button.

[jonegil]

Super, I'll take a look.

Depending on code I might or might not merge, but will nonetheless link to our conversation here.

My goal is to keep things simple and will err on the side of understandability, if that's even a word. Love your contrib and get back if you see more.

Cheers