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

Make data binding thread safe #5361

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

andydotxyz
Copy link
Member

Get/Set can be called from anywhere, Reload too.
Remove all the tedious waiting in data binding tests.
This is not the final public API for goroutine work, but as drivers needed to add this support first I added it to that interface.

Relates to #4654

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style and have Since: line.

Get/Set can be called from anywhere, Reload too.
This is not the final public API, but as drivers needed to add this support first I added it to that interface.
@andydotxyz andydotxyz mentioned this pull request Jan 5, 2025
17 tasks
@coveralls
Copy link

coveralls commented Jan 5, 2025

Coverage Status

coverage: 59.294% (-0.4%) from 59.655%
when pulling 60c9002 on andydotxyz:fix/mergethreads
into 436a222 on fyne-io:develop.

Copy link
Contributor

@dweymouth dweymouth left a comment

Choose a reason for hiding this comment

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

Really nice to see the threading work nearing the finish line! Just a few comments so far

data/binding/queue.go Outdated Show resolved Hide resolved
data/binding/queue.go Outdated Show resolved Hide resolved
driver.go Show resolved Hide resolved
data/binding/binding_test.go Show resolved Hide resolved
@andydotxyz
Copy link
Member Author

Thanks for the comments, I think that's all good now - with an ongoing discussion for public API for enqueuing functions

Copy link
Contributor

@dweymouth dweymouth left a comment

Choose a reason for hiding this comment

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

Approved, so long as we don't care about potential breaking changes on develop in case we rename or move the new public "CallFromGoroutine" function introduced here

@Jacalz
Copy link
Member

Jacalz commented Jan 6, 2025

Approved, so long as we don't care about potential breaking changes on develop in case we rename or move the new public "CallFromGoroutine" function introduced here

We never care about that. It is one of the reasons to have such a branch. Anyone pulling latest master or release/v2.x.y will never get breaking changes :)

But yeah, I agree we need a better name in the future

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.

Nice step in the right direction. As discussed inline, we need a better name for the function to queue events but we can tackle that further down the line :)

@andydotxyz andydotxyz merged commit d47fdda into fyne-io:develop Jan 6, 2025
12 checks passed
@andydotxyz andydotxyz deleted the fix/mergethreads branch January 6, 2025 17:13
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.

4 participants