Skip to content

Allow disabling of the thread checks as a "migration" - a pattern we can re-use #5425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 21, 2025
7 changes: 5 additions & 2 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,14 @@ type AppMetadata struct {
// Icon contains, if present, a resource of the icon that was bundled at build time.
Icon Resource
// Release if true this binary was build in release mode
// Since 2.3
// Since: 2.3
Release bool
// Custom contain the custom metadata defined either in FyneApp.toml or on the compile command line
// Since 2.3
// Since: 2.3
Custom map[string]string
// Migrations allows an app to opt into features before they are standard
// Since: 2.6
Migrations map[string]bool
}

// Lifecycle represents the various phases that an app can transition through.
Expand Down
16 changes: 10 additions & 6 deletions app/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import (
)

var meta = fyne.AppMetadata{
ID: "",
Name: "",
Version: "0.0.1",
Build: 1,
Release: false,
Custom: map[string]string{},
ID: "",
Name: "",
Version: "0.0.1",
Build: 1,
Release: false,
Custom: map[string]string{},
Migrations: map[string]bool{},
}

// SetMetadata overrides the packaged application metadata.
Expand All @@ -21,6 +22,9 @@ func SetMetadata(m fyne.AppMetadata) {
if meta.Custom == nil {
meta.Custom = map[string]string{}
}
if meta.Migrations == nil {
meta.Migrations = map[string]bool{}
}
}

func (a *fyneApp) Metadata() fyne.AppMetadata {
Expand Down
1 change: 1 addition & 0 deletions app/meta_development.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func checkLocalMetadata() {

meta.Release = false
meta.Custom = data.Development
meta.Migrations = data.Migrations
}

func getProjectPath() string {
Expand Down
3 changes: 3 additions & 0 deletions cmd/fyne/internal/commands/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,9 @@ func (b *Builder) build() error {
if b.release {
tags = append(tags, "release")
}
if ok, set := b.appData.Migrations["fyneDo"]; ok && set {
tags = append(tags, "migrated_fynedo")
}
if len(tags) > 0 {
args = append(args, "-tags", strings.Join(tags, ","))
}
Expand Down
2 changes: 2 additions & 0 deletions cmd/fyne/internal/commands/data.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ type appData struct {
ResGoString string
Release, rawIcon bool
CustomMetadata map[string]string
Migrations map[string]bool
VersionAtLeast2_3 bool
}

Expand Down Expand Up @@ -47,4 +48,5 @@ func (a *appData) mergeMetadata(data *metadata.FyneApp) {
} else {
a.appendCustomMetadata(data.Development)
}
a.Migrations = data.Migrations
}
3 changes: 3 additions & 0 deletions cmd/hello/FyneApp.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@
ID = "io.fyne.hello"
Version = "2.5.3"
Build = 2

[Migrations]
fyneDo = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as further down. Maybe calling this threading = true would make more sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That assumes we will only ever have one threading migration. I'd rather be more specific

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, true but I'm not all too enthusiastic about the previous name either. I would like to hear what @dweymouth thinks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are to do a v3.0.0, then we can remove the tag and use the same tag again in the future in case we want to do one more thread migration (which I kind of think is unlikely before v3.0.0). I am still in favour of migrating threads as a better name (fynedo sounds a bit strange) but I approved this for now so it can land and be useful for more people.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly I think that re-use as you described would be a very bad thing. A migration system where what it means changes over time seems like it will be confusing and probably problematic.
Hence I'm happy we are sticking with what is here as it's explicit.

4 changes: 2 additions & 2 deletions internal/async/goroutine.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func IsMainGoroutine() bool {
//
// This will be removed later and should never be public
func EnsureNotMain(fn func()) {
if build.DisableThreadChecks || !IsMainGoroutine() {
if build.MigratedToFyneDo() || !build.HasHints || !IsMainGoroutine() {
fn()
return
}
Expand All @@ -47,7 +47,7 @@ func EnsureNotMain(fn func()) {
//
// This will be removed later and should never be public
func EnsureMain(fn func()) {
if build.DisableThreadChecks || IsMainGoroutine() {
if build.MigratedToFyneDo() || !build.HasHints || IsMainGoroutine() {
fn()
return
}
Expand Down
27 changes: 26 additions & 1 deletion internal/build/build.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,29 @@
// Package build contains information about they type of build currently running.
package build

const DisableThreadChecks = false
import (
"sync"

"fyne.io/fyne/v2"
)

var (
migrateCheck sync.Once

migratedFyneDo bool
)

func MigratedToFyneDo() bool {
if DisableThreadChecks {
return true
}

migrateCheck.Do(func() {
v, ok := fyne.CurrentApp().Metadata().Migrations["fyneDo"]
if ok {
migratedFyneDo = v
}
})

return migratedFyneDo
}
6 changes: 6 additions & 0 deletions internal/build/migrated_fynedo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
//go:build migrated_fynedo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe migrated_threading would be better? Calling it FyneDo sounds a bit non descriptive to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd thing that "threading" is generic which is potentially problematic too.
The reasoning here is that the name matches an API that developers will have used to prepare this migration.


package build

// DisableThreadChecks disables the thread safety checks for performance.
const DisableThreadChecks = true
6 changes: 6 additions & 0 deletions internal/build/migrated_notfynedo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
//go:build !migrated_fynedo

package build

// DisableThreadChecks set to false enables the thread safety checks for logging of incorrect usage.
const DisableThreadChecks = false
1 change: 1 addition & 0 deletions internal/metadata/data.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ type FyneApp struct {
Source *AppSource `toml:",omitempty"`
LinuxAndBSD *LinuxAndBSD `toml:",omitempty"`
Languages []string `toml:",omitempty"`
Migrations map[string]bool `toml:",omitempty"`
}

// AppDetails describes the build information, this group may be OS or arch specific
Expand Down
15 changes: 9 additions & 6 deletions widget/widget.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package widget // import "fyne.io/fyne/v2/widget"
import (
"fyne.io/fyne/v2"
"fyne.io/fyne/v2/canvas"
"fyne.io/fyne/v2/internal/async"
"fyne.io/fyne/v2/internal/cache"
internalWidget "fyne.io/fyne/v2/internal/widget"
"fyne.io/fyne/v2/theme"
Expand Down Expand Up @@ -111,14 +112,16 @@ func (w *BaseWidget) Hide() {

// Refresh causes this widget to be redrawn in its current state
func (w *BaseWidget) Refresh() {
impl := w.super()
if impl == nil {
return
}
async.EnsureMain(func() {
impl := w.super()
if impl == nil {
return
}

w.themeCache = nil
w.themeCache = nil

cache.Renderer(impl).Refresh()
cache.Renderer(impl).Refresh()
})
}

// Theme returns a cached Theme instance for this widget (or its extending widget).
Expand Down
Loading