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 lifetime from Buffer and Mutex from FontSystem #97

Merged
merged 9 commits into from
Mar 18, 2023

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Mar 3, 2023

This includes all changes from #95.

This makes the Buffer and Editor types not borrow FontSystem, changing their methods to take a &mut FontSystem when needed, like #39 does. A helper type BorrowedWithFontSystem is added (this was proposed here: #39 (comment)), which allows calling methods without having to pass a FontSystem for each call.

Making FontSystem::get_font take &mut self also allows us to lazily call make_shared_face_data.

This also adds an FontSystem::db_mut method, allowing mutable access to the fontdb::Database, which also clears the font match cache to make sure that newly added fonts can be matched. This would fix #75 and resolve #57. It also fixes #107.

@geieredgar geieredgar changed the title Call make_shared_face_data only on deman Call make_shared_face_data only on demand Mar 3, 2023
@geieredgar geieredgar marked this pull request as draft March 3, 2023 20:16
@notgull
Copy link
Contributor

notgull commented Mar 4, 2023

By the way, this wouldn't fix #91. There are still outstanding issues with font loading that still slow it down in certain cases.

@geieredgar geieredgar changed the title Call make_shared_face_data only on demand Remove lifetime from Buffer and Mutex from FontSystem Mar 5, 2023
@geieredgar geieredgar marked this pull request as ready for review March 5, 2023 03:28
@geieredgar geieredgar marked this pull request as draft March 12, 2023 10:57
@geieredgar
Copy link
Contributor Author

@notgull Could you check if this improves the load times? I've added a commit so it should only load the fonts that are really needed/used.

@i509VCB
Copy link

i509VCB commented Mar 14, 2023

@notgull Could you check if this improves the load times? I've added a commit so it should only load the fonts that are really needed/used.

This does appear to improve the loading time on my system. You can notice main wait a moment to load the fonts. With this branch it does still take a tiny bit but it's generally done when Plasma's pop in animations for new windows finish.

@geieredgar geieredgar marked this pull request as ready for review March 14, 2023 01:42
Copy link
Member

@jackpot51 jackpot51 left a comment

Choose a reason for hiding this comment

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

All tests are passing and performance is good

@MarijnS95
Copy link
Contributor

MarijnS95 commented Oct 25, 2023

Making FontSystem::get_font take &mut self also allows us to lazily call make_shared_face_data.

User question: I'm upgrading to cosmic-text 0.10 in an internal crate, and have a loop of this form:

for face in system.db().faces() {
    let font = system.get_font(face.id).unwrap();
    let font_ref = font.as_swash();

This won't compile because the .faces() iterator holds a borrow on system, while get_font() also wants to mutably borrow system. Is there a better solution than precomputing the list of IDs with let face_ids = system.db().faces().map(|face| face.id).collect::<Vec<_>>();?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants