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

Fix the last remaining races in fyne_demo #5382

Merged
merged 5 commits into from
Jan 9, 2025

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Jan 9, 2025

Description:

This adds appropriate safety in goroutines and also moves the advanced tab to only redraw when settings actually change (it was drawing each second before and wasting CPU time). I clicked through all the tabs now and can no longer see any races reported.

For #4654
Fixes #4568

Checklist:

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

Jacalz added 3 commits January 9, 2025 11:33
This adds appropriate safety in groutines and also moves the advanced tab to only redraw when settings actually change (it was incorrectly drawing each second before).
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks so much for this Jacob.

Just a quick question - are you sure it will update with changes to texture scale? (When a window drags to a non-retina screen on macOS...
I think that's why the timer was there.

@Jacalz
Copy link
Member Author

Jacalz commented Jan 9, 2025

Good point. I think it also will be wrong when just dragging between two windows with different DPI, right? I'll change the code to only refresh the widgets when scale changes instead :)

@dweymouth
Copy link
Contributor

I think @andydotxyz was talking about the label that shows the scale info, not the widgets themselves not getting redrawn? The driver handles refreshing the window when the scale changes.

@coveralls
Copy link

coveralls commented Jan 9, 2025

Coverage Status

coverage: 59.374% (+0.06%) from 59.316%
when pulling 73d0cbf on Jacalz:fix-demo-races
into e6225b5 on fyne-io:develop.

@Jacalz
Copy link
Member Author

Jacalz commented Jan 9, 2025

I think @andydotxyz was talking about the label that shows the scale info, not the widgets themselves not getting redrawn? The driver handles refreshing the window when the scale changes.

Indeed, I know. I just meant that I would try to make the existing code a bit faster by not redrawing/refreshing them when the scale hasn't changed. I'm not a huge fan of an infinite loop with refresh each second.

@andydotxyz
Copy link
Member

I guess just be careful because texScale is not a scale change, it's an internal detail to the GLFW windows...

@Jacalz
Copy link
Member Author

Jacalz commented Jan 9, 2025

I ended up just moving to the old once per second update but tidied up the code a bit.

@Jacalz Jacalz requested a review from andydotxyz January 9, 2025 20:37
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Nice use of the animation construct :)

@Jacalz
Copy link
Member Author

Jacalz commented Jan 9, 2025

Thanks. I was rather happy with the one as well. Haven't used it before actually. Now we're just trying to get the flaky Ubuntu test to work :)

@Jacalz Jacalz merged commit f2ba049 into fyne-io:develop Jan 9, 2025
12 checks passed
@Jacalz Jacalz deleted the fix-demo-races branch January 9, 2025 21:23
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