Revert "Merge pull request #52 from thiagokokada/pass-ctx-as-parameter"

This reverts commit 45d4f4a, reversing
changes made to 4ad0142.

internal/core/core.go

@@ -5,15 +5,14 @@ import (
 	"fmt"
 	"log"
 	"log/slog"
-	"sync/atomic"
 	"time"
 
 	"github.com/thiagokokada/twenty-twenty-twenty/internal/ctxlog"
 	"github.com/thiagokokada/twenty-twenty-twenty/internal/notification"
 	"github.com/thiagokokada/twenty-twenty-twenty/internal/sound"
 )
 
-var loop atomic.Int64
+var loop int
 
 /*
 Create a new TwentyTwentyTwenty struct.
@@ -22,13 +21,21 @@ func New(features Features, settings Settings) *TwentyTwentyTwenty {
 	return &TwentyTwentyTwenty{Features: features, Settings: settings}
 }
 
+/*
+Returns the current running twenty-twenty-twenty's context.
+
+Can be used to register for context cancellation, so if [Stop] is called the
+context will be done (see [pkg/context] for details).
+*/
+func (t *TwentyTwentyTwenty) Ctx() context.Context { return t.loopCtx }
+
 /*
 Start twenty-twenty-twenty.
 
 This will start the main twenty-twenty-twenty loop in a goroutine, so avoid
 calling this function inside a goroutine.
 */
