Bug 179321 - TextProcessor should treat "he" as a rtl language
Summary: TextProcessor should treat "he" as a rtl language
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Framework (show other bugs)
Version: unspecified   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 3.3 M7   Edit
Assignee: Thomas Watson CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 183164
  Show dependency tree
 
Reported: 2007-03-26 11:31 EDT by Tod Creasey CLA
Modified: 2007-04-19 07:44 EDT (History)
3 users (show)

See Also:


Attachments
patch (3.59 KB, patch)
2007-03-27 13:11 EDT, Thomas Watson CLA
no flags Details | Diff
patch (1.44 KB, patch)
2007-03-27 16:47 EDT, Thomas Watson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tod Creasey CLA 2007-03-26 11:31:41 EDT
M6

When we started to do the bidi support for Eclipse we had to support an existing command line argument - dir (help had this in 2.1). 

The TextProcessor class does not support this parameter and so is inconsistent with the bidi support in the rest of the SDK.

I had a look to see how to implement it but I am not sure how to get the command line args at the OSGi level. Here is how we did it in the WorkbenchPlugin for reference:

for (int i = 0; i < commandLineArgs.length - 1; i++) {
			if(commandLineArgs[i].equalsIgnoreCase(ORIENTATION_COMMAND_LINE)){
				String orientation = commandLineArgs[i+1];
				if(orientation.equals(RIGHT_TO_LEFT)){
					System.setProperty(ORIENTATION_PROPERTY,RIGHT_TO_LEFT);
					return SWT.RIGHT_TO_LEFT;
				}
				if(orientation.equals(LEFT_TO_RIGHT)){
					System.setProperty(ORIENTATION_PROPERTY,LEFT_TO_RIGHT);
					return SWT.LEFT_TO_RIGHT;
				}
			}
		}
Comment 1 Thomas Watson CLA 2007-03-27 13:11:38 EDT
Created attachment 62125 [details]
patch

I've said this more than once, but I will say it again.  I really wish we never placed this class so far down in the stack ;-)  Absolutely nobody in Equinox uses this class.

Here is a patch to process the -dir and eclipse.orientation property.  There seems to be an inconsistency between what WorkbenchPlugin and TextProcessor thinks are rtl locales.  In TextProcessor the following langs are considered rtl by default (if -dir or eclipse.orientation are not used):

"iw" "ar" "fa" "ur"

In WorkbenchPlugin the following are considered rtl, notice the extra "he":
"iw" "he" "ar" "fa" "ur"

Which is correct?  Should we add "he" as an RTL in TextProcessor?
Comment 2 Tod Creasey CLA 2007-03-27 13:28:36 EDT
he is the deprecated Hebrew locale identifier - adding he should likely be done for compatbility reasons.
Comment 3 Karice McIntyre CLA 2007-03-27 13:49:04 EDT
Oops, Tod got it backwards, "iw" is deprecated and "he" is the new code.  See:
http://ftp.ics.uci.edu/pub/ietf/http/related/iso639.txt

I added "he" to the workbench because I wasn't sure what the translation centers were going to use for the locale in the translated Hebrew files (turns out they stuck with "iw" for 3.2.x).

I don't anticipate Equinox ever using this API either but we needed it this low in the stack so other potential users wouldn't have to pre-req the UI (or some other arbitrary plugin higher up the stack) just to have this functionality available to them.  If there comes a day where a more sensical place to put this API emerges then perhaps it could be moved.  

In any case, the patch looks like what we are looking for.  I guess we should supply you with some tests before making it official.
Comment 4 Thomas Watson CLA 2007-03-27 14:54:43 EDT
should we add "he" to TextProcessor then?
Comment 5 Tod Creasey CLA 2007-03-27 16:00:40 EDT
Yes we should.

I am also thinking that should be the only thing we should do. Sorry to create the fire drill but on discussion with people here the processing of text really doesn't have anything to do with direction.

If I run in Hebrew with no command line parameters I get text processing turned on  and left to right layout. This is correct behaviour.

I think that if it is unclear that this is required that we only do the change to add he unless someone gives us a strong reason not to.
Comment 6 Thomas Watson CLA 2007-03-27 16:43:20 EDT
Changing title to reflect the requested change.
Comment 7 Thomas Watson CLA 2007-03-27 16:47:23 EDT
Created attachment 62154 [details]
patch

Here is a patch to add "he".  I will release this for M7 unless someone objects.
Comment 8 Thomas Watson CLA 2007-03-30 15:55:23 EDT
Fixed released in HEAD for M7.