[refactor] several simplifications after review

- simplify notification routine (we dont need no rejected action). we
  just check every hour, as in the original code.
- open links directly from Qt
- rename some global variables to make them less cryptic
- move cleanup function to the same module that created them

go.mod

@@ -11,12 +11,14 @@ require (
 	github.com/agl/ed25519 v0.0.0-20170116200512-5312a6153412 // indirect
 	github.com/apparentlymart/go-openvpn-mgmt v0.0.0-20161009010951-9a305aecd7f2
 	github.com/dchest/siphash v1.2.1 // indirect
+	github.com/godoctor/godoctor v0.0.0-20181123222458-69df17f3a6f6 // indirect
 	github.com/jmshal/go-locale v0.0.0-20190124211249-eb00fb25cc61
 	github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 // indirect
 	github.com/keybase/go-ps v0.0.0-20190827175125-91aafc93ba19
 	github.com/rakyll/statik v0.1.7
 	github.com/sevlyar/go-daemon v0.1.5
 	github.com/stretchr/testify v1.3.0 // indirect
+	github.com/willf/bitset v1.1.10 // indirect
 	golang.org/x/crypto v0.0.0-20191105034135-c7e5f84aec59 // indirect
 	golang.org/x/sys v0.0.0-20200212091648-12a6c2dcc1e4
 	golang.org/x/text v0.3.2

go.sum

@@ -16,6 +16,8 @@ github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dchest/siphash v1.2.1 h1:4cLinnzVJDKxTCl9B01807Yiy+W7ZzVHj/KIroQRvT4=
 github.com/dchest/siphash v1.2.1/go.mod h1:q+IRvb2gOSrUnYoPqHiyHXS0FOBBOdl6tONBlVnOnt4=
+github.com/godoctor/godoctor v0.0.0-20181123222458-69df17f3a6f6 h1:iRn4qXDcqlb2sFqTdyTdYKjaPoLka7tvcyF+FZA9/qw=
+github.com/godoctor/godoctor v0.0.0-20181123222458-69df17f3a6f6/go.mod h1:+tyhT8jBF8E0XvdlSXOSL7Iko7DlNiongHq3q+wcsPs=
 github.com/jmshal/go-locale v0.0.0-20190124211249-eb00fb25cc61 h1:9vsXCXRCUb82jJKv4O+R8Hyo4oPJsOjVwT0pWvHgeyc=
 github.com/jmshal/go-locale v0.0.0-20190124211249-eb00fb25cc61/go.mod h1:+Ny9b1U6p4zX0L9w+k3hSkz3puupLFP14Mion+rGNF8=
 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 h1:iQTw/8FWTuc7uiaSepXwyf3o52HaUYcV+Tu66S3F5GA=
@@ -31,6 +33,8 @@ github.com/sevlyar/go-daemon v0.1.5/go.mod h1:6dJpPatBT9eUwM5VCw9Bt6CdX9Tk6UWvhW
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
+github.com/willf/bitset v1.1.10 h1:NotGKqX0KwQ72NUzqrjZq5ipPNDQex9lo3WpaS8L2sc=
+github.com/willf/bitset v1.1.10/go.mod h1:RjeCKbqT1RxIR/KWY6phxZiaY1IyutSBfGjNPySAYV4=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=

gui/backend.go

@@ -37,11 +37,6 @@ func DonateAccepted() {
 	backend.DonateAccepted()
 }
 
-//export DonateRejected
-func DonateRejected() {
-	backend.DonateRejected()
-}
-
 //export SubscribeToEvent
 func SubscribeToEvent(event string, f unsafe.Pointer) {
 	backend.SubscribeToEvent(event, f)

gui/handlers.cpp

@@ -30,16 +30,6 @@ void Backend::donateAccepted()
     DonateAccepted();
 }
 
-void Backend::donateRejected()
-{
-    DonateRejected();
-}
-
-void Backend::openURL(QString link)
-{
-    QDesktopServices::openUrl(QUrl(link));
-}
-
 void Backend::quit()
 {
     Quit();

gui/handlers.h

@@ -34,8 +34,6 @@ public slots:
     void switchOff();
     void unblock();
     void donateAccepted();
-    void donateRejected();
-    void openURL(QString link);
     void quit();
 };
 

gui/qml/DonateDialog.qml

@@ -16,13 +16,8 @@ MessageDialog {
 
     onAccepted: {
         if (backend) {
-            backend.openURL(ctx.donateURL)
+            Qt.openUrlExternally(ctx.donateURL)
             backend.donateAccepted()
         }
     }
-    onRejected: {
-        if (backend) {
-            backend.donateRejected()
-        }
-    }
 }

gui/qml/main.qml

@@ -178,7 +178,7 @@ ApplicationWindow {
 
             MenuItem {
                 text: qsTr("Help...")
-                onTriggered: backend.openURL(ctx.helpURL)
+                onTriggered: Qt.openUrlExternally(ctx.helpURL)
             }
 
             MenuItem {

pkg/backend/api.go

@@ -6,7 +6,6 @@ import (
 	"C"
 	"fmt"
 	"log"
-	"time"
 	"unsafe"
 
 	"0xacab.org/leap/bitmask-vpn/pkg/bitmask"
@@ -34,17 +33,13 @@ func Quit() {
 		ctx.cfg.SetUserStoppedVPN(true)
 		stopVPN()
 	}
-	cleanupTempDirs()
+	cleanup()
 }
 
 func DonateAccepted() {
 	donateAccepted()
 }
 
-func DonateRejected() {
-	donateRejected()
-}
-
 func SubscribeToEvent(event string, f unsafe.Pointer) {
 	subscribe(event, f)
 }
@@ -53,17 +48,8 @@ func InitializeBitmaskContext() {
 	p := bitmask.GetConfiguredProvider()
 
 	initOnce.Do(func() { initializeContext(p.Provider, p.AppName) })
+	runDonationReminder()
 	go ctx.updateStatus()
-
-	go func() {
-		if needsDonationReminder() {
-			// wait a bit before launching reminder
-			timer := time.NewTimer(time.Minute * 5)
-			<-timer.C
-			showDonate()
-		}
-
-	}()
 }
 
 func RefreshContext() *C.char {

pkg/backend/callbacks.go

@@ -19,11 +19,11 @@ import (
 // }
 import "C"
 
-/* callbacks into C-land */
+/* callbacks into C-land. We keep a registry, and protect its updates with a mutex. */
+
+var callbacks = make(map[string](*[0]byte))
+var callbackMutex sync.Mutex
 
-var mut sync.Mutex
-var stmut sync.Mutex
-var cbs = make(map[string](*[0]byte))
 var initOnce sync.Once
 
 // Events are just a enumeration of all the posible events that C functions can
@@ -38,23 +38,23 @@ const OnStatusChanged string = "OnStatusChanged"
 // subscribe registers a callback from C-land.
 // This callback needs to be passed as a void* C function pointer.
 func subscribe(event string, fp unsafe.Pointer) {
-	mut.Lock()
-	defer mut.Unlock()
+	callbackMutex.Lock()
+	defer callbackMutex.Unlock()
 	e := &Events{}
 	v := reflect.Indirect(reflect.ValueOf(&e))
 	hf := v.Elem().FieldByName(event)
 	if reflect.ValueOf(hf).IsZero() {
 		fmt.Println("ERROR: not a valid event:", event)
 	} else {
-		cbs[event] = (*[0]byte)(fp)
+		callbacks[event] = (*[0]byte)(fp)
 	}
 }
 
 // trigger fires a callback from C-land.
 func trigger(event string) {
-	mut.Lock()
-	defer mut.Unlock()
-	cb := cbs[event]
+	callbackMutex.Lock()
+	defer callbackMutex.Unlock()
+	cb := callbacks[event]
 	if cb != nil {
 		C._do_callback(cb)
 	} else {

pkg/backend/cleanup.go

 package backend
 
 import (
-	"log"
-	"os"
-	"path"
-	"path/filepath"
+	"0xacab.org/leap/bitmask-vpn/pkg/vpn"
 )
 
-func cleanupTempDirs() {
-	dirs, _ := filepath.Glob(path.Join(os.TempDir(), "leap-*"))
-	for _, d := range dirs {
-		log.Println("removing temp dir:", d)
-		os.RemoveAll(d)
-	}
+func cleanup() {
+	vpn.Cleanup()
 }

pkg/backend/donate.go

 package backend
 
 import (
-	"log"
 	"time"
 
 	"0xacab.org/leap/bitmask-vpn/pkg/config"
 )
 
+// runDonationReminder checks every hour if we need to show the reminder,
+// and trigger the launching of the dialog if needed.
+func runDonationReminder() {
+	go func() {
+		for {
+			time.Sleep(time.Hour)
+			if needsDonationReminder() {
+				showDonate()
+			}
+		}
+	}()
+}
+
 func wantDonations() bool {
 	if config.AskForDonations == "true" {
 		return true
@@ -19,25 +31,16 @@ func needsDonationReminder() bool {
 }
 
 func donateAccepted() {
-	stmut.Lock()
-	defer stmut.Unlock()
+	statusMutex.Lock()
+	defer statusMutex.Unlock()
 	ctx.DonateDialog = false
-	log.Println("marking as donated")
 	ctx.cfg.SetDonated()
 	go trigger(OnStatusChanged)
 }
 
-func donateRejected() {
-	timer := time.NewTimer(time.Hour)
-	go func() {
-		<-timer.C
-		showDonate()
-	}()
-}
-
 func showDonate() {
-	stmut.Lock()
-	defer stmut.Unlock()
+	statusMutex.Lock()
+	defer statusMutex.Unlock()
 	ctx.DonateDialog = true
 	ctx.cfg.SetLastReminded()
 	go trigger(OnStatusChanged)

pkg/backend/status.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"log"
+	"sync"
 
 	"0xacab.org/leap/bitmask-vpn/pkg/bitmask"
 	"0xacab.org/leap/bitmask-vpn/pkg/config"
@@ -17,6 +18,8 @@ const (
 	failedStr   = "failed"
 )
 
+var statusMutex sync.Mutex
+
 // ctx will be our glorious global object.
 // if we ever switch again to a provider-agnostic app, we should keep a map here.
 var ctx *connectionCtx
@@ -42,8 +45,8 @@ type connectionCtx struct {
 }
 
 func (c connectionCtx) toJson() ([]byte, error) {
-	stmut.Lock()
-	defer stmut.Unlock()
+	statusMutex.Lock()
+	defer statusMutex.Unlock()
 	b, err := json.Marshal(c)
 	if err != nil {
 		log.Println(err)
@@ -69,8 +72,8 @@ func (c connectionCtx) updateStatus() {
 }
 
 func setStatus(st status) {
-	stmut.Lock()
-	defer stmut.Unlock()
+	statusMutex.Lock()
+	defer statusMutex.Unlock()
 	ctx.Status = st
 	go trigger(OnStatusChanged)
 }

pkg/vpn/main.go

@@ -19,6 +19,8 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
+	"path"
+	"path/filepath"
 
 	"0xacab.org/leap/bitmask-vpn/pkg/config"
 	"0xacab.org/leap/bitmask-vpn/pkg/vpn/bonafide"
@@ -52,10 +54,15 @@ func Init() (*Bitmask, error) {
 	b := Bitmask{tempdir, statusCh, nil, bonafide, launch, "", nil}
 
 	/*
-		err = b.StopVPN()
-		if err != nil {
-			return nil, err
-		}
+		TODO -- we still want to do this, since it resets the fw/vpn if running
+		from a previous one, but first we need to complete all the
+		system/helper checks that we can do. otherwise this times out with an
+		error that's captured badly as of today.
+
+			err = b.StopVPN()
+			if err != nil {
+				return nil, err
+			}
 	*/
 
 	err = ioutil.WriteFile(b.getCaCertPath(), config.CaCert, 0600)
@@ -87,3 +94,11 @@ func (b *Bitmask) Close() {
 func (b *Bitmask) Version() (string, error) {
 	return "", nil
 }
+
+func Cleanup() {
+	dirs, _ := filepath.Glob(path.Join(os.TempDir(), "leap-*"))
+	for _, d := range dirs {
+		log.Println("removing temp dir:", d)
+		os.RemoveAll(d)
+	}
+}