The documentation for the `new_text_layout` is inaccurate

[PhilipDaniels]

The documentation for new_text_layout function here https://github.com/linebender/piet/blob/f01d6d93d71e7f9e4045342b309ce28fbfd21859/piet/src/text.rs#L95-L107

implies that an Arc is allocated whenever the function is called. Given the impls at

https://github.com/linebender/piet/blob/f01d6d93d71e7f9e4045342b309ce28fbfd21859/piet/src/text.rs#L625-L659

that does not appear to be true - in particular I was interested in the case for static strings: no allocation will occur.

[cmyr]

Mm, correct, these are slightly out of date. Thanks!

For what it is worth, the implementation is backend dependent, but in general I believe that we do allocate a new Rc or Arc on each call; for instance on macOS a layout contains an Rc<dyn TextStorage>. This means that we can reuse storage, at the cost of an extra layer of pointer indirection. In any case, I can update these docs. :)

[PhilipDaniels]

Hi @cmyr

I think I have found the backend code you are referring to, such as this in the Cairo backend

https://github.com/linebender/piet/blob/436c5d9a68491450968d86be6b00a29178315c8f/piet-cairo/src/text.rs#L35-L37

Thank you for pointing it out.

So I if have a non-static &str my only solution is to make an Rc or Arc or String - there's no impl that takes a lifetime other than static that I can see.

I had a use-case in some code I was contribbing to the Lapce editor. Basically there is a utility method that is used to measure a piece of text. It's called a lot and was doing a lot of to_string calls which seemed wasteful. Given that the text_layout object was thrown away at the end of the function, the solution was to lie about the lifetime of the &str being measured. The relevant bit of code is here

https://github.com/lapce/lapce/blob/d57f85892acd8afa915d9f963889e040e18ebf55/lapce-data/src/config.rs#L591-L628

I didn't come up with this hack, and it's the first bit of unsafe code I have ever written but it does work :-) Lapce does not crash with this in place and seems to function as before.

It's possible there is a better way of doing this (another API you have?) or maybe it indicates a misunderstanding or a hole in the API surface. It feels like a common requirement though. Your API shouldn't expect clients to put all their text behind a reference count?

Interested in your comments.

[cmyr]

Hi @cmyr

I think I have found the backend code you are referring to, such as this in the Cairo backend

https://github.com/linebender/piet/blob/436c5d9a68491450968d86be6b00a29178315c8f/piet-cairo/src/text.rs#L35-L37

Thank you for pointing it out.

So I if have a non-static &str my only solution is to make an Rc or Arc or String - there's no impl that takes a lifetime other than static that I can see.

Yea, our API is a bit bad, and was tailored specifically for druid, which encourages the use of Arc<str> where possible.

I had a use-case in some code I was contribbing to the Lapce editor. Basically there is a utility method that is used to measure a piece of text. It's called a lot and was doing a lot of to_string calls which seemed wasteful. Given that the text_layout object was thrown away at the end of the function, the solution was to lie about the lifetime of the &str being measured. The relevant bit of code is here

https://github.com/lapce/lapce/blob/d57f85892acd8afa915d9f963889e040e18ebf55/lapce-data/src/config.rs#L591-L628

I didn't come up with this hack, and it's the first bit of unsafe code I have ever written but it does work :-) Lapce does not crash with this in place and seems to function as before.

This code is a bit scary. It is essentially erasing the lifetime, which should be undefined behaviour if the the layout is used after whatever the source of the borrowed str is has been deallocated.

It's possible there is a better way of doing this (another API you have?) or maybe it indicates a misunderstanding or a hole in the API surface. It feels like a common requirement though. Your API shouldn't expect clients to put all their text behind a reference count?

Interested in your comments.

firstly, in general with width measurement it is important to do caching; I'm not sure if this is happening elsewhere in the project, but before creating a TextLayout you should be first checking if this word/font/size combo is in your cache, and then you can just reuse the value.

secondly, you might be able to do something fancy, depending on how you are representing your text internally. For instance, if you were using a rope internally then you could have a custom type that stored the rope and a range, and you could implement TextStorage for that type.

It is conceivable that we could have a separate API that was just for measurement and which did not retain the provided text; this just hasn't been a need of ours thus far.

Where exactly are you using this method? Doing a quick search I see that it's currently being used in one place, in a paint method; this should really be happening somewhere like layout (or even just in update, if the font has changed?) and the result should definitely be cached and reused.

Looking at your code, it would be nice if we just had a utility method for determining the advance width of a glyph from the font, which would be all you need in the fixed-width case...

[PhilipDaniels]

Thanks for your thoughts. I agree it's not nice, I consider it more of a temporary hack than anything. I did consider implementing a cache for text measurement but it seemed a tricky bit of work considering I not yet really familiar with the codebase. The thing with caches, is always invalidating them at the right time and making lookups fast enough to be worth it.

Lapce does actually use Druid, and the text for the main document is stored in a rope so your approach of implementing TextStorage is one possibility. But there are also a fair number of other usages, such as for menus and palettes and other UI chrome.

I'll also have a look at moving things to the layout events. Lapce is still early-days, there's been quite a lot of work recently to clean up the codebase and this is just one aspect of that. Eventually I aim to get it to support proportional fonts so all this "measure W" stuff will have to go away anyway.