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

Use TextBuffer for layouter in TextEdit instead of &str #5712

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kernelkind
Copy link

@kernelkind kernelkind commented Feb 11, 2025

This change allows layouter to use the TextBuffer instead of &str in the closure. It is necessary when layout decisions depend on more than just the raw string content, such as metadata stored in the concrete type implementing TextBuffer.

In our use case, we needed this to support mention highlighting when a user selects a mention. Since mentions can contain spaces, determining mention boundaries from the &str alone is impossible. Instead, we use the TextBuffer implementation to retrieve the correct bounds.

See the video below for a demonstration:

Screen.Recording.2025-02-11.at.4.30.13.PM.mov

Breaking change

This PR introduces a breaking change to the layouter function in TextEdit.

Previous API:

pub fn layouter(mut self, layouter: &'t mut dyn FnMut(&Ui, &str, f32) -> Arc<Galley>) -> Self

New API:

pub fn layouter(mut self, layouter: &'t mut dyn FnMut(&Ui, &dyn TextBuffer, f32) -> Arc<Galley>) -> Self

Impact on Existing Code

• Any existing usage of layouter will no longer compile.
• Callers must update their closures to use &dyn TextBuffer instead of &str.

Migration Guide

Before:

let mut layouter = |ui: &Ui, text: &str, wrap_width: f32| {
    let layout_job = my_highlighter(text);
    layout_job.wrap.max_width = wrap_width;
    ui.fonts(|f| f.layout_job(layout_job))
};

After:

let mut layouter = |ui: &Ui, text: &dyn TextBuffer, wrap_width: f32| {
    let layout_job = my_highlighter(text.as_str());
    layout_job.wrap.max_width = wrap_width;
    ui.fonts(|f| f.layout_job(layout_job))
};

  • There is not an issue for this change.
  • I have followed the instructions in the PR template

@kernelkind kernelkind marked this pull request as ready for review February 11, 2025 21:20
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Makes sense

@emilk emilk added feature New feature or request egui labels Mar 21, 2025
@kernelkind
Copy link
Author

added suggested change as_any and rebased with master

@kernelkind kernelkind requested a review from emilk March 23, 2025 19:45
}

/// Immutable view of a `&str`!
impl TextBuffer for &str {
impl TextBuffer for &'static str {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were these changed to 'static? Wouldn't this mean some_string.as_str() stops implementing TextBuffer with this change?

Copy link
Author

Choose a reason for hiding this comment

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

std::any::Any only works for static types, so that's the downside of using it. If this isn't acceptable we can revert back to the type id implementation, unless you have a better idea.

Copy link
Collaborator

@lucasmerlin lucasmerlin Mar 27, 2025

Choose a reason for hiding this comment

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

Ah I see, that's annoying 🤔 What do you need Any for?
I think I'd prefer not having the 'static limitation, if the type_id thing works well enough for your usecase

Copy link
Author

Choose a reason for hiding this comment

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

Any is useful for downcasting without writing unsafe code:

pub fn downcast_post_buffer(buffer: &dyn TextBuffer) -> Option<&PostBuffer> {
    buffer.as_any().downcast_ref::<PostBuffer>()
}

But with using type_id we need:

pub fn downcast_post_buffer(buffer: &dyn TextBuffer) -> Option<&PostBuffer> {
    let mut hasher = DefaultHasher::new();
    TypeId::of::<PostBuffer>().hash(&mut hasher);
    let post_id = hasher.finish() as usize;

    if buffer.type_id() == post_id {
        unsafe { Some(&*(buffer as *const dyn TextBuffer as *const PostBuffer)) }
    } else {
        None
    }
}

That's understandable to not want the 'static limitation, I can certainly revert back to using type_id instead of Any

Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilk what do you prefer, considering the 'static limitation?

Copy link
Owner

Choose a reason for hiding this comment

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

The 'static limitation is annoying; I din't foresee that.

I suggest we either go with @kernelkind ideas of returning a std::any::TypeId, or we return an Option<&dyn std::any::Any> that returns None for non-'static types

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5712-updatelayouter
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@kernelkind
Copy link
Author

reverted to 6b999e0 and rebased with master

@kernelkind
Copy link
Author

kernelkind commented Mar 27, 2025

I think it would be better to use std::any::TypeId instead of usize. Then implementers would only need to do:

    fn type_id(&self) -> std::any::TypeId {
        std::any::TypeId::of::<Self>()
    }

instead of:

    fn type_id(&self) -> usize {
        let mut hasher = DefaultHasher::new();
        std::any::TypeId::of::<Self>().hash(&mut hasher);
        hasher.finish() as usize
    }

though we would lose the trait level implementation, so all implementers would be forced to implement type_id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants