-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Move events to main thread and remove unneeded locks #5322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Just one minor comment (not necessarily request for changes) inline. On more question: are there any parts where we should still support calling methods on different threads? Just asking so we don't get carried away and rip out too many locks at once :)
impl atomic.Pointer[fyne.Widget] | ||
propertyLock sync.RWMutex | ||
themeCache fyne.Theme | ||
impl atomic.Pointer[fyne.Widget] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should atomic types and things from the async
package also be removed or is that perhaps better to have as a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same. I think later PR as it could impact semantics too
FYI: There seems to be compilation issues? |
Yeah I just noteiced I didn't compile the tests for last refactor. Updated |
I think it's just data binding which requires more work for various reasons. |
Looks like there may be a deadlock on Linux only - I'll check it out tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine on Linux for me. Maybe deadlock is for tests only?
Yes it was, or a codepath that was not executed on the testing. Fixed. |
One thing led to another and I just refactored the event queue out in this PR too. Fixes a few flakey tests :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Well done. I love it when the diff of added vs removed looks like it does now. It also feels like the event queue removal made a huge performance difference compared to a previous version of this PR :)
This means for race-free apps except for data binding and goroutines, which are covered later, separately.
Progresses #4654
Checklist: