Skip to content

Conversation

@nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Sep 9, 2025

We should probably wait until we've backported linebender_resource_handle to peniko 0.4 to land this. Then the font/blob conversion code can disappear.

We may want to name parley::Rect something more specific like parley::SelectionArea

Depends on:

@nicoburns nicoburns force-pushed the remove-kurbo-peniko branch 6 times, most recently from 34c41f7 to 6be402c Compare September 15, 2025 15:06
@nicoburns nicoburns force-pushed the remove-kurbo-peniko branch 5 times, most recently from 4ef0637 to dfa357f Compare September 15, 2025 17:42
@nicoburns nicoburns marked this pull request as ready for review September 15, 2025 17:46
@nicoburns nicoburns requested a review from DJMcNab September 15, 2025 17:46
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

This is a good way to achieve the goal, and I agree with the goal. I've got some thoughts about making sure that we make clear to our users and tooling what the purpose of this new type is.


/// A rectangle.
#[derive(Clone, Copy, Default, PartialEq)]
pub struct Rect {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making this non_exhaustive (and removing pub fn new/Default), to gently discourage more general use. Obviously if people can get a single Rect, since it's Copy, they can easily make new ones, but it should be sufficient roadblocks to prevent people doing that.

I'd also consider renaming to something like OutputRect or similar. In particular, I never want Rust Analyzer to suggest this type when I am looking to import Rect.
Basically strongly saying to applications that they should standardise on a rectangle type, and it shouldn't be this one.

That would make this PR much easier for me to approve, as it makes this API much clearer.
But I'd also suggest that we can talk about this in office hours.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess so. I don't really care if anybody uses it, but I do agree that it would be good to remove it from autocomplete for Rect. How about a rename to BoundingBox (with an eye on using BoundingBox from the font-types crate once font-types hits 1.0.

Or we could even be really specific with CursorArea.

Copy link
Member

Choose a reason for hiding this comment

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

The rename is the more important of the two, I agree; BoundingBox is a pretty good name here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to BoundingBox

@nicoburns
Copy link
Collaborator Author

This turns out to have a nice compile time benefit for fontique (which people may well wish to use without using other parts of the linebender ecosystem - e.g. https://xi.zulipchat.com/#narrow/channel/296640-font-tech/topic/Refactoring.20fontique.20backend.20into.20separate.20crate).

peniko itself is quite lightweight, but it depends on color and kurbo which are both small-but-not-nothing.

@nicoburns nicoburns force-pushed the remove-kurbo-peniko branch 3 times, most recently from f76b2cf to 0285ce9 Compare September 30, 2025 10:54
Signed-off-by: Nico Burns <nico@nicoburns.com>
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

A few nits. Approval is conditional on them being applied

I would probably like to see a slightly expanded doc on bounding box (e.g. mentioning y-down space, talking about whether the values are logical pixels, etc.). But I'm not going to block on that.

I also think that this change does deserve a changelog entry, but again I'm not going to block on that.

Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
@nicoburns nicoburns added this pull request to the merge queue Sep 30, 2025
Merged via the queue into linebender:main with commit 5709830 Sep 30, 2025
24 checks passed
@nicoburns nicoburns deleted the remove-kurbo-peniko branch September 30, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants