Skip to content

Remove Mutex from FontSystem, and make methods take &mut self#39

Closed
ids1024 wants to merge 2 commits intomainfrom
no-mutex
Closed

Remove Mutex from FontSystem, and make methods take &mut self#39
ids1024 wants to merge 2 commits intomainfrom
no-mutex

Conversation

@ids1024
Copy link
Copy Markdown
Member

@ids1024 ids1024 commented Nov 8, 2022

Types wrapping &FontSystem instead need to have methods taking an &mut FontSystem. Eliminating the lifetime bounds make the API more usable, but perhaps too many methods require this to be passed.

Moving interior mutability to the caller means it isn't used unless needed, and the caller can use a Mutex, some nostd alternative, or a RefCell.

This updates the editor-orbclient to compile with the API changes. The others need to be updated, but perhaps not until any changes to make methods not require an &mut FontSystem if possible.

Base automatically changed from no_std to main November 8, 2022 20:24
Types wrapping `&FontSystem` instead need to have methods taking an
`&mut FontSystem`. Eliminating the lifetime bounds make the API more
usable, but perhaps too many methods require this to be passed.

Moving interior mutability to the caller means it isn't used unless
needed, and the caller can use a `Mutex`, some nostd alternative, or a
`RefCell`.

This updates the `editor-orbclient` to compile with the API changes. The
others need to be updated, but perhaps not until any changes to make
methods not require an `&mut FontSystem` if possible.
Copy link
Copy Markdown
Contributor

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Getting rid of the Buffer lifetime is great. It'll help me get rid of ouroboros in my iced integration.

@tigregalis
Copy link
Copy Markdown
Contributor

tigregalis commented Feb 27, 2023

Types wrapping &FontSystem instead need to have methods taking an &mut FontSystem. Eliminating the lifetime bounds make the API more usable, but perhaps too many methods require this to be passed.

Perhaps as future work, to recover some of the ergonomics, when you are calling methods on the same instance repeatedly, so that you're not always passing &mut font_system to every method, you could expose a helper type that borrows the inner type and the FontSystem then forwards the methods:

fn main() {
    let mut font_system = FontSystem::new();
    let param = ();
    let mut buffer = Buffer::new();
    {
        let mut buffer = buffer.with_font_system(&mut font_system);
        buffer.method(param);
    }
}



/// Gets dropped at the end of the scope, releasing the borrows
struct WithFontSystem<'a, T> {
    inner: &'a mut T,
    font_system: &'a mut FontSystem,
}

impl Buffer {
    fn method(&mut self, font_system: &mut FontSystem, param: Param) {
        todo!()
    }

    fn with_font_system<'a>(&'a mut self, font_system: &'a mut FontSystem) -> WithFontSystem<'a, Self> {
        WithFontSystem {
            inner: self,
            font_system,
        }
    }
}

impl<'a> WithFontSystem<'a, Buffer> {
    // ... reimplement all of the methods on `Buffer`, simply forwarding to the relevant `Buffer` methods
    fn method(&mut self, param: Param) {
        self.inner.method(&mut self.font_system, param);
    }
}

@geieredgar
Copy link
Copy Markdown
Contributor

@ids1024 @tigregalis Thank you for your ideas, which I have incorporated in #97. With #97 being merged, I think this can be closed?

@ids1024 ids1024 closed this Mar 19, 2023
@jackpot51 jackpot51 deleted the no-mutex branch March 20, 2023 13:54
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.

5 participants