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

Remove closed windows in place without allocating a new slice #5427

Merged
merged 5 commits into from
Jan 19, 2025

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Jan 17, 2025

Description:

Just a small improvement that I noticed could be done, thanks to everything happening on one thread, while working on #5422. Will give a bit of a merge conflict but this seems to make more sense separately (especially if that for some reason wouldn't be ready in time for 2.6).

This also includes a fix from Drew from the same PR to address window repaints and animation ticking happening once for every window on each frame tick. We were basically doing a frame render O(n^2) times, where n is amount of visible windows.

Checklist:

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

Jacalz and others added 3 commits January 17, 2025 19:11
As noted by Drew, we should not tick animations and draw the frame once for every window available. This would make us slower the more windows you have and tick animations too fast.

Co-authored-by: Drew Weymouth <[email protected]>
@coveralls
Copy link

coveralls commented Jan 17, 2025

Coverage Status

coverage: 59.03% (+0.04%) from 58.99%
when pulling 95f99a9 on Jacalz:cleanup-closed-windows-inline
into 6fc5c9a on fyne-io:develop.

@andydotxyz
Copy link
Member

Looks like a race or two opened up in the tests

@Jacalz
Copy link
Member Author

Jacalz commented Jan 17, 2025

The race seems like the same one I fixed in the other PR, I think

@andydotxyz
Copy link
Member

The race seems like the same one I fixed in the other PR, I think

Ah, perhaps - let's get that in and rebase to make sure.

@Jacalz
Copy link
Member Author

Jacalz commented Jan 19, 2025

The race does indeed look to be the same :)

@Jacalz Jacalz requested a review from andydotxyz January 19, 2025 09:36
@Jacalz Jacalz merged commit 4fb39d6 into fyne-io:develop Jan 19, 2025
12 checks passed
@Jacalz Jacalz deleted the cleanup-closed-windows-inline branch January 19, 2025 14:57
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.

3 participants