Bug 273198 - [Bidi] Lack of support for controlling text direction independently from widget orientation
Summary: [Bidi] Lack of support for controlling text direction independently from widg...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.5   Edit
Hardware: PC Windows 2000
: P3 normal (vote)
Target Milestone: 4.3 M6   Edit
Assignee: Silenio Quarti CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 307307
Blocks: 273722 308847 316267 325994 402256
  Show dependency tree
 
Reported: 2009-04-22 03:09 EDT by Tomer Mahlin CLA
Modified: 2013-03-14 11:04 EDT (History)
14 users (show)

See Also:


Attachments
Initial version of a win32 patch (18.35 KB, patch)
2012-12-05 11:54 EST, Lina Kemmel CLA
no flags Details | Diff
Modified controlexample (17.31 KB, patch)
2012-12-10 11:06 EST, Lina Kemmel CLA
no flags Details | Diff
Patch updated per comment 11 (24.44 KB, patch)
2013-03-13 12:56 EDT, Lina Kemmel CLA
no flags Details | Diff
Patch updated per comment 11 (24.46 KB, patch)
2013-03-13 13:08 EDT, Lina Kemmel CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomer Mahlin CLA 2009-04-22 03:09:43 EDT
Background
-----------
 Natural direction of text in Bidi languages is RTL. This is true even if Bidi sentence includes words in English. After all, this is the reason why those languages are called BI-directional. When sentence in Bidi language is displayed with not natural to it direction (i.e. LTR) it becomes incomprehensible. Here is an example: HELLO WORLD man !
   LTR: DLROW OLLEH man !
   RTL: ! man DLROW OLLEH

On the other hand, natural direction of English sentence (even if it includes several words in Bidi language) is LTR. When it is displayed with not natural to it direction it becomes incomprehensible. 
Here is an example: my name is TOMER.
    LTR: my name is REMOT.
    RTL: .REMOT my name is

How it currently works
-----------------------
  By default text direction is inherited from widget orientation. For not mirrored widget the direction of text is LTR, while for mirrored widgets, the direction of text is RTL. Widget orientation is inherited from higher level container orientation up to the top most container. Consequently, not mirrored GUI provides optimal display for English text (since not mirrored GUI supports by default LTR text direction which is a natural text direction for English text), while mirrored one, provides optimal display for Bidi text(since mirrored GUI supports by default RTL text direction which is a natural text direction for Bidi text).
  However, quite often Bidi text is used / shown on the not mirrored GUI and English text is used/shown on the mirrored one.

What is the problem
--------------------
  Unfortunately, SWT widgets don't allow control over text direction independently from GUI orientation. Specifically:
    1. It is not possible to set text direction on the widget level 
       independently from widget orientation.
    2. For complex widgets (or/and containers) it is not possible to propagate 
       text direction property to all children down the chain independently 
       from orientation.
 Thus, display of Bidi text in the not mirrored GUI and English text in the mirrored one become incomprehensible.

How is this issue different from complex expressions issues
-----------------------------------------------------------
  The main difference between this issue and the complex expression issues is that in this case the focus is on the plain text which does not have any internal structure, while in case of complex expressions, the text has well defined syntax (i.e. file path, SQL query etc.).

Contexts of interest
--------------------
  This issue is of special interest in the context of sentences formulated in natural language. Those sentences can appear in numerous places. Here are several examples:
  a. Description - almost all tools create entities having short/long descriptions
  b. Text editors including rich text editors - different paragraphs may include text in different languages.
  c. Code editors allowing providing comments - Description of a function in Java code editor.
  d. Chatting tools - different people can use different language to construct their messages.
  e. Java doc viewer.
  f. Bug monitoring tools (i.e. Jazz - Work Items) - different people can provide their comment to a defect using different languages.
...

Observations
-------------
  There is no one single widget (i.e. StyleText) which is always used for display of sentences formulated in natural language. Consequently the issue should be addressed on ideally all SWT widget by allowing control of text direction independently from widget orientation. Once this support is provided 
by SWT widgets, specific components (several examples were provided in the previous section), will be able to leverage it to resolve specific problems locally.
Comment 1 Felipe Heidrich CLA 2009-04-22 10:15:15 EDT
You can solve this problem using bidi control characters, that is what win32 programmer do. It is guarantee to work on all platforms.

So in a way, this is a case (maybe the simplest) of the complex expressions problem.

The problem: how to override default bidi reordering
The solution: bidi control characters
Comment 2 Tomer Mahlin CLA 2009-04-23 13:57:18 EDT
 Indeed, in general, Unicode control characters can offer some help in resolution of the problem. However there are following (I would say usability issues) with suggested approach:

1. Not all contexts/widgets in which text is edited have built in support for adding control characters. StyledText is a classical example. The standard context menu which exist for platform input field does not exist in it (this is a known problem, see bug 73685). In such contexts the only way to add control characters is copy/paste them from external source. 

2. Manual addition of Unicode control characters is troublesome and can easily make editor's work a nightmare. To make it even more problematic, consider that in some editors (i.e. StyleText based editors) it is impossible to make Unicode characters visible (as opposed to platform based input fields). Consequently it is easy to forget where in text you placed them and thus it is easy to remove them during the editing. The results or inadvertent removal of Unicode control character might be very unexpected from the editing/display perspective. It is one thing when editor injects Unicode control characters automatically (i.e. Java code editor leveraging support of BidiSegmentListener), but it is quite another thing to do it yourself. 

3. Not all software programs address the problem of Bidi text display the same way. Consider if text edited in one program is transferred to another one. If the approach for providing Bidi support is not the same on the both ends, the results may be unexpected. Namely, the text will not be displayed the same way in both programs. A classical example is display of Bidi text on Windows and on Linux. While on Windows the direction of text is based on GUI orientation and can be either LTR (for not mirrored GUI) and RTL (for the mirrored one), on Linux-GTK a contextual text direction is supported (the direction of text is derived from the text content only and is independent from GUI orientation). So, while on Windows for not mirrored GUI it is required to add control characters to enforce RTL text direction for Bidi text, on Linux it is not required.
Comment 3 Lina Kemmel CLA 2010-07-08 07:31:51 EDT
I believe this bug can be covered by bug 241482 and bug 230854 (provided that Segments support is introduced for all text-enabled widgets, including static ones).

To accomplish the required behavior, application would do the following:
  1. Indicate segment offsets in positions {0, text_length},
  2. Indicate appropriate segment chars:
    2.1. To achive LTR base text direction: { LRE, PDF }
    2.2. To achive RTL base text direction: { LRE, PDF }

Of course those can be combined with additional segments (e.g. required for proper display of complex expressions).
Comment 4 Lina Kemmel CLA 2010-07-08 07:36:00 EDT
There was a typo in my previous comment, posting correction.

To accomplish the required base text direction, application will be able to do the following:
  1. Indicate segment offsets in positions {0, text_length},
  2. Indicate appropriate segment chars:
    2.1. To achieve LTR base text direction: { LRE, PDF }
    2.2. To achieve RTL base text direction: { RLE, PDF }
Comment 5 Tomer Mahlin CLA 2010-07-08 07:42:46 EDT
  We can easily achieve Contextual base text direction the same way. The only addition is to inspect the content of the text in BidiSegmentListener and identify first character with strong directionality. If it is LTR character, enforce LTR, if it is RTL enforce RTL. 
  This means that suggested approach covers all possible values for base text direction. 

PS. In the original description I forgot to mention that following values of base text direction should be supported:
  LTR - Left to right (natural for Latin languages)
  RTL - Right to left (natural for Bidi languages)
  Contextual-LTR - Contextual base text direction which defaults to LTR in case no characters with strong directionality is found.
  Contextual-RTL - Contextual base text direction which defaults to RTL in case no characters with strong directionality is found.
Comment 6 Tomer Mahlin CLA 2010-07-08 09:35:05 EDT
Here is the code supporting identification of base text direction based on 
user preference (LTR or RTL or Contextual-LTR or Contextual-RTL) and text content. 

Input: text, user preference
Output: LTR or RTL text direction 

// str - holds the input string for which direction is computed
// txtDirPref - can be either CONTEXTUAL_LTR, CONTEXTUAL_RTL, LTR or RTL

String computeTxtDir(String str, String txtDirPref) {

       String dir = "";
       if (txtDirPref.equals("LTR))
          return "LTR";
       if (txtDirPref.equals("RTL"))
          return "RTL";

       if (str == null) {
	   if (txtDirPref.equals("CONTEXTUAL_LTR"))
		return "LTR";
	   else if (txtDirPref.equals("CONTEXTUAL_RTL"))
		return "RTL";			
        }

	for (int i = 0; i < str.length(); i++) {
		byte directionality = Character.getDirectionality(str.charAt(i));
		if (directionality == Character.DIRECTIONALITY_RIGHT_TO_LEFT
				|| directionality == Character.DIRECTIONALITY_RIGHT_TO_LEFT_ARABIC) {
			dir = "RTL";
			break;
		} else if (directionality == Character.DIRECTIONALITY_LEFT_TO_RIGHT) {
			dir = "LTR";
			break;
		}
	}
     
        // case in which no character with strong directionality is present in the string
	if (dir.length() == 0) {
		if (txtDirPref.equals("CONTEXTUAL_LTR"))
			dir = "LTR";
		else if (txtDirPref.equals("CONTEXTUAL_RTL"))
			dir = "RTL";
	}
        
        return dir;		
}
Comment 7 Lina Kemmel CLA 2012-11-11 08:47:34 EST
I am currently finalizing a win32 patch.

Base text direction (BTD) for most widgets will be set through manipulating the OS.WS_EX_RTLREADING style bit.

Note that this style is not treated by Windows literally (as indicating explicit RTL direction), but causes the BTD to get changed to that mismatching the natural BTD for the current layout (controlled in turn by the OS.WS_EX_LAYOUTRTL flag). I.e.:

  - (OS.WS_EX_LAYOUTRTL | OS.WS_EX_RTLREADING = 0) => BTD = LTR
  - (OS.WS_EX_RTLREADING)                          => BTD = RTL
  - (OS.WS_EX_LAYOUTRTL)                           => BTD = RTL
  - (OS.WS_EX_LAYOUTRTL | OS.WS_EX_RTLREADING)     => BTD = LTR

API changes:
------------

1. At Control level:

/**
 * Sets the base text direction (a.k.a. "paragraph direction") of the receiver,
 * which must be one of the constants <code>SWT.LEFT_TO_RIGHT</code> or
 * <code>SWT.RIGHT_TO_LEFT</code>.
 * <p>
 * <code>setOrientation</code> would override this value with the text direction
 * that is consistent with the new orientation
 *
 * @param textDirection New base text direction style
 * 
 * @return Whether or not the direction was updated
 *  
 * @exception SWTException <ul>
 *    <li>ERROR_WIDGET_DISPOSED - if the receiver has been disposed</li>
 *    <li>ERROR_THREAD_INVALID_ACCESS - if not called from the thread that created the receiver</li>
 * </ul>
 * 
 * @since 4.x (TBD)
 */
   public boolean setTextDirection(int textDirection)

2. The API will be also introduced to selective Widgets:

  2.1. Item#setTextDirection(int textDirection)

  2.2. ToolTip#setTextDirection(int textDirection)
Comment 8 Lina Kemmel CLA 2012-12-05 11:54:57 EST
Created attachment 224319 [details]
Initial version of a win32 patch
Comment 9 Lina Kemmel CLA 2012-12-05 12:01:05 EST
(In reply to comment #7)
> 2. The API will be also introduced to selective Widgets:
> 
>   2.1. Item#setTextDirection(int textDirection)
> 
>   2.2. ToolTip#setTextDirection(int textDirection)

This turned out not necessary, but some Widget descendants have a non-API updateTextDirection(int textDirection) method.
Comment 10 Lina Kemmel CLA 2012-12-10 11:06:16 EST
Created attachment 224525 [details]
Modified controlexample

I also attach a patch for org.eclipse.swt.examples.controlexample package which should help test patch 224319.
Comment 11 Silenio Quarti CLA 2013-03-07 13:06:59 EST
Thanks Lina, this patch is generally good, we just need a few modifications:

- I think the APIs should be "*TextOrientation*" so that they resemble the exiting "*Orientation*" API.

void setTextOrientation(int)
int getTextOrientation()
SWT.FLIP_TEXT_ORIENTATION

- Would it be possible to merge the updateTextDirection() function with the updateOrientation() function? They both change the GWL_STYLE and with this patch the updateOrientation() method traverses the whole widget hierarchy twice (bad for performance). 

- Is FLIP_TEXT_ORIENTATION respected at widget creation time? I was expecting to see some code in Control.widgetExtStyle() for that.

- I do not think Composite.updateTextDirection() needs to go through the controls in the tabList array. _getChildren() already contains those controls. I believe the work is done twice for those controls.

- Move the constants in Item.java to Widget.java. Remove the ones in Group.

- the code in ToolItem.updateTextDirection() is a copy of a portion ToolItem.setText(). We should add a _setText() helper to reuse the portion of the code.

- the same as above for the code in Group.enableWidget(), Group.setText and Group.updateTextDirection()

- the @since tag needs to be 3.101 for 4.3

- we need the stubs for GTK and COCOA
Comment 12 Silenio Quarti CLA 2013-03-07 14:07:43 EST
(In reply to comment #10)
> Created attachment 224525 [details]
> Modified controlexample
> 
> I also attach a patch for org.eclipse.swt.examples.controlexample package
> which should help test patch 224319.

Is this just for testing purposes or do you want to get this patch released as well?

If the later, I think we need a way to turn on and off the "..." added to the contents. I would not want to see those all the time as it makes to sample less polished.
Comment 13 Lina Kemmel CLA 2013-03-12 10:30:44 EDT
(In reply to comment #11)
Silenio, thank you very much for the comments.

> - I think the APIs should be "*TextOrientation*" so that they resemble the
> exiting "*Orientation*" API.

- OK, but frankly I'd prefer "*TextDirection*" as it is more consistent the terminology/naming already used in some existing applications.
 
> void setTextOrientation(int)
> int getTextOrientation()
> SWT.FLIP_TEXT_ORIENTATION
> 
> - Would it be possible to merge the updateTextDirection() function with the
> updateOrientation() function? They both change the GWL_STYLE and with this
> patch the updateOrientation() method traverses the whole widget hierarchy
> twice (bad for performance). 

- Yes, I am trying to do so.

> - Is FLIP_TEXT_ORIENTATION respected at widget creation time? I was
> expecting to see some code in Control.widgetExtStyle() for that.

- Yes, this should be done. I didn't add it at widget creation time for 2 reasons:
1. I was under impression that WS_EX_RTLREADING is not respected upon widget creation perfectly. However, testing it now with several widgets, I see it does appear to work so far.
2. I think it is preferable that application operates constants expressing explicit (right-to-left or left-to-right) direction (like those we use in setOrientation and getOrientation methods). To make it possible, we would need to introduce 2 new SWT style constants (and change the type of style-related variables to long).

> - I do not think Composite.updateTextDirection() needs to go through the
> controls in the tabList array. _getChildren() already contains those
> controls. I believe the work is done twice for those controls.

- Oh sure, I removed that piece of code already (but unfortunately, after submitting the patch...)

> - we need the stubs for GTK and COCOA
- Should it be added also for carbon, motif, wpf, photon?
Comment 14 Lina Kemmel CLA 2013-03-12 10:41:19 EDT
Regarding the controlexample patch: it is for local testing purposes only.
To get it suitable to be released we can create separate resource bundles accommodating the ellipses and other content changes.
Comment 15 Lina Kemmel CLA 2013-03-12 10:46:20 EDT
(In reply to comment #11)
> - Move the constants in Item.java to Widget.java. Remove the ones in Group.
> 
> - the code in ToolItem.updateTextDirection() is a copy of a portion
> ToolItem.setText(). We should add a _setText() helper to reuse the portion
> of the code.
> 
> - the same as above for the code in Group.enableWidget(), Group.setText and
> Group.updateTextDirection()
> 
> - the @since tag needs to be 3.101 for 4.3

Yes, I am changing these places, thank you.
Comment 16 Lina Kemmel CLA 2013-03-12 13:03:45 EDT
Silenio,
In Group#enableWidget, there is the following piece of code:
	String string = null;
	if ((style & SWT.RIGHT_TO_LEFT) != 0) {
		if (OS.COMCTL32_MAJOR < 6 || !OS.IsAppThemed ()) {
			string = enabled || text.length() == 0 ? text : " " + text + " ";
		}
	}
	if (string != null) {
		TCHAR buffer = new TCHAR (getCodePage (), string, true);
		OS.SetWindowText (handle, buffer);
	}

Shouldn't it instead of:
	string = enabled || text.length() == 0 ? text : " " + text + " ";

be the following:
	string = enabled || text.length() == 0 ? *null* : " " + text + " ";

- since (string != null) is to trigger further action.
Comment 17 Silenio Quarti CLA 2013-03-12 14:07:46 EDT
(In reply to comment #13)
> (In reply to comment #11)
> Silenio, thank you very much for the comments.
> 
> > - I think the APIs should be "*TextOrientation*" so that they resemble the
> > exiting "*Orientation*" API.
> 
> - OK, but frankly I'd prefer "*TextDirection*" as it is more consistent the
> terminology/naming already used in some existing applications.



APIs on different toolkits:

Cocoa -> NSCell.userInterfaceLayoutDirection, NSCell.baseWritingDirection, NSCell.setBaseWritingDirection
GTK2/GTK3 -> gtk_widget_get_direction, gtk_widget_set_direction
WPF -> FrameworkElement.FlowDirection
Qt -> layoutDirection

They all have "direction". Ok, let's keep your initial suggestion.


>  
> > void setTextOrientation(int)
> > int getTextOrientation()
> > SWT.FLIP_TEXT_ORIENTATION
> > 
> > - Is FLIP_TEXT_ORIENTATION respected at widget creation time? I was
> > expecting to see some code in Control.widgetExtStyle() for that.
> 
> 2. I think it is preferable that application operates constants expressing
> explicit (right-to-left or left-to-right) direction (like those we use in
> setOrientation and getOrientation methods). To make it possible, we would
> need to introduce 2 new SWT style constants (and change the type of
> style-related variables to long).


Yes, we should have two constants (TEXT_LEFT_TO_RIGHT and TEXT_RIGHT_TO_LEFT), but we do not have any bits left. 

 
> > - we need the stubs for GTK and COCOA
> - Should it be added also for carbon, motif, wpf, photon?


We are not updating those anymore.
Comment 18 Silenio Quarti CLA 2013-03-12 14:18:14 EDT
(In reply to comment #16)
> Silenio,
> In Group#enableWidget, there is the following piece of code:
> 	String string = null;
> 	if ((style & SWT.RIGHT_TO_LEFT) != 0) {
> 		if (OS.COMCTL32_MAJOR < 6 || !OS.IsAppThemed ()) {
> 			string = enabled || text.length() == 0 ? text : " " + text + " ";
> 		}
> 	}
> 	if (string != null) {
> 		TCHAR buffer = new TCHAR (getCodePage (), string, true);
> 		OS.SetWindowText (handle, buffer);
> 	}
> 
> Shouldn't it instead of:
> 	string = enabled || text.length() == 0 ? text : " " + text + " ";
> 
> be the following:
> 	string = enabled || text.length() == 0 ? *null* : " " + text + " ";
> 
> - since (string != null) is to trigger further action.

Why would we set to *null*? I thing it would cause an exception in TCHAR.
Comment 19 Lina Kemmel CLA 2013-03-12 15:30:06 EDT
(In reply to comment #18)
> (In reply to comment #16)
> > Silenio,
> > In Group#enableWidget, there is the following piece of code:
> > 	String string = null;
> > 	if ((style & SWT.RIGHT_TO_LEFT) != 0) {
> > 		if (OS.COMCTL32_MAJOR < 6 || !OS.IsAppThemed ()) {
> > 			string = enabled || text.length() == 0 ? text : " " + text + " ";
> > 		}
> > 	}
> > 	if (string != null) {
> > 		TCHAR buffer = new TCHAR (getCodePage (), string, true);
> > 		OS.SetWindowText (handle, buffer);
> > 	}
> > 
> > Shouldn't it instead of:
> > 	string = enabled || text.length() == 0 ? text : " " + text + " ";
> > 
> > be the following:
> > 	string = enabled || text.length() == 0 ? *null* : " " + text + " ";
> > 
> > - since (string != null) is to trigger further action.
> 
> Why would we set to *null*? I thing it would cause an exception in TCHAR.

*null* indicates here that the text adjustment is not required (and a TCHAR buffer won't be created). This adjustment is needed "When a group control is right-to-left and is disabled" (and also when the text is not empty). So I think that the case (enabled || text.length() == 0) should not be handled - which would be indicated by nullifying the 'string'.
Comment 20 Lina Kemmel CLA 2013-03-13 11:30:10 EDT
(In reply to comment #11)
> - Would it be possible to merge the updateTextDirection() function with the
> updateOrientation() function?

Sorry, updateOrientation() should not call updateTextDirection() at all, since the requirement is to reset text direction to the default one when orientation changes. So updateOrientation() should just reset the styles related to text direction.
Comment 21 Lina Kemmel CLA 2013-03-13 11:47:53 EDT
(In reply to comment #20)
> updateOrientation() should just reset the styles related to text direction.
- and this will be done inline without walking the widget hierarchy again.
Comment 22 Lina Kemmel CLA 2013-03-13 12:56:26 EDT
Created attachment 228372 [details]
Patch updated per comment 11

Silenio, could you please review the patch? Thank you!
Comment 23 Lina Kemmel CLA 2013-03-13 13:08:29 EDT
Created attachment 228374 [details]
Patch updated per comment 11

I noticed some problem in Group and resubmitting the patch.
Comment 24 Silenio Quarti CLA 2013-03-13 15:17:25 EDT
Thanks!

I have released the changes. I made only a couple of updates:
- Item.updateTextDirection() should not call checkWidget() and 
- the 4.3 version has changed from 3.101 to 3.102 due to bug#401797.

Pushed to eclipse.org

http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=2b14d767020ed3598cdf53c0df27bc3b8bd1ea64
Comment 25 Markus Keller CLA 2013-03-14 11:04:51 EDT
I've just added http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=1014d7ede6c35fcf01a04fb6d5978ea9132b07cf to avoid surprising clients that try to use Control#setTextDirection(int) on GTK and Cocoa.