Add new callback-based settings listener API #5431
Conversation
|
Not to sure how to resolve the conflict through web UI sorry @dweymouth |
app/settings.go
Outdated
| @@ -170,7 +168,13 @@ func (s *settings) setupTheme() { | |||
| variant = theme.VariantDark | |||
| } | |||
|
|
|||
| s.applyTheme(effectiveTheme, variant) | |||
| if async.IsMainGoroutine() { | |||
There was a problem hiding this comment.
Is it OK that setupTheme is sometimes called from non-main?
There was a problem hiding this comment.
That was the reasoning behind adding the IsMainGoroutine check. setupTheme is called from platform-specific code and I wasn't sure which of those would be on main, and which may be from a goroutine
There was a problem hiding this comment.
I don't think we should rely too much on async.IsMainGoroutine(). There is a reason that the Go developers don't expose the goroutine ID in a simple way as it allows people to more easily write bad code. Can we not use async.EnsureMain() for now until the migration is complete?
There was a problem hiding this comment.
All OS specific code should come in on main. If it doesn't then other things will break.
There was a problem hiding this comment.
Then this should not do the ID check at all, right?
There was a problem hiding this comment.
That would be my take. Unless there is some reason we know that this call can come from other places. In that situation it may be more desirable to fyne.Do it at the source.
app/settings.go
Outdated
| @@ -170,7 +168,13 @@ func (s *settings) setupTheme() { | |||
| variant = theme.VariantDark | |||
| } | |||
|
|
|||
| s.applyTheme(effectiveTheme, variant) | |||
| if async.IsMainGoroutine() { | |||
There was a problem hiding this comment.
I don't think we should rely too much on async.IsMainGoroutine(). There is a reason that the Go developers don't expose the goroutine ID in a simple way as it allows people to more easily write bad code. Can we not use async.EnsureMain() for now until the migration is complete?
Description:
The existing Settings.AddChangeListener needs to be deprecated, since it is channel-based, and user code cannot block on channel receives from the main thread, and communicating to user code via a goroutine is against the new threading model.
Checklist:
Where applicable: