Support Text Wrapping #712
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stuartmscott
force-pushed
the
wrapping
branch
4 times, most recently
from
b331bab
to
b303d6e
Compare
|
Seeing sporadic failures: |
|
Thanks for your work on this - it's really promising. Many of the issues you see in test failures are to do with the old glfw3.2 on develop we have upgraded to 3.3. Work of this size should be based off develop anyway, so if you could please rebase your work then re-running the tests should be far more favourable. From what you have described it seems like the selection issues should be easily addressed - probably the new boundary code is not respected by cursor movement? The WrapLineLimit API that I proposed was not intended to limit the line length, but the number of lines that we wrap over (I guess like CSS "-webkit-line-clamp" - so we can code simply a "3 line preview" for example - like wrap turned on with the last line truncating). I wonder if TextWrapOn is a little ambiguous. Essentially we could have TextWrapWord or TextWrapBreak where it defines whether or not words should be broken - I think you have implemented the latter, but by calling it just "TextWrapOn" then adding the nuance could be a bit unclear. I hope that helps. I thought it would be good to chat through these points before doing an in-depth review of the PR. |
stuartmscott
force-pushed
the
wrapping
branch
from
b303d6e
to
e98f21b
Compare
|
No worries :-)
Perhaps it would be useful if I added the different wrapping modes to fyne_demo, both inside and without a scroller for devs to see how they behave for labels, hyperlinks, and entries? How about WrapMaxLines or WrapLineCountLimit? Yup I agree that splitting TextWrapOn into multiple types makes it clearer. Right now it implements TextWrapBreak, and I could take a swing at TextWrapWord, but I think there should also be TextWrapHypenate which behaves like TextWrapBreak but will add a '-' to the end of a word broken by the wrapping. I purposely did not implement this in this PR as it is a lot of work to do well - you cant just hyphenate words at any old place; Each word has places where it is acceptable to hyphenate and others where it is not, and this varies between publishing house, dialects, enUK/enUS, and that's without thinking about internationalization. Suffice to say it is a tough problem and I think it makes sense to leave it out of this PR and we can revisit later (probably after i18n support). |
stuartmscott
force-pushed
the
wrapping
branch
3 times, most recently
from
a384dbe
to
a5047cf
Compare
|
Re: tests - yes you are right. That canvas test is flakey and needs improved. The "_running count" seems to be something strange in the latest macOS that only manifests in the test suite, not yet tracked down. |
|
Thanks for the work on this, your thoughts all seem to be going in the right direction. WrapMaxLines seems clearer than the longer form to me. I agree that adding examples to fyne_demo would be really helpful - maybe on the "Buttons" tab (as it has Label) and possibly it should have a clearer name anyhow. I'm happy to consider merging this in with just TextWrapBreak, then add TextWrapWord a little later if it helps. Also happy about not writing TextWrapHyphenate any time soon ;). However I do wonder if we should have the ellipsis on by default for truncate, it seems clearer - if required in the future there could always be a mode with no added character? indicating that something has been truncated is likely more useful than hiding that fact. Sorry if this isn't what I said before! |
stuartmscott
force-pushed
the
wrapping
branch
2 times, most recently
from
eaa7c2d
to
45182c0
Compare
|
I agree that ellipsis are a good idea as they make it obvious text has been truncated, however it will be a bigger change (see below). Would it be better to remove Truncation from this PR and implement it properly later? The current implementation uses "t.rowBounds [][2]int" to hold the start and end indicies of each line of text in "t.buffer", however this is not sufficient to hold all the information about the line such as whether it was truncated and should have ellipsis added. It doesn't make sense to insert the ellipsis into "t.buffer" as this at the model level and ellipsis are at the presentation level. To implement truncation properly I would change "t.rowBounds [][2]int" to "t.lines []*line" where line is; This gives the presentation layer a place to store the text to present that is separate from the model storage, and gives the flexibility to add a suffix for truncation '...', or hyphenation '-'. It also affords some future projects; I also noticed that Fyne doesn't support a text alignment of "justified". This is another part of typesetting that is difficult to do well, but it is something I have recently implemented as part of https://github.com/AletheiaWareLLC/pdfgo which is a PDF generator written in Go. It supports;
The reason there are three types of Justification is all about how the last sentence in a paragraph is displayed - you don't want to stretch the sentence to full justification as this will be unreadable. Instead the bulk of the paragraph is fully justified and the last paragraph is either left, center, or right aligned. The way I implemented this was with;
With this information all the text alignments above can be implemented, eg. right alignment just means that the indent is set to widget.width - text.width, center alignment just means that the indent is set to half widget.width - text.width. Justification uses a combination of spacing and indentation to balance the line in the widget. Implementing the above would negate the need for canvas/text.go and internal/painter/gl/draw.go to know about alignment. internal/painter/gl/gl_common.go newGlTextTexture would advance the dot by the indent and, for each character, the relevant spacing. Obviously this is a much bigger change and would also need to be mirrored to mobile, thus way outside the scope of this PR, but it is worth thinking about now. I think it would be better to implement truncation properly in a later PR rather than some quick hack in this one. |
|
These are good observations, though I would be very careful before extending the bounds code - that is model level, but the wrap and truncate is render level (as you said). So perhaps the new info is in the renderer matching the indexes of the bounds slice? (and you're right there is no way that we should insert ellipses). But as the output is a list of canvas Text objects then it's a case of whether or not we append one in the render output so it shouldn't have to touch the model. Feel free to add an issue for the missing "Justified" alignment - but are you sure all 3 types are needed? most places really only have 1. I appreciate that it could remove some code from the driver, but we need to consider speed here as well and having a whole line rendered in 1 go is a lot faster than rendering 1 character at a time (I think). let's discuss this in another issue though. |
|
One other observation on the code as is - that's a lot of new public APIs at the top level. |
|
It is possible to add ellipsis (truncate) and hyphens (wrap w/ break) in the canvas Text objects but then they will need to know whether their text has been truncated or wrapped w/ break so either way "rowBounds [2]int" is insufficient.
Looking at the code that renders the line (https://github.com/golang/image/blob/58c23975cae11f062d4b3b0c143fe248faac195d/font/font.go#L165) it just loops through each character in the line anyway, so moving this loop into fyne would give us the flexibility and control without compromising the performance. I'll leave justification for now, it can be implemented in a future PR. This PR now includes an addition to fyne_demo which showcases text alignment and wrapping, could you take a look and let me know what you think? There is still a bug here because Entry was hardcoded to TextAlignLeading and the code handling Cursor assumes the line always starts at theme.Padding(). So if you change the Entry's alignment to "Center" or "Trailing" and try to place the Cursor you will see some weird behaviour. |
stuartmscott
force-pushed
the
wrapping
branch
2 times, most recently
from
4ddcadb
to
58a41b2
Compare
|
Seeing some test failures, gomobile/canvas_test.go TestCanvas_TappedSecondary is failing, the rest appear flakey; |
|
Intermittent failures due to 'NSApp with wrong _running count' are a new issue with Catalina - not yet figured out the cause or the fix. |
canvas.Text is a single string of runes painted - so I'm not sure that hyphenating automatically makes sense as there is an implied second line in that instance. I suggested that ellipsis could be put there because it could adjust based on the object size and does not imply any overflow. Therefore it could be switched on and the result forgotten - if used carefully.
By rendering I meant the OpenGL texture code. Internally it may paint characters one at a time but this is passed to the graphics card, and cached appropriately, as one texture.
I don't think this is necessary to expose alignment in Entry at this stage - it's the wrapping that this is all about :) |
Fixes fyne-io#332 Applies to Label, Hyperlink, and Entry There are three text wrapping modes; - TextWrapOff (default) - Widgets are expanded to fit the text. - Consistent with previous behaviour. - TextTruncate - Text longer than the widget's width is not displayed. - An ellipsis is NOT added if text gets truncated, although this should be considered for future (perhaps TextTruncateEllipsis). - TextWrapBreak - Text longer than the widget's width is wrapped onto the next line. - A hyphen is NOT added if a word gets broken, although this should be considered for future (perhaps TextWrapOnHypenate). - TextWrapWord - Text longer than the widget's width is wrapped onto the next line. - Text is broken on word boundaries (spaces) to avoid breaking words. - Text without word boundaries is broken the same as TextWrapBreak. Added new tab to demo showcasing different wrapping modes.
|
The latest Travis run (https://travis-ci.org/github/fyne-io/fyne/builds/665231013) failed on OSX with;
How would canvas.Text know whether to add ellipsis or not? When MinSize is called it return the minimum of the whole string of runes, are you suggesting that canvas.Text will add ellipsis if Resize is called with size.Width less than MinSize.Width? The approach I would take is to keep canvas.Text simple - it just takes the string, size, style, x, y and renderers the text. glPainter.drawText should not be calculating the dot. The layers above, eg. textRenderer or Entry, are responsible for chopping up the text into lines and aligning them. The canvas.Texts are cached by the textRenderer, but these get their text updated and are redraw each time the text changes.
We should cache the texture, I'm talking about what happens before that, where the texture is generated and Drawer.drawString is called to render each character, instead we move that loop into glPainter.newGlTextTexture so that we iterate each character and advance the dot to space the words and characters to achieve proper text justification (not part of this PR, but a future one).
Agreed, removed from this PR. |
stuartmscott
commented
Mar 26, 2020
text.go
Outdated
|
|
||
| // SplitLines accepts a slice of runes and returns a slice containing the | ||
| // start and end indicies of each line delimited by the newline character. | ||
| func SplitLines(text []rune) [][2]int { |
There was a problem hiding this comment.
Ack, I'll unexport it - its signature will change in future anyway with the rest of the text improvements.
text.go
Outdated
|
|
||
| // LineBounds accepts a slice of runes, a wrapping mode, a maximum line width and a function to measure line width. | ||
| // LineBounds returns a slice containing the start and end indicies of each line with the given wrapping applied. | ||
| func LineBounds(text []rune, wrap TextWrap, maxWidth int, measurer func([]rune) int) [][2]int { |
There was a problem hiding this comment.
Ack, will simplify.
text.go
Outdated
| lines := SplitLines(text) | ||
| switch wrap { | ||
| case TextTruncate: | ||
| if maxWidth > 0 { |
text.go
Outdated
| for _, l := range lines { | ||
| low := l[0] | ||
| high := l[1] | ||
| if low == high { |
There was a problem hiding this comment.
Sure, and that pattern can be applied to the other cases too.
widget/label_test.go
Outdated
| @@ -21,15 +21,15 @@ func TestLabel_MinSize(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestLabel_Text(t *testing.T) { | |||
| label := &Label{Text: "Test"} | |||
There was a problem hiding this comment.
Andy, supporting both NewX() and &X{} is great, the problem is with "ExtendBaseWidget". If you support &X{} then basically all "func (x *X)" need to call "x.ExtendBaseWidget(x)" which is a code smell.
I'll revert these back to &X{} for this PR.
widget/hyperlink_test.go
Outdated
| @@ -24,15 +24,16 @@ func TestHyperlink_MinSize(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestHyperlink_Alignment(t *testing.T) { | |||
| hyperlink := &Hyperlink{Text: "Test", Alignment: fyne.TextAlignTrailing} | |||
widget/entry_test.go
Outdated
| @@ -180,7 +179,7 @@ func TestEntry_OnKeyDown_Insert(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestEntry_OnKeyDown_Newline(t *testing.T) { | |||
| entry := &Entry{MultiLine: true} | |||
andydotxyz
requested changes
Mar 27, 2020
andydotxyz
previously approved these changes
Mar 27, 2020
|
Travis approves, honest: https://travis-ci.org/github/fyne-io/fyne/builds/667814891 |
|
Before I merge I wanted to check, did you feel that you wanted this to be squashed? |
|
Thanks for asking. Now that the scroller changes are in, I am going to merge 'develop' into 'wrapping' and use the new widget.NewVerticalScrollContainer for the Lorem Ipsum Multiline Entry. Once that is pushed (and reviewed) we can either squash it into one (most of the info is in the first commit anyway), or merge all of them. I have a minor preference toward the former just to keep git history neat. |
|
Nice idea thanks. I have run it locally and (performance aside) seems pretty slick. If you turn off wrap (or use truncate) the size of the window becomes absolutely huge... Is it worth using slightly shorter lorem lines so that it can be sized back down if people want? Also perhaps the multiline entry could be expanded by the layout - for example: fixedText := widget.NewVBox(
widget.NewHBox(
radioAlign,
layout.NewSpacer(),
radioWrap,
),
label,
hyperlink,
entry,
entryDisabled,
entryMultiLine,
)
return fyne.NewContainerWithLayout(layout.NewBorderLayout(fixedText, nil, nil, nil),
fixedText, entryLoremIpsumScroller) |
|
You're welcome! Do you see any obvious ways to improve the performance issues from the code? Splitting the text and wrapping it is quite a chunk of work - lots of text measure calls and slice operations, but not that many memory allocations - but perhaps it is getting called too many times unnecessarily. Lol yeah sure I'll shorten them. Sure, although it still feels hacky to use a borderlayout to force content to take up all the space. |
Layouts control the size and position of objects in their containers. Some layouts expand objects, others collapse them - most do a bit of both. I'm not sure what is hacky? |
In the main codepaths we cache calculations that involve text measurement (by caching the result of MinSize(). Anything that can be done to measure things less is probably a good plan. I'll look into the code later on and see if I have more concrete suggestions. |
I just mean that a borderlayout is all about a centerpiece with other cardinal pieces, like an editor and toolbars/sidebars. But in this case it is a vertical stack of elements and so it feels like using a hammer as a screwdriver.
Cool thanks! Caching the text measurements is a good idea, but before that we should write some benchmark tests to make sure our changes are improving the performance. |
Yes I see your point. Maybe we lack a layout with the type of flexiility that folk want (i.e. #4)? |
|
Yeah maybe, anyway borderlayout is fyne for this demo :P |
andydotxyz
approved these changes
Mar 31, 2020
|
I'm happy with this and think it's good to merge, thanks. Let's work on performance though |
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support Text Wrapping
Fixes #332
Applies to Label, Hyperlink, and Entry
There are three text wrapping modes;
should be considered for future.
should be considered for future (perhaps TextWrapOnHypenate).
This commit has some issues and is therefore not yet ready to merge;