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

Move fields from Buffer into a BufferData struct #86

Closed
wants to merge 1 commit into from

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Feb 28, 2023

As noted by others in #39, keeping Buffers around is cumbersome because it borrows FontSystem. This PR tries to solve that by making Buffer a wrapper around a new BufferData struct that contains all data previously owned by Buffer.

Buffer::from_data and Buffer::into_data allow converting between Buffer and BufferData.

Users of Buffer shouldn't need to change anything, because Buffer references are coerced to BufferData references via Deref/DerefMut.

Note: This is only a first step towards better ergonomics, because moving BufferData into Buffer can be itself pretty cumbersome and may require suboptimal code like using Option<BufferData>.
I envision the following steps for a follow-up:

  1. Make Buffer borrow BufferData instead of owning it. The idea is that users would always store BufferData instead of Buffer and create a temporary Buffer when they need to work with it.
  2. (Optional) If it is desired to keep an owning Buffer, we could instead rename Buffer to OwnedBuffer, and add something like this:
pub struct Buffer<'a> {
    font_system: &'a FontSystem,
    data: &'a mut BufferData,
}

pub trait BufferMethods {
    fn inner(&mut self) -> (&FontSystem, &mut BufferData);

    // Methods of the old Buffer would be moved here, with a default implementation 
    // that operates on the FontSystem and BufferData returned by inner
    ...
}

impl<'a> BufferMethods for Buffer<'a> {
    fn inner(&mut self) -> (&FontSystem, &mut BufferData) {
        (self.font_system, self.data)
    }
}  

impl<'a> BufferMethods for OwnedBuffer<'a> {
    fn inner(&mut self) -> (&FontSystem, &mut BufferData) {
        (self.font_system, &mut self.data)
    }
}

@geieredgar geieredgar changed the title Move fields from Buffer into new BufferData struct Move fields from Buffer into a BufferData struct Feb 28, 2023
@dhardy
Copy link

dhardy commented Feb 28, 2023

Is it possible to construct a BufferData without a &FontSystem? That would be the way I would want to use this.

KAS-text has an even lower level decomposition: Text is a "text" + Environment (options) + TextDisplay (runs & glyphs without the source text), though in practice the only use I found for this is being generic over the text (String, &str` or some type of rich text providing formatting markers). The font library is completely excluded (it's a lazy static, and not accessed until the text is "prepared").

@geieredgar
Copy link
Contributor Author

Not with this PR alone, but I think a possible addition could be adding a flag to BufferData that signals Buffer::from_data to prepare the text.

Keeping `Buffer`s around is cumbersome because it borrows `FontSystem`.
This tries to solve that by making `Buffer` a wrapper around a new
`BufferData` struct that contains all data previously owned by `Buffer`.
`Buffer::from_data` and `Buffer::into_data` allow converting between
`Buffer` and `BufferData`.
Users of `Buffer` don't need to change anything, because `Buffer`
implements `Deref` and `DerefMut` with `Target = BufferData`.
@geieredgar
Copy link
Contributor Author

Closing this in favor of #97.

@geieredgar geieredgar closed this Mar 5, 2023
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.

2 participants