-func (t *TwentyTwentyTwenty) Start(ctx context.Context) {
+func (t *TwentyTwentyTwenty) Start() {
 	if t.Features.Sound {
 		log.Printf(
 			"Running twenty-twenty-twenty every %.1f minute(s), with %.f second(s) duration and sound set to %t\n",
@@ -43,8 +50,27 @@ func (t *TwentyTwentyTwenty) Start(ctx context.Context) {
 			t.Settings.Duration.Seconds(),
 		)
 	}
+	t.Stop() // make sure we cancel the previous instance
 
-	t.loop(ctxlog.AppendCtx(ctx, slog.Int64("loop", loop.Add(1))))
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	loop++
+	t.loopCtx, t.cancelLoopCtx = context.WithCancel(
+		ctxlog.AppendCtx(context.Background(), slog.Int("loop", loop)),
+	)
+	go t.loop()
+}
+
+/*
+Stop twenty-twenty-twenty.
+*/
+func (t *TwentyTwentyTwenty) Stop() {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	if t.loopCtx != nil {
+		slog.DebugContext(t.loopCtx, "Cancelling main loop context")
+		t.cancelLoopCtx()
+	}
 }
 
 /*
@@ -65,6 +91,7 @@ func (t *TwentyTwentyTwenty) Pause(
 	timerCallbackPos func(),
 ) {
 	log.Printf("Pausing twenty-twenty-twenty for %.2f hour(s)\n", t.Settings.Pause.Hours())
+	t.Stop() // cancelling current twenty-twenty-twenty goroutine
 	timer := time.NewTimer(t.Settings.Pause)
 	defer timer.Stop()
 
@@ -77,7 +104,7 @@ func (t *TwentyTwentyTwenty) Pause(
 		// need to start a new instance before calling the blocking
 		// SendWithDuration(), otherwise if the user call Pause() again,
 		// we are going to call Stop() in the previous loop
-		go t.Start(ctx)
+		t.Start()
 		err := notification.SendWithDuration(
 			ctx,
 			&t.Settings.Duration,
@@ -96,8 +123,8 @@ func (t *TwentyTwentyTwenty) Pause(
 	}
 }
 
-func (t *TwentyTwentyTwenty) loop(ctx context.Context) {
-	slog.DebugContext(ctx, "Starting new loop")
+func (t *TwentyTwentyTwenty) loop() {
+	slog.DebugContext(t.loopCtx, "Starting new loop")
 	ticker := time.NewTicker(t.Settings.Frequency)
 	defer ticker.Stop()
 
@@ -111,7 +138,7 @@ func (t *TwentyTwentyTwenty) loop(ctx context.Context) {
 				go sound.SuspendAfter(min(t.Settings.Duration*3/2, t.Settings.Frequency))
 			}
 			err := notification.SendWithDuration(
-				ctx,
+				t.loopCtx,
 				&t.Settings.Duration,
 				&t.Settings.Sound,
 				"Time to rest your eyes",
@@ -120,7 +147,7 @@ func (t *TwentyTwentyTwenty) loop(ctx context.Context) {
 			if err != nil {
 				log.Printf("Error while sending notification: %v\n", err)
 			}
-		case <-ctx.Done():
+		case <-t.loopCtx.Done():
 			log.Println("Disabling twenty-twenty-twenty")
 			return
 		}

internal/core/core_test.go

@@ -71,9 +71,9 @@ func TestStart(t *testing.T) {
 	// the last notification may or may not come because of timing
 	expectCount := int32(timeout/twenty.Settings.Frequency) - 1
 
-	ctx, cancelCtx := context.WithTimeout(context.Background(), timeout)
-	defer cancelCtx()
-	twenty.Start(ctx)
+	twenty.Start()
+	defer twenty.Stop()
+	time.Sleep(timeout)
 
 	assert.GreaterOrEqual(t, notifier.notificationCount.Load(), expectCount)
 	assert.GreaterOrEqual(t, notifier.notificationCancelCount.Load(), expectCount)
@@ -83,17 +83,18 @@ func TestPause(t *testing.T) {
 	resetCounters()
 
 	const timeout = time.Second
-	ctx, cancelCtx := context.WithTimeout(context.Background(), timeout)
-	defer cancelCtx()
+	go func() { time.Sleep(timeout); twenty.Stop() }()
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
 	callbackPreCalled := false
 	callbackPosCalled := false
 	twenty.Pause(
 		ctx,
 		func() { callbackPreCalled = true },
 		func() { callbackPosCalled = true },
 	)
-	<-ctx.Done()
+	<-twenty.Ctx().Done()
 
 	assert.Equal(t, callbackPreCalled, true)
 	assert.Equal(t, callbackPosCalled, true)
@@ -105,17 +106,19 @@ func TestPauseCancel(t *testing.T) {
 	resetCounters()
 
 	const timeout = time.Second
-	// will be cancelled before the timeout
-	ctx, cancelCtx := context.WithTimeout(context.Background(), timeout/20)
-	defer cancelCtx()
+	go func() { time.Sleep(timeout); twenty.Stop() }()
 
+	// will be cancelled before the timeout
+	ctx, cancel := context.WithTimeout(context.Background(), timeout/10)
+	defer cancel()
 	callbackPreCalled := false
 	callbackPosCalled := false
 	twenty.Pause(
 		ctx,
 		func() { callbackPreCalled = true },
 		func() { callbackPosCalled = true },
 	)
+	<-twenty.Ctx().Done()
 
 	assert.Equal(t, callbackPreCalled, false)
 	assert.Equal(t, callbackPosCalled, false)
@@ -127,9 +130,10 @@ func TestPauseNilCallbacks(t *testing.T) {
 	resetCounters()
 
 	const timeout = time.Second
-	ctx, cancelCtx := context.WithTimeout(context.Background(), timeout)
-	defer cancelCtx()
+	go func() { time.Sleep(timeout); twenty.Stop() }()
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
 	twenty.Pause(ctx, nil, nil)
 
 	assert.Equal(t, notifier.notificationCount.Load(), 1)

internal/core/structs.go

@@ -1,6 +1,8 @@
 package core
 
 import (
+	"context"
+	"sync"
 	"time"
 )
 
@@ -12,6 +14,10 @@ Keeps the main state of the program.
 type TwentyTwentyTwenty struct {
 	Features
 	Settings
+
+	cancelLoopCtx context.CancelFunc
+	loopCtx       context.Context
+	mu            sync.Mutex
 }
 
 /*

main.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"context"
 	"fmt"
 	"log"
 	"log/slog"
@@ -15,10 +14,8 @@ import (
 )
 
 var (
-	ctx       context.Context
-	ctxCancel context.CancelFunc
-	twenty    *core.TwentyTwentyTwenty
-	version   = "development"
+	version = "development"
+	twenty  *core.TwentyTwentyTwenty
 )
 
 func main() {
@@ -58,18 +55,19 @@ func main() {
 	if err != nil {
 		log.Fatalf("Test notification failed: %v. Exiting...", err)
 	}
-
 	twenty = core.New(features, settings)
-	ctx, ctxCancel = context.WithCancel(context.Background())
+	// we need to start notification cancellation in a goroutine to show the
+	// systray as soon as possible (since it depends on the loop() call), but we
+	// also need to give it access to the core.Ctx to cancel it if necessary
+	twenty.Start()
 	go func() {
-		twenty.Start(ctx)
 		if features.Sound {
 			// wait the 1.5x of duration so we have some time for the sounds to
 			// finish playing
 			go sound.SuspendAfter(min(settings.Duration*3/2, settings.Frequency))
 		}
 		err := notification.CancelAfter(
-			ctx,
+			twenty.Ctx(),
 			sentNotification,
 			&twenty.Settings.Duration,
 			&twenty.Settings.Sound,

systray.go

@@ -7,12 +7,19 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
+	"sync"
 
 	"fyne.io/systray"
 )
 
 const systrayEnabled bool = true
 
+func withMutex(mu *sync.Mutex, f func()) {
+	mu.Lock()
+	defer mu.Unlock()
+	f()
+}
+
 func onReady() {
 	systray.SetIcon(systrayIcon)
 	systray.SetTooltip("TwentyTwentyTwenty")
@@ -29,72 +36,85 @@ func onReady() {
 	systray.AddSeparator()
 	mQuit := systray.AddMenuItem("Quit", "Quit the whole app")
 
-	// running `go run . -race` detects a bunch of data races here that are safe
-	// to ignore since the access to the actual critical regions in systray are
-	// protected with a mutex
+	var pauseCtx context.Context
+	var cancelPauseCtx context.CancelFunc
+	var mu sync.Mutex
+
 	for {
 		select {
 		case <-mEnabled.ClickedCh:
-			// make sure that we cancel the current loop
-			ctxCancel()
 			if mEnabled.Checked() {
-				slog.DebugContext(ctx, "Enable button unchecked")
+				slog.DebugContext(twenty.Ctx(), "Enable button unchecked")
+				twenty.Stop()
 
-				mEnabled.Uncheck()
-				mPause.Disable()
+				withMutex(&mu, func() {
+					mEnabled.Uncheck()
+					mPause.Disable()
+				})
 			} else {
-				slog.DebugContext(ctx, "Enable button checked")
-				ctx, ctxCancel = context.WithCancel(context.Background())
-				go twenty.Start(ctx)
+				slog.DebugContext(twenty.Ctx(), "Enable button checked")
+				twenty.Start()
 
-				mEnabled.Check()
-				mPause.Enable()
+				withMutex(&mu, func() {
+					mEnabled.Check()
+					mPause.Enable()
+				})
 			}
 		case <-mPause.ClickedCh:
-			// make sure that we cancel the current loop
-			ctxCancel()
+			if pauseCtx != nil {
+				slog.DebugContext(twenty.Ctx(), "Cancelling current pause")
+				cancelPauseCtx()
+			}
 			if mPause.Checked() {
-				slog.DebugContext(ctx, "Pause button unchecked")
-				ctx, ctxCancel = context.WithCancel(context.Background())
-				go twenty.Start(ctx)
+				slog.DebugContext(twenty.Ctx(), "Pause button unchecked")
+				twenty.Start()
 
-				mEnabled.Enable()
-				mPause.Uncheck()
+				withMutex(&mu, func() {
+					mEnabled.Enable()
+					mPause.Uncheck()
+				})
 			} else {
-				slog.DebugContext(ctx, "Pause button checked")
-				ctx, ctxCancel = context.WithCancel(context.Background())
-				go twenty.Pause(
-					ctx,
-					func() {
-						slog.DebugContext(ctx, "Calling pause callback")
-						mEnabled.Enable()
-						mPause.Uncheck()
-					},
-					nil,
-				)
+				slog.DebugContext(twenty.Ctx(), "Pause button checked")
+				pauseCtx, cancelPauseCtx = context.WithCancel(context.Background())
+				go func() {
+					defer cancelPauseCtx()
+					twenty.Pause(
+						pauseCtx,
+						func() {
+							withMutex(&mu, func() {
+								slog.DebugContext(twenty.Ctx(), "Calling pause callback")
+								mEnabled.Enable()
+								mPause.Uncheck()
+							})
+						},
+						nil,
+					)
+				}()
 
-				mEnabled.Disable()
-				mPause.Check()
+				withMutex(&mu, func() {
+					mEnabled.Disable()
+					mPause.Check()
+				})
 			}
 		case <-mSound.ClickedCh:
 			if mSound.Checked() {
-				slog.DebugContext(ctx, "Sound button unchecked")
+				slog.DebugContext(twenty.Ctx(), "Sound button unchecked")
 				twenty.Settings.Sound = false
 
-				mSound.Uncheck()
+				withMutex(&mu, func() { mSound.Uncheck() })
 			} else {
-				slog.DebugContext(ctx, "Sound button checked")
+				slog.DebugContext(twenty.Ctx(), "Sound button checked")
 				twenty.Settings.Sound = true
 
-				mSound.Check()
+				withMutex(&mu, func() { mSound.Check() })
 			}
 		case <-mQuit.ClickedCh:
-			slog.DebugContext(ctx, "Quit button clicked")
+			slog.DebugContext(twenty.Ctx(), "Quit button clicked")
 			systray.Quit()
 		}
 	}
 }
 
 func onExit() {
-	ctxCancel()
+	twenty.Stop()
 }