-
Notifications
You must be signed in to change notification settings - Fork 39
Use taffy for tables #129
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
Use taffy for tables #129
Conversation
|
I'll be free to give this a look tomorrow if ya want 👍 |
CosmicHorrorDev
left a comment
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.
Only a couple tiny nits that can just have the suggestions approved to make things easy. Other than that everything looks good :)
Having to wrap the text cache and font system in a Mutex is a bit unfortunate since we don't ever try to access either simultaneously, but it looks like Measurable requires a Sync bound, so we can't use a RefCell. At the very least locking doesn't even show up when I profile things (probably because there's never any contention)
We could switch to parking_lot::Mutex just to avoid having to .unwrap() every time we acquire the lock. It's already in our dep tree in several places, so it doesn't actually add a dependency
src/text.rs
Outdated
| text_cache: &Arc<Mutex<TextCache>>, | ||
| font_system: &Arc<Mutex<FontSystem>>, |
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.
| text_cache: &Arc<Mutex<TextCache>>, | |
| font_system: &Arc<Mutex<FontSystem>>, | |
| text_cache: &Mutex<TextCache>, | |
| font_system: &Mutex<FontSystem>, |
The Arc wrappers can be dropped here because of deref magic
src/table.rs
Outdated
| let max_columns = self.rows.iter().fold(self.headers.len(), |max, row| { | ||
| if row.len() > max { | ||
| row.len() | ||
| } else { | ||
| max | ||
| } | ||
| }); |
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.
| let max_columns = self.rows.iter().fold(self.headers.len(), |max, row| { | |
| if row.len() > max { | |
| row.len() | |
| } else { | |
| max | |
| } | |
| }); | |
| let max_columns = self | |
| .rows | |
| .iter() | |
| .fold(self.headers.len(), |max, row| std::cmp::max(row.len(), max)); |
The if statement here can just be replaced with std::cmp::max(row.len(), max) (suggestion includes reformatting from rustfmt)
nicoburns
left a comment
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.
This looks good to me. There are some refactors that could be done, but if Taffy's scope within Inlyne is going to be expanded in future (e.g. to do the main vertical layout of the page) then it probably makes more sense to make those changes then.
This should be quite easy to do. Just keep the
This is definitely fixable in Taffy. It just requires us to be generic over the |
Really excited for this PR. Should make future additions to table layout logic a lot simpler to deal with (for images and nesting etc).
Right now this doesn't cache layouts and forces
text_cacheandfont_systemto become anArc<Mutex<>>but why fight the borrow checker when you can let it have its way with you. Plus premature optimization is the root of all evil so let's just enjoy properly sized tables.Big thanks to @nicoburns for his help with this.