Bugzilla – Full Text Bug Listing
| Summary: | StyledText hides last character of wrapped line | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> | ||||||
| Component: | SWT | Assignee: | Brian de Alwis <bsd> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | Niraj Modi <niraj.modi> | ||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | bsd, daniel_megert, Lars.Vogel, loskutov, lshanmug, niraj.modi, peter | ||||||
| Version: | 4.3 | ||||||||
| Target Milestone: | 4.6 M4 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows 7 | ||||||||
| See Also: |
https://git.eclipse.org/r/53437 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=5a79c3e65a59cceee29d76c03249fe0d5f9ed374 https://git.eclipse.org/r/60742 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=9e76f3ed668a6862d407d4ac586dcbb28cbaef59 |
||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 35779, 479980 | ||||||||
| Attachments: |
|
||||||||
Created attachment 255722 [details]
Sample test program
I did some digging into this and have a solution. I've attached the test program I used to reproduce Markus' example above.
The problem is in the win32-specific TextLayout in computeRuns(), specifically around line 335. This section handles where there is insufficent room on the line to include the next character run, but there is no obvious spot to break the line (i.e., a soft-break/line-break has not been found). The code needs to decide whether to split the current character run (at the 'o' in Markus' example above) or push the entire run to the next line.
The previous code would only push the run to the next line if there had been a previous run (firstIndice > 0) and that the previous runs fit exactly into the line (lineWidth == wrapWidth). Otherwise it would split the current run, and include at least one character of the run even if there was only 1 pixel of space.
The fixed code pushes the run to the next line if nothing fits (firstStart == 0) and there a previous run has already been included (lineWidth > 0 && firstIndice > 0).
} else if (start <= 0 && i == lineStart) {
- if (lineWidth == wrapWidth && firstIndice > 0) {
+ if (lineWidth > 0 && firstIndice > 0 && firstStart == 0) {
In my testing, the StyledText examples now behaves as it does on OS X.
New Gerrit change created: https://git.eclipse.org/r/53437 Can this be reviewed for M2? It blocks the popular Bug 35779 in platform UI. Gerrit change https://git.eclipse.org/r/53437 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=5a79c3e65a59cceee29d76c03249fe0d5f9ed374 Thanks for the patch, resolving the bug now. Re-opening.. fix for this bug introduced a freeze in TextLayout.computeRuns bug 479980, hence reverted this fix via below git commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b92b665169259a26da4e7566268a748473b777c4 Since this bug blocks most wanted "word wrapping" feature (see bug 35779 comment 234), increasing the priority and targeting to M4. Brian, what do you think, would you have time to look at this issue again in M4? (In reply to Andrey Loskutov from comment #7) > Since this bug blocks most wanted "word wrapping" feature (see bug 35779 > comment 234), increasing the priority and targeting to M4. +1. (In reply to Andrey Loskutov from comment #7) > Since this bug blocks most wanted "word wrapping" feature (see bug 35779 > comment 234), increasing the priority and targeting to M4. > Brian, what do you think, would you have time to look at this issue again in > M4? Hi Brian, Just a quick check, are you planning this bug for M4 ? (In reply to Niraj Modi from comment #9) > Just a quick check, are you planning this bug for M4 ? Yes! New Gerrit change created: https://git.eclipse.org/r/60742 Great test case from Markus on bug 479980. The reasoning behind my fix is correct, but the actual conditions tested weren't quite right. > The fixed code pushes the run to the next line if nothing fits (firstStart == 0) and there a previous run has already been included (lineWidth > 0 && firstIndice > 0). Markus' case is a bit pathological as the StyledText is initially wrapped with a line length of 1 pixel, and it adds a wrapIndent of 15 pixels. This meant every subsequent line starts with lineWidth == 15, and (wrongly) triggers the push-current-run-to-the-next-line case: if (lineWidth > 0 && firstIndice > 0 && firstStart == 0) { The correct test for this condition is if (firstStart == 0 && firstIndice > lineStart) { lineStart is the run index for the first run of the current line. firstIndice is the run index for the last run that could be completely fit into the current line with no wrapping. If firstIndice == lineStart, then no runs were able to be fit into the current line. The test of 'firstIndice > 0' is faulty and was present in the original code. It should be 'firstIndice > lineStart' as 'firstIndice > 0' will true for all lines > 1. I've revised my patch. (In reply to Brian de Alwis from comment #12) > Great test case from Markus on bug 479980. The reasoning behind my fix is > correct, but the actual conditions tested weren't quite right. > > > The fixed code pushes the run to the next line if nothing fits (firstStart == 0) and there a previous run has already been included (lineWidth > 0 && firstIndice > 0). > > Markus' case is a bit pathological as the StyledText is initially wrapped > with a line length of 1 pixel, and it adds a wrapIndent of 15 pixels. This > meant every subsequent line starts with lineWidth == 15, and (wrongly) > triggers the push-current-run-to-the-next-line case: > > if (lineWidth > 0 && firstIndice > 0 && firstStart == 0) { > > The correct test for this condition is > > if (firstStart == 0 && firstIndice > lineStart) { > > lineStart is the run index for the first run of the current line. > firstIndice is the run index for the last run that could be completely fit > into the current line with no wrapping. If firstIndice == lineStart, then > no runs were able to be fit into the current line. > > The test of 'firstIndice > 0' is faulty and was present in the original > code. It should be 'firstIndice > lineStart' as 'firstIndice > 0' will true > for all lines > 1. > > I've revised my patch. Revised patch looks good, works fine for the scenario in bug 479980 as well. Will test this further and planning to push it for coming I-Build. Gerrit change https://git.eclipse.org/r/60742 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=9e76f3ed668a6862d407d4ac586dcbb28cbaef59 (In reply to Eclipse Genie from comment #14) > Gerrit change https://git.eclipse.org/r/60742 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=9e76f3ed668a6862d407d4ac586dcbb28cbaef59 Thanks for the revised patch Brian, have verified this patch with Eclipse's Wrod-wrap option as well. Resolving this bug now. Verified the fix in I-Build: I20151207-2000 |
Created attachment 231388 [details] Screenshot I20130522-2000 StyledText completely hides the last character of a line when wrapping with certain fonts and widget widths. Unless the widget is very narrow, the last character of each line should always be rendered entirely (or moved to the next line if there's not enough space). The Text widget behaves as expected. The screenshot shows a case in the CustomControlExample where the "h" on the first line is completely invisible and the "w" on the second line is truncated. The text is: "Hello_world,_how_do_you_wrap_today?" Originally seen in the Variables view's detail pane when rendering string "file:/C:/e/w/runtime-head-CLEAN-2/org.eclipse.jdt.ui/about.html" with Lucida Console 8, and the "a" after the last slash was completely missing.