Bug 467328

Comment 1 Moshe WAJNBERG 2015-05-14 09:18:41 EDT

Created attachment 253479 [details]
Adding auto text direction for BidiUtils.applyTextDirection(control, textDirection)

Hello Markus,

Could you please review these changes ?

Thank you very much for your review

Comment 2 Tomer Mahlin 2015-05-15 08:36:20 EDT

*** Bug 465496 has been marked as a duplicate of this bug. ***

Comment 3 Markus Keller 2015-05-18 13:44:55 EDT

I'll test/review tomorrow.

Comment 4 Lina Kemmel 2015-05-19 05:27:48 EDT

One comment on the patch.

if (LEFT_TO_RIGHT.equals(textDirection)) {
 			textDir = SWT.LEFT_TO_RIGHT;
 		} else if (RIGHT_TO_LEFT.equals(textDirection)) {
 			textDir = SWT.RIGHT_TO_LEFT;
 		} else if (AUTO.equals(textDirection)) {
-			auto = true;
+			textDir = SWT.LEFT_TO_RIGHT | SWT.RIGHT_TO_LEFT;
 		} 
......................

I suggest to keep "auto = true;" line, since (SWT.LEFT_TO_RIGHT | SWT.RIGHT_TO_LEFT) is not supported by StyledText yet.

For the same reason:

-		} else if (control instanceof StyledText && (auto || textDir != 0)) {
+		} else if (control instanceof StyledText && textDir != 0) {

- keep the old code :
"} else if (control instanceof StyledText && (auto || textDir != 0)) {"

Comment 5 Lina Kemmel 2015-05-19 05:33:31 EDT

Sorry: please discard comment 4.

Comment 6 Markus Keller 2015-05-19 12:34:00 EDT

(In reply to Moshe WAJNBERG from comment #1)
> Created attachment 253479 [details] [diff]
> Adding auto text direction for BidiUtils.applyTextDirection(control,
> textDirection)

+1 for RC2. Verified that the patch works and effectively only changes the behavior when Control#setTextDirection(int) is called with SWT.LEFT_TO_RIGHT | SWT.RIGHT_TO_LEFT. Filed bug 467601 for an issue in that API. But that shouldn't stop this fix.

Lina and Dani, do approve for RC2?

Comment 7 Lina Kemmel 2015-05-19 21:22:58 EDT

I think that in applyTextDirection the following section:

		if (control instanceof Text && textDir != 0) {
 			applyBidiProcessing((Text) control, textDirection);
		} else if (control instanceof StyledText && textDir != 0) {
 			applyBidiProcessing((StyledText) control, textDirection);
		} else if (control instanceof Combo && textDir != 0) {
 			applyBidiProcessing((Combo) control, textDirection);
 		} else if (textDir != 0) {
 			control.setTextDirection(textDir);
		}

- can be changed to:

		if (control instanceof StyledText && textDir == SWT.LEFT_TO_RIGHT | SWT.RIGHT_TO_LEFT) {
 			applyBidiProcessing((StyledText) control, textDirection);
 		} else if (textDir != 0) {
 			control.setTextDirection(textDir);
		}

- since only StyledText with AUTO direction needs now a SegmentListener...

Comment 8 Lina Kemmel 2015-05-19 21:44:23 EDT

This, of course, applies to win32... On GTK, SegmentsListener has an added value also for Text.

Comment 9 Markus Keller 2015-05-20 06:00:11 EDT

(In reply to Lina Kemmel from comment #8)
> This, of course, applies to win32... On GTK, SegmentsListener has an added
> value also for Text.

Yes, and that's why we should go with the proposed patch. The simplification on Windows is not necessary and would add undue risk in RC2.

Comment 10 Lina Kemmel 2015-05-20 07:26:40 EDT

Absolutely, agree, thanks.
+1 from me too.

Comment 11 Dani Megert 2015-05-20 09:52:31 EDT

(In reply to Lina Kemmel from comment #10)
> Absolutely, agree, thanks.
> +1 from me too.

Lina, please set the review flag.

Comment 12 Lina Kemmel 2015-05-20 10:00:41 EDT

Done!

Comment 13 Dani Megert 2015-05-20 11:05:11 EDT

(In reply to Moshe WAJNBERG from comment #1)
> Created attachment 253479 [details] [diff]
> Adding auto text direction for BidiUtils.applyTextDirection(control,
> textDirection)

Submitted with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6fb0a9af7ffa9b88dd0aa37b7606ae4f46a02d93

Comment 14 Dani Megert 2015-05-21 10:53:27 EDT

Verified that the fix is in I20150520-2000.