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

Bug 312346

Summary:

StyledText doesn't report a text style when requesting the end of the line.

Product:

[Eclipse Project] Platform

Reporter:

Scott Kovatch <skovatch>

Component:

SWT

Assignee:

Scott Kovatch <skovatch>

Status:

RESOLVED FIXED

QA Contact:

Carolyn MacLeod <carolynmacleod4>

Severity:

normal

Priority:

CC:

eclipse.felipe

Version:

3.6

Flags:

eclipse.felipe: review+
carolynmacleod4: review+

Target Milestone:

3.6 RC2

Hardware:

Macintosh

OS:

Mac OS X

Whiteboard:

Attachments:

Description	Flags
Fix	none
Better fix	none
new patch	none
new patch (2)	none

Description Scott Kovatch

2010-05-10 17:56:45 EDT

In StyledText's implementation of getTextAttributes() at line 6712 (roughly) it's possible for the method to return without returning a TextStyle object in e.textStyle. On the Mac this causes VoiceOver to interpret a line ending as a font change because the character before and character after have font information returned for them.

Fix is simple -- just add 

e.textStyle = new TextStyle(st.getFont(), st.foreground, st.background);

before the return on line 6715.

Comment 1 Scott Kovatch

2010-05-10 17:58:38 EDT

Created attachment 167834 [details]
Fix

Comment 2 Scott Kovatch

2010-05-10 17:59:22 EDT

Felipe, let me know what you think.

Comment 3 Scott Kovatch

2010-05-10 17:59:51 EDT

Car, you should make sure this is okay, too.

Comment 4 Felipe Heidrich

2010-05-11 09:23:31 EDT

That block of code runs when the offset points to the line delimiter.
Not sure if you should return the default font or the font of the preceding character.

What happens when the last word in the line is bold, how does your patch perform in this case ?

Comment 5 Scott Kovatch

2010-05-11 12:43:43 EDT

(In reply to comment #4)
> That block of code runs when the offset points to the line delimiter.
> Not sure if you should return the default font or the font of the preceding
> character.
> 
> What happens when the last word in the line is bold, how does your patch
> perform in this case ?

Ah, good catch. In CustomControlExample, if I select "dog.<newline>One" it tells me that the newline is plain, but the rest of the text is bold.

Getting the style of the last character works as you suggested. I'll add the patch.

Comment 6 Scott Kovatch

2010-05-11 12:44:05 EDT

Created attachment 167967 [details]
Better fix

Comment 7 Carolyn MacLeod

2010-05-12 16:47:53 EDT

I am going to leave this one up to Felipe, but yes, the styles seem to work as expected (except for italic?)

Comment 8 Felipe Heidrich

2010-05-17 16:40:10 EDT

Created attachment 168822 [details]
new patch

Your patch decrements offset without any test, I'm afraid you can get a -1 and layout.getStyle() will throw an expection (the case to my mind is empty line "\n").

Please try this patch, it clamps offset to range, so it should return the style of preceeding characters.

Comment 9 Scott Kovatch

2010-05-17 17:03:26 EDT

(In reply to comment #8)
> Created an attachment (id=168822) [details]
> new patch
> 
> Your patch decrements offset without any test, I'm afraid you can get a -1 and
> layout.getStyle() will throw an expection (the case to my mind is empty line
> "\n").
> 
> Please try this patch, it clamps offset to range, so it should return the style
> of preceeding characters.

Oops -- this patch still triggers an index out of bounds at

e.textStyle = layout.getStyle(Math.max(0, Math.min(offset, lineLength - 1)));

	at org.eclipse.swt.graphics.TextLayout.getStyle(TextLayout.java:1412)
	at org.eclipse.swt.custom.StyledText$13.getTextAttributes(StyledText.java:6708)
	at org.eclipse.swt.accessibility.Accessible.getAttributedStringForRangeParameterizedAttribute(Accessible.java:1520)
	at org.eclipse.swt.accessibility.Accessible.internal_accessibilityAttributeValue_forParameter(Accessible.java:1198)
	at org.eclipse.swt.widgets.Control.accessibilityAttributeValue_forParameter(Control.java:252)

in the single blank line case you describe above. This clamps to 0, but TextLayout is throwing an exception because 0 <= 0 < 0 is false.

Comment 10 Scott Kovatch

2010-05-17 17:05:35 EDT

(Not that mine was any better -- same exception.)

Comment 11 Felipe Heidrich

2010-05-18 09:21:18 EDT

(In reply to comment #10)
> (Not that mine was any better -- same exception.)

Okay, all we need is a lineLength > 0 before making the call.
sorry, I didn't test the code.

Comment 12 Felipe Heidrich

2010-05-18 09:27:33 EDT

Created attachment 168925 [details]
new patch (2)

Is the code right now ?

Comment 13 Scott Kovatch

2010-05-18 14:18:34 EDT

(In reply to comment #12)
> Created an attachment (id=168925) [details]
> new patch (2)
> 
> Is the code right now ?

Interesting.... I thought I tried to make this exact change yesterday but wasn't getting the right transition when there are multiple blank lines. This morning it's okay. +1 for the last patch.

Comment 14 Felipe Heidrich

2010-05-18 14:34:29 EDT

We still need an extra +1 before we release this. 

Car, please review the patch in comment 12. Thank you.

Comment 15 Carolyn MacLeod

2010-05-19 15:56:09 EDT

I looked at this with SSQ, and in order for this patch to work, the patch in bug 313444 also needs to be installed. So, please have a look at SSQ's comment 3 in bug 313444 first, and if there's any changes, those need to be reviewed/tested first, and then both will have to go in together.

Comment 16 Carolyn MacLeod

2010-05-19 16:30:18 EDT

OK - this is good. I am going to +1 bug 313444 is a second. They both should go in together.

Comment 17 Scott Kovatch

2010-05-19 18:07:01 EDT

Fixed > 20100519.