Skip to content
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

Moving window handling functions into a safer transition mode #5415

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Jan 15, 2025

Avoid the most likely crashes as people try out 2.6

Description:

Progressing #5411

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

I tested using the following code:

package main

import (
	"time"

	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/container"
	"fyne.io/fyne/v2/widget"
)

func main() {
	a := app.New()
	w := a.NewWindow("Hello")

	w.SetContent(widget.NewButton("Hi!", func() {}))

	go func() {
		time.Sleep(time.Second * 2)
		w2 := a.NewWindow("Two")
		w2.SetContent(widget.NewLabel("Small"))
		w2.Show()

		time.Sleep(time.Second)
		w2.SetContent(widget.NewLabel("Updated"))

		time.Sleep(time.Second)
		w2.Hide()
	}()

	w.ShowAndRun()
}

Which worked, but logged this:

2025/01/15 14:08:39 *** Error in Fyne call thread, this should have been called in fyne.Do ***
2025/01/15 14:08:39   From: /Users/andy/Code/Fyne/fyne/cmd/hello/main.go:27
2025/01/15 14:08:39 *** Error in Fyne call thread, this should have been called in fyne.Do ***
2025/01/15 14:08:39   From: /Users/andy/Code/Fyne/fyne/cmd/hello/main.go:28
2025/01/15 14:08:40 *** Error in Fyne call thread, this should have been called in fyne.Do ***
2025/01/15 14:08:40   From: /Users/andy/Code/Fyne/fyne/cmd/hello/main.go:31
2025/01/15 14:08:41 *** Error in Fyne call thread, this should have been called in fyne.Do ***
2025/01/15 14:08:41   From: /Users/andy/Code/Fyne/fyne/cmd/hello/main.go:34

Sorry, something went wrong.

Avoid the most likely crashes as people try out 2.6
@coveralls
Copy link

coveralls commented Jan 15, 2025

Coverage Status

coverage: 59.023% (-0.2%) from 59.189%
when pulling a3ff2a6 on andydotxyz:fix/smootherfynedotransition
into d7eeddc on fyne-io:develop.

…t the wrong time or recursively
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. Will test this in Rymdport but this was just a quick drive-by review on my phone in the meantime :)

@@ -1,2 +1,4 @@
// Package build contains information about they type of build currently running.
package build

const DisableThreadChecks = false
Copy link
Member

Choose a reason for hiding this comment

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

This is always false. There is no build tag?

Copy link
Member

Choose a reason for hiding this comment

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

As for a build tag, I'd rather have something like new-threading-model or maybe using-correct-threads (or similar) over disable-thread-checks. The latter sounds a bit negative like you are now using unsafe and might shoot yourself in the foot 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure I agree with that, to me the current name is more clear what it's doing, where the new-threading-model name suggests it would be enabling something bigger (like if we supported both the old and new threading models together and the app could choose which). And the naming of using-correct-threads doesn't really suggest what it does at all IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the confusion, there is a local TODO in my code maybe i should have left it here ;).
Disabling is a follow up as that is where the naming discussions come in!

go fn()
}

func EnsureMain(fn func()) {
Copy link
Member

Choose a reason for hiding this comment

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

I kind of feel like these should have comments as they are exported (although in an internal package but anyway)? Applies to the ones above as well.

@andydotxyz
Copy link
Member Author

@dweymouth @Jacalz thanks for the comments.
How and when to disable this functionality is a follow up PR as outlined in the parent ticket. I don't think that a build flag is the way because it is important that the project is marked as transitioned at a source level IMHO.

My aim is to land this code ASAP so developers on this branch get the immediate benefit and we get more testing.

@Jacalz
Copy link
Member

Jacalz commented Jan 17, 2025

I don't think that a build flag is the way because it is important that the project is marked as transitioned at a source level IMHO.

How would you otherwise do it? I need another way of turning it off for Flatpak compared to FyneApp.toml. What I would propose, like on Slack, is having a build tag that gets set by cmd/fyne when a value in FyneApp.toml is set.

@Jacalz
Copy link
Member

Jacalz commented Jan 17, 2025

Thanks for the doc comments. I don't think we need "Since:" for internal packages but you don't have to remove it :)

@andydotxyz
Copy link
Member Author

How would you otherwise do it? I need another way of turning it off for Flatpak compared to FyneApp.toml. What I would propose, like on Slack, is having a build tag that gets set by cmd/fyne when a value in FyneApp.toml is set.

The point of following up with another PR is so we can have that discussion - I don't know the answer for sure and want to discuss it. It's pretty clear from people testing develop that we need this - so can we get it added before deciding how and when it can be disabled?

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

I'm still in favour of a build tag to disabled this but approved for now as we obviously need that transition for older codebases.

@andydotxyz andydotxyz merged commit abef71c into fyne-io:develop Jan 17, 2025
12 checks passed
@andydotxyz andydotxyz deleted the fix/smootherfynedotransition branch January 17, 2025 13:48
@andydotxyz
Copy link
Member Author

I'm still in favour of a build tag to disabled this but approved for now as we obviously need that transition for older codebases.

Thanks. I will open the follow-on PR shortly and we can discuss further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants