Bug 274567

Summary: [Palette] BIDI3.5:Wrong shaping for digits in Customize Palette dialog and in Logic editor
Product: [Tools] GEF Reporter: Dina Sayed <dsayed>
Component: GEF-Legacy GEF (MVC)Assignee: gef-inbox <gef-inbox>
Status: NEW --- QA Contact:
Severity: normal    
Priority: P3 CC: ahunter.eclipse, camle, dsayed, ghadas, kitlo, mahag, melsayad, mfadl, nyssen, YEHIASED
Version: 3.5   
Target Milestone: ---   
Hardware: PC   
OS: Windows Vista   
Whiteboard:
Bug Depends on: 339090    
Bug Blocks: 135242, 307328    
Attachments:
Description Flags
Snap shots for the defects
none
Proposed patch
none
Proposed Patch
none
Proposed patch for ICU shaping in solution2
none
Snapshots
none
text flow patch
none
bidi processor patch none

Description Dina Sayed CLA 2009-04-30 14:35:45 EDT
Created attachment 133993 [details]
Snap shots for the defects

Build ID: I20090313-0100

Steps To Reproduce:
Platform : Windows vista

1-Open the eclipse
2-Go to File->New->Other->General->Project 
3-Add Arabic name mixed with numbers for the project name and then press finish

4-Go to File->New->Example->GEF->Logic diagram and hit Next. 
Be sure to select Logic Diagram  and not Logic under the GEF (Graphical Editing Framework) Plug-ins
5- give the new file an Arabic name.
6-Create a label in the logic editor that opens up (by dragging a label - under components - from the palette).  
7-Edit the label text in the logic editor and enter an Arabic text mixe with digits

Check Result 1

8-Right-click anywhere on the palette and select "Customize" from the pop-up menu.
9-In the dialog that pops up, select the tree item labeled "Components." You should be able to edit its name and description
 by typing Arabic text and numbers in the areas on the right-hand side of the tree
10-press ok to close the dialog 
11-View the changes you applied on the palette component name and hover the mouse to check the description

Check result 2




Expected Results


Result 1:Arabic text and numerals are displayed and shaped correctly in the label inside the logic editor
Result 2:Arabic text and numerals are displayed and shaped correctly



Actual Results:

Result 1:When editing in the text in the label the numerals are shaped correctly (check screen shot LabelInLogicEditor_underFocus_Result1.JPG) 
However the numerals are shaped wrongly it is shaped as Arabic European digits instead of Arabic indic digits after losing the focus  (check screen shot LogicEditor_Result1.JPG)

Result 2:When editing in the customize palette editor ,Arabic and numeral are displayed and shaped correctly(check screen shot CustomizeEditingdialog_Result2.JPG)
However after pressing ok and hover over the component to check the new description for the component , the numerals are shaped wrongly
it is shaped as Arabic European digits instead of Arabic indic digits(check screen shot CustomizeEdit_Result2.JPG)

More information:
Comment 1 Anthony Hunter CLA 2009-04-30 15:03:20 EDT
Resolution of BIDI defects are not in plan for Galileo. BIDI in text fields in
SWT applications are limited by issues in the base platform and there is no
plan to address these issues in Galileo.

Leaving open in case BIDI defects are looked at in a future release.
Comment 2 Dina Sayed CLA 2011-03-07 07:33:42 EST
Hi Anthony, 
I am interested in contributing to fix this bug, Would you direct me for what was reached so far from SWT limitation so that I may continue?
Comment 3 Dina Sayed CLA 2011-03-31 11:40:02 EDT
Created attachment 192295 [details]
Proposed patch

Part of this fix will depend on SWT 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=339090
Comment 4 Ghada Selim CLA 2011-08-15 07:05:15 EDT
Created attachment 201482 [details]
Proposed Patch
Comment 5 Ghada Selim CLA 2011-08-15 07:08:07 EDT
I believe that the call org.eclipse.draw2d.text.TextFlow.contributeBidi(BidiProcessor proc) is responsible for dividing the text into fragments and then each fragment is passed individually to be drawn, if a fragment contains numbers, then it is not passed within its context and therefore it will not be shaped correctly

Now I have a few questions
1- What is the purpose of BidiProcessor?
2- why it was added, what kind of bidi issues it solve and why it is needed in TextFlow?

As far as i see stopping its calls solve the problem and do not cause any issue
please check the proposed patch
Comment 6 Dina Sayed CLA 2011-10-11 16:33:40 EDT
(In reply to comment #4)
> Created attachment 201482 [details]
> Proposed Patch

Would anyone at least check this patch? fixing this bug will also fix a number of related bugs
Comment 7 Alexander Nyßen CLA 2011-10-11 17:06:58 EDT
(In reply to comment #6)
> (In reply to comment #4)
> > Created attachment 201482 [details] [details]
> > Proposed Patch
> 
> Would anyone at least check this patch? fixing this bug will also fix a number
> of related bugs

Well, I don't know much about Bidi (and I don't read or speak Arabic), but according to the Javadoc of BidiProcessor#process(), it is responsible of the following: 

* Processes the contributed text, determines the Bidi levels, and assign
* them to the FlowFigures that made thet contributions. This class is for
* INTERNAL use only. Shaping of visually contiguous Arabic characters that
* are split in different figures is also handled. This method will do
* nothing if the contributed text does not require Bidi evaluation. All
* contributions are discarded at the end of this method.

Thus, I doubt that simply removing the call is a valid patch.
Comment 8 Alexander Nyßen CLA 2011-10-11 17:10:13 EDT
(In reply to comment #3)
> Created attachment 192295 [details]
> Proposed patch
> 
> Part of this fix will depend on SWT 
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=339090

Concerning this one it seems that the underlying SWT changes have not yet been applied. What could we do beforehand?
Comment 9 Ghada Selim CLA 2011-11-29 06:36:10 EST
I can see that there are two proposed solutions for this bug:
1)Solution 1: Stopping the call to org.eclipse.draw2d.text.BidiProcessor in 
org.eclipse.birt.report.designer.internal.ui.editors.schematic.figures.TextFlow in case of Arabic locale.(refer to the patch in comment 4).

2)Solution 2: Shaping the digits using ICU to be in Arabic form in case of locale = Arabic and digit substitution = Context. (Please Check the attached patch which is based on the proposed fix in comment 3). This solution introduces a new method[isContextDigitSubistitution()] in "org.eclipse.swt.graphics.TextLayout" in SWT to check the digit substitution and then it is used by method [shapeArabicDigitsContextually(String text, int orient)]in org.eclipse.draw2d.text.BidiProcessor to perform the shaping. The method [isContextDigitSubistitution()] is added also to org.eclipse.swt.graphics.Resource which is the super class of org.eclipse.swt.graphics.TextLayout to avoid compilation errors in different platforms other than Windows which don't have this bug present(See the attached 
snapshots for Windows and Linux platforms after applying the fix).
So this solution requires changes in these classes:
    org.eclipse.draw2d.text.BidiProcessor
  org.eclipse.draw2d.text.TextFlow

  org.eclipse.swt.graphics.TextLayout
  org.eclipse.swt.graphics.Resource

Please advice.
Comment 10 Ghada Selim CLA 2011-11-29 06:37:15 EST
Created attachment 207645 [details]
Proposed patch for ICU shaping in solution2
Comment 11 Ghada Selim CLA 2011-11-29 06:39:04 EST
Created attachment 207646 [details]
Snapshots

Snapshots on Linux and Windows platforms for the fix in solution2
Comment 12 Yehia Abo Sedera CLA 2012-10-22 06:46:20 EDT
Created attachment 222628 [details]
text flow patch
Comment 13 Yehia Abo Sedera CLA 2012-10-22 06:46:54 EDT
Created attachment 222629 [details]
bidi processor patch
Comment 14 Yehia Abo Sedera CLA 2012-10-22 06:47:34 EDT
I have fixed the issue . kindly check attached patches (text flow & bidi processor )
Comment 15 Yehia Abo Sedera CLA 2012-12-18 07:19:49 EST
Hi All,
Kindly review the attached patch.
Thank you