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

TextLayout::update_width buggy by design #298

Closed
cmyr opened this issue Sep 4, 2020 · 1 comment · Fixed by #306
Closed

TextLayout::update_width buggy by design #298

cmyr opened this issue Sep 4, 2020 · 1 comment · Fixed by #306
Labels
bug Something isn't working text

Comments

@cmyr
Copy link
Member

cmyr commented Sep 4, 2020

The way TextLayout::update_width works means that it can result in invalid layout objects when a layout has been cloned.

When this method is called, it calls a mutating method on the platform layout object, and updates state (such as line metrics) on the piet object; however the platform layout object is shared between all clones of a layout object, whereas other state is not.

We need to think more about how we want to address this; it may make sense to just remove update_width for the time being.

@cmyr cmyr added bug Something isn't working text labels Sep 4, 2020
@raphlinus
Copy link
Contributor

I agree that I would prefer to remove update_width for now and take up the performance improvement in a future cycle. One of the reasons I come to this conclusion is that the LayoutRope in the xi2 exploration really wants to store immutable text layout objects, so it is additional architectural work there to enable mutable changes all the way through.

That said, I think it's possible. I just think we have our hands full with text work right now without this issue complicating things. My rough thinking is that when we reinstate this function, we will have explicit copy-on-write semantics in the platform wrappers, (ie we'll never call clone on the underlying platform layout object), and use Rc::get_mut() or similar method to explicitly determine when the mutable method is safe. If not, then the platform wrapper is responsible for building a new text platform text layout object of the desired width.

Correspondingly, on the xi-rope side, more work is needed to expose in-place mutation capabilities, again based on make_mut or similar. Until then, mutation of the rope will cause temporary clones, which will defeat the optimization outlined above.

So I still think the method is a great idea (and potentially a really significant performance improvement, especially for live resize of large documents), but we should defer it until after we have the major bones of the text implementation in place and are able to undertake the work to enable the performance optimization end-to-end. I don't think its absence will block other important architectural work; in particular I think it doesn't affect the API provided to the clients of either LayoutRope or the new Druid-level TextLayout object, just the internal implementation.

cmyr added a commit that referenced this issue Sep 12, 2020
@cmyr cmyr closed this as completed in #306 Sep 16, 2020
cmyr added a commit that referenced this issue Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working text
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants