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

New widgets #39

Merged
merged 12 commits into from
May 20, 2019
Merged

New widgets #39

merged 12 commits into from
May 20, 2019

Conversation

futurepaul
Copy link
Collaborator

Slider and progress bar feel pretty good. TextBox is still pretty much a hack.

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.

Looks good, I'll merge soon if other people don't have comments. Thanks!

@raphlinus raphlinus merged commit f0148cf into linebender:master May 20, 2019
@zaynetro
Copy link
Collaborator

I have noticed that space break the cursor. If you type space cursor doesn't move.

image

I haven't looked deep into the issue but I remember was having similar issues when trying to calculate the width of text.

@futurepaul
Copy link
Collaborator Author

Yeah, I'm getting this .width() measurement from this trait in Piet:

pub trait TextLayout {
    type Coord: Into<f64> + RoundFrom<f64>;

    /// Measure the advance width of the text.
    fn width(&self) -> Self::Coord;
}

In addition to fixing this, I believe we'll need more detail about the text layout to position the cursor within the string as well.

@raphlinus
Copy link
Contributor

Yes, at the very least the TextLayout needs a string offset to horiz position (to display the cursor) and a horiz position to string offset (for placing a caret on mouse click). I've held back adding these to piet because it opens up a lot of complexity.

  • The method should actually take at least a boolean indicating primary/secondary or trailing/leading, as there are two horiz positions in the case of a split cursor (which happens in BiDi). I'm reluctant to add an API which makes it impossible to get this right.

  • The DirectWrite implementation of horiz to offset rounds down (ie finds the caret to the left of the test point, at least for LTR), while I think it would be more useful to round-to-nearest. I think the correct implementation of this is to measure the horiz position of the next grapheme cluster boundary, test if it's closer.

  • The string offset should be in UTF-8 because that's Rust native, but most of the underlying implementations use UTF-16. Thus, TextLayout will need to retain the string (separately, in most cases) and do the conversion. I'm still wrestling with the question of whether it might make sense to expose UTF-16 in the case where the client can deal with it.

  • The web canvas implementation can't get this totally right, we have to fudge it by doing something like measuring substrings; this will work pretty well for simple scripts but give quite wrong answers sometimes for complex scripts such as Arabic.

  • Around this time might be a good opportunity to put in extremely simple line breaking. That raises some questions, notably whether the offset <-> horiz position methods take line breaks into account (so might give an x,y point rather than just a horiz pos).

I would like to get a "good enough" solution in place pretty soon, as this is clearly very important, but at the same time don't want to put stuff in that's broken by design and needs to be reworked later.

@futurepaul
Copy link
Collaborator Author

Would adding something similar to these "hit_test" methods from directwrite be a necessary first step either way?

@raphlinus
Copy link
Contributor

Yes, that code is a good start. I am open to the idea of adding something provisional to get stuff working, with the clear understanding that it will need to be reworked at some point.

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.

4 participants