Bug 400727 - StyledText#addBidiSegmentListener(..) only works if RTL input language is installed
Summary: StyledText#addBidiSegmentListener(..) only works if RTL input language is ins...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.3   Edit
Hardware: PC Windows 7
: P3 major (vote)
Target Milestone: 4.3 M6   Edit
Assignee: Markus Keller CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 400662
  Show dependency tree
 
Reported: 2013-02-13 14:21 EST by Markus Keller CLA
Modified: 2013-03-08 13:30 EST (History)
6 users (show)

See Also:

Attachments
Fix (2.72 KB, patch)
2013-03-08 08:34 EST, Markus Keller CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2013-02-13 14:21:38 EST 
On Windows, StyledText#addBidiSegmentListener(..) only works if an RTL input language is installed in the OS.

This limitation is not documented there, and it is unexpected. Javadocs of BidiSegmentEvent and BidiSegmentListener#lineGetSegments(BidiSegmentEvent) mention "in a bidi locale", but that is not the correct precondition, and is easy to miss (especially on non-Windows platforms, where bidi-processing is always enabled, see StyledText#isBidi()).

Furthermore, org.eclipse.swt.internal.BidiUtil.isBidiPlatform() caches the value, so changes in the OS are not reflected until the application is restarted.

This hits us in bug 400662 (and in debug's ProcessPropertyPage).

I think the best solution is to remove StyledText#isBidi() and always process BidiSegmentListeners. Eclipse workbench plug-ins that want to reduce bidi-processing overhead should rely on the new command line flag proposed in bug 383185:

 -bidi "on=[y/n];textDir=[ltr/rtl/auto]"
Comment 1 Markus Keller CLA 2013-03-08 08:34:26 EST 
Created attachment 228136 [details]
Fix

Can we fix this for M6? I can release the fix unless there's opposition.
Comment 2 Silenio Quarti CLA 2013-03-08 11:01:44 EST 
My only concern is whether this will case performance degradation on Windows. Do you have any numbers?

The command line switch will only help eclipse plugins. I am not a big fun of system properties, but we could add this line to the beginning of isBidi():

if (!"true".equals(System.getProperty("org.eclipse.swt.checkBidiLocale"))) return true;
Comment 3 Markus Keller CLA 2013-03-08 12:04:32 EST 
It will cause more BidiSegmentListener callbacks, but it can't be a relevant performance degradation, since we already accept this unconditionally on Mac and GTK, and on Windows platforms where a bidi keyboard language is installed.

In the Eclipse SDK, the only relevant BidiSegmentListener is in the JavaSourceViewer. This is called every time a line is rendered in a Java editor. When I scroll 1000 lines of StyledText (takes about 30s here), both YourKit and System.currentTimeMillis() have a hard time measuring any time spent in StyledText#getBidiSegments(..). Often, it's 0ms, sometimes 1ms. Accumulated over the 30s, it's around 30ms.

I think we should just remove this and we don't need a system property.
Comment 4 Silenio Quarti CLA 2013-03-08 13:21:59 EST 
OK, +1 to your patch.