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

Text/multiline #134

Merged
merged 85 commits into from
Apr 9, 2020
Merged

Text/multiline #134

merged 85 commits into from
Apr 9, 2020

Conversation

hwchen
Copy link
Collaborator

@hwchen hwchen commented Feb 10, 2020

Replaces #103 (which used dwrite and direct2d crates instead of raw_d2d).

Line breaking algorithm is naive. Iterates down text, checking width of each possible break.

  • implement new methods for multiline in `TextLayout'

    • update_width
    • line_text
    • line_metric
    • line_count
    • and fix d2d width to include trailing whitespace
  • update hit testing

    • account for multiline
    • if out of bounds top or bottom in y axis, will hit test closest line to boundary
    • all hit testing includes trailing whitespace
  • lots of tests

    • basic multiline and hit testing for d2d and cairo and web
    • new test harness (using wasm-pack) for web

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

A couple notes and questions about the API here. :)

piet/src/text.rs Outdated Show resolved Hide resolved
piet/src/text.rs Outdated
/// Does not include trailing whitespace.
pub line_end_offset: usize,

/// Line width.
Copy link
Member

Choose a reason for hiding this comment

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

What are the units? I would expect this to be in pixels or something but it's not clear.

I'm also unsure about this name, 'cumulative' implies that this includes the widths of preceding lines or something? If that's the case we should document it, but otherwise this can just be width?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm... this was in the last PR for line breaking, so I carried it over. But looking at it again I'm not sure exactly why we need cumulative width. Is it better to just use width?

Copy link
Member

Choose a reason for hiding this comment

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

I think we want just "width", in pixels. Also I think this comment was maybe in response to this?

Raph: I think it makes sense not to do cumulative width of lines, but it does make sense to do cumulative code unit counts. This is not a tricky thing, a "cumulative code unit count" is just an offset.

#134 (comment)

piet/src/text.rs Outdated
/// Does not include trailing whitespace.
pub cumulative_width: f64,

/// Length (in code units) of trailing whitespace.
Copy link
Member

Choose a reason for hiding this comment

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

if this is code units it should be an integer type?

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 made some mixup here; I think trailing whitespace should be in code units (usize)

piet/src/text.rs Outdated
fn line_text(&self, line_number: usize) -> Option<&str>;

/// Given a line number, return a reference to that line's metrics.
fn line_metric(&self, line_number: usize) -> Option<&LineMetric>;
Copy link
Member

Choose a reason for hiding this comment

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

if LineMetric doesn't contain anything expensive, this should return it by value, not by reference.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

I did a medium-depth pass. I found a few minor things, but overall I think we can merge this and then start trying to do the druid integration side; no doubt that will uncover whatever additional things might need to be fixed.

piet-cairo/src/text.rs Outdated Show resolved Hide resolved
piet/src/text.rs Show resolved Hide resolved
piet/src/text.rs Outdated Show resolved Hide resolved
piet-cairo/src/text.rs Outdated Show resolved Hide resolved
@hwchen
Copy link
Collaborator Author

hwchen commented Apr 2, 2020

Thanks for the quick turnaround. I'll get this ported to piet-web asap.

@hwchen hwchen marked this pull request as ready for review April 9, 2020 02:07
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I am of the opinion that we get this in and then see what issues shake out. 😄

@hwchen hwchen merged commit dff29a3 into linebender:master Apr 9, 2020
@hwchen
Copy link
Collaborator Author

hwchen commented Apr 9, 2020

Woohoo! I'm ready for the bug reports :)

@hwchen hwchen deleted the text/multiline branch April 10, 2020 16:32
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.

None yet

3 participants