Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 360724

Summary: Maximum line width is often wrong
Product: [ECD] Orion Reporter: Mihai Sucan <mihai.sucan>
Component: EditorAssignee: Felipe Heidrich <eclipse.felipe>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: eclipse.felipe, mihai.sucan, Silenio_Quarti
Version: unspecified   
Target Milestone: 0.4 M1   
Hardware: PC   
OS: Linux   
Whiteboard:

Description Mihai Sucan CLA 2011-10-12 16:03:58 EDT 
STR:

1. Load http://orion.eclipse.org/examples/textview/demo.html
2. Pick "JavaScript file".
3. Move caret to line 30.
4. Press the End key to go to line end.
5. Press the Home key to go to line start.
4. Note the scrollbar width and press Page down to scroll one page down.

Expected result: scrollbar width stays the same.

Actual result: scrollbar width changes (less code to scroll now!).

Press page up again to see the scrollbar width change again.

Problem explanation: TextView _updatePage() calculates the maximum line width as lines are rendered. See, for example, the Java file demo: initial load has no scrollbar, but as you scroll, the scrollbar width changes.

The _redrawLines() method resets the maximum line width in some well-defined cases, but the given steps above cause the redraw and the max width reset. This reset should be avoided as much as possible.

This bug affects bug 360672 in an interesting way: _doPageDown/Up() call _scrollView() to make the scrolled line and caret visible. The caret X offset is calculated and both of the correct relative coordinates (x, y) are given to the _scrollView() method. However, when _viewDiv.style.width is wrong, the scrollLeft update will be clamped - the call to _scrollView() happens before the view is updated to the new size. So, _scrollView() fails to scroll vertically to the desired pixel location and the caret is not always made visible when Page Down / Up is used.

I reported this as a separate bug because I assume this is something with wider impact within Orion's text view, and that this problem is no longer strictly related to bug 360672.

One possible solution: within the TextModel track the line with the highest number of characters. Since we already calculate the line height, we can also determine the char width. Using the highestLineCharCount * charWidth we can always keep _viewDiv.style.width up-to-date. This would also make the code faster - _updatePage() doesn't need to check the bounding client rects for the drawn lines, no tracking of the widest line, etc.

Thoughts?
Comment 2 Mihai Sucan CLA 2011-10-14 05:28:00 EDT 
Thanks for the quick fix! Silenio's solution is less radical. I like that. :)
Comment 3 Felipe Heidrich CLA 2011-10-14 09:39:53 EDT 
(In reply to comment #2)
> Thanks for the quick fix! Silenio's solution is less radical. I like that. :)

Basically the same solution we used in StyledText (SWT).

Note that using line char count to measure a line has several problems:
- fails for multi font
- fails for proportional font
- fails for international text (specially bad for CJK)
- fails for tab expansions
Comment 4 Mihai Sucan CLA 2011-10-14 10:38:21 EDT 
(In reply to comment #3)
> (In reply to comment #2)
> > Thanks for the quick fix! Silenio's solution is less radical. I like that. :)
> 
> Basically the same solution we used in StyledText (SWT).
> 
> Note that using line char count to measure a line has several problems:
> - fails for multi font
> - fails for proportional font
> - fails for international text (specially bad for CJK)
> - fails for tab expansions

I did think of tab expansions, and that's something (hopefully) easy to fix.

Is TextView supposed to work with multiple fonts and proportional fonts? (in the future)
Comment 5 Felipe Heidrich CLA 2011-10-17 09:39:26 EDT 
(In reply to comment #4)
> Is TextView supposed to work with multiple fonts and proportional fonts? (in
> the future)

proportional fonts:
Yes, that said, we never tested it much.
Note that this is important for unicode text. We can't expected JP characters to have the same width as EN characters.


multiple fonts:
Yes, as long as the fonts all have the same height (in other words, all lines have the same height - if you set a font to a line that makes it taller than the other lines it will fail).
We tested for regular/italic/bold/bold-italic.
We should have this fully working when Bug 334910 is fixed.