Bug 465495 - [BIDI] BidiUtils.applyBidiProcessing should work on String level (replace TextProcessor API)
Summary: [BIDI] BidiUtils.applyBidiProcessing should work on String level (replace Tex...
Status: ASSIGNED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Runtime (show other bugs)
Version: 4.5   Edit
Hardware: PC Windows Server 2003
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Markus Keller CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 466345 467112
  Show dependency tree
 
Reported: 2015-04-26 07:04 EDT by Tomer Mahlin CLA
Modified: 2017-05-26 09:33 EDT (History)
4 users (show)

See Also:


Attachments
new API applyBidiProcessing(String string, String handlingType) (4.30 KB, patch)
2015-05-04 09:10 EDT, Moshe WAJNBERG CLA
no flags Details | Diff
new API applyBidiProcessing(String string, String handlingType) (4.47 KB, patch)
2015-05-05 04:51 EDT, Moshe WAJNBERG CLA
no flags Details | Diff
new API applyBidiProcessing(String string, String handlingType) (5.44 KB, patch)
2015-05-05 09:00 EDT, Moshe WAJNBERG CLA
no flags Details | Diff
new API applyBidiProcessing(String string, String handlingType) (5.89 KB, patch)
2015-05-05 11:25 EDT, Moshe WAJNBERG CLA
no flags Details | Diff
new API applyBidiProcessing(String string, String handlingType) (5.77 KB, patch)
2015-05-06 05:07 EDT, Moshe WAJNBERG CLA
no flags Details | Diff
new API applyBidiProcessing(String string, String handlingType) (6.44 KB, patch)
2015-05-13 06:03 EDT, Moshe WAJNBERG 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 2015-04-26 07:04:38 EDT
BidiUtils.applyBidiProcessing from org.eclipse.jface.util.BidiUtils package
is currently useful in graphical widget context only. Namely it can be called on input field, combo box etc. Following signature is supported:
   applyTextDirection(Control, String) 

It should be useful in the context of plain text as well. Namely following signature should be added: 
   applyTextDirection(String inputStr, String sttType) 

The implementation can be as simple as follows: 
 return STextExpertFactory.getExpert(sttType).leanToFullText (inputStr);


PS. BidiUtils.applyBidiProcessing is discussed in defect 403052    , defect 403643, defect 409391, defect 427794, defect 457025.
Comment 1 Moshe WAJNBERG CLA 2015-05-04 09:10:45 EDT
Created attachment 253137 [details]
new API applyBidiProcessing(String string, String handlingType)

Hello Markus,

Can you please review this patch ?

Thank you very much fro your review
Comment 2 Markus Keller CLA 2015-05-04 09:47:50 EDT
This doesn't look much different from org.eclipse.equinox.bidi.StructuredTextProcessor#processTyped(String, String), except that you use getStatefulExpert(..) instead of getExpert(..), and that you added support for the BTD options.

If the goal is to replace the old org.eclipse.osgi.util.TextProcessor, then we'd also need a deprocess(..) method. This is already available in StructuredTextProcessor, and the STP methods also look more compatible with the old TP in that they skip processing if the given string has already been processed.

> PS. BidiUtils.applyBidiProcessing is discussed in [..]
(Bugs as links:) bug 403052, bug 403643, bug 409391, bug 427794, bug 457025.
Comment 3 Tomer Mahlin CLA 2015-05-04 11:57:34 EDT

2. (In reply to Markus Keller from comment #2)
> This doesn't look much different from
> org.eclipse.equinox.bidi.StructuredTextProcessor#processTyped(String,
> String), except that you use getStatefulExpert(..) instead of getExpert(..),
> and that you added support for the BTD options.
> 
> If the goal is to replace the old org.eclipse.osgi.util.TextProcessor, then
> we'd also need a deprocess(..) method. This is already available in
> StructuredTextProcessor, and the STP methods also look more compatible with
> the old TP in that they skip processing if the given string has already been
> processed.
> 
> > PS. BidiUtils.applyBidiProcessing is discussed in [..]
> (Bugs as links:) bug 403052, bug 403643, bug 409391, bug 427794, bug 457025.


TextProcessor is considered a deprecated solution by now. The main reasons are:
a. it is too rigid (not extendable) 
b. does not cover many different types of structured text 
c. does not work according to general logic for invocation of Bidi support in Eclipse. 

The goal of suggested change is two fold:

1. Replace TextProcessor - it should not be a problem to add a deprocess method (even though it was mainly used for editable fields - which are now covered by a different solution which automates deprocess phase). 

2. Provide a single API for handling the same type of issues in different graphical contexts - signature suggested in this bug covers String context.


Finally, through a different code submission we will propose a general API for handling Bidi in the context of message formatting. This latter API will heavily use the one suggested in this bug. 
This new API is supposed to cover following use cases:
1. Concatenations
 displayText = someStr1 + someStr2 + someStr3 + ... + someStrN

2. Templated messages
 - load a message template based on message ID (i.e. "{0} file does not exist")
 - replace the placeholders (i.e. {0}) with actual data (i.e. c:\file.txt)
 - create result display string
Please observe that Concatenation is a particular case of templated messages, since every concatenation can be represented by template like: 
{0}{1}{2}{3}...{n}

In all those cases it might be important (depending on actual text / data) to enforce text direction or order of tokens for either placeholder or/and entire message. 

@Moshe, can you please add a deprocess function as requested by Markus ? Thanks.
Comment 4 Tomer Mahlin CLA 2015-05-04 14:14:30 EDT
I logged bug 466345 to track message formatting work.
Comment 5 Moshe WAJNBERG CLA 2015-05-05 04:51:52 EDT
Created attachment 253158 [details]
new API applyBidiProcessing(String string, String handlingType)

Hello Markus,

I followed your advice and use the StructuredTextProcessor#processTyped method now.

Can you please review the changes ?

Thank you
Comment 6 Markus Keller CLA 2015-05-05 06:38:12 EDT
(In reply to Tomer Mahlin from comment #3)
> 1. Replace TextProcessor - it should not be a problem to add a deprocess
> method (even though it was mainly used for editable fields - which are now
> covered by a different solution which automates deprocess phase).

The two main areas where we use deprocess are cases where we work with a rendered item label:
- Copy to Clipboard
- filtering on tree/table items (e.g. JDT's Quick Outline)
Comment 7 Markus Keller CLA 2015-05-05 06:42:02 EDT
(In reply to Moshe WAJNBERG from comment #5)
- What about BidiUtils#VISUAL_* ?
- The Javadoc of the new API should mention StructuredTextProcessor#deprocessTyped(String, String). And it should tell what to do in case the handlingType is not an STT but BTD_DEFAULT etc.
Comment 8 Tomer Mahlin CLA 2015-05-05 07:56:09 EDT
(In reply to Markus Keller from comment #7)
> (In reply to Moshe WAJNBERG from comment #5)
> - What about BidiUtils#VISUAL_* ?

Visual is a completely different world (world of Mainframe). The problem of enforcement of text direction or handling structured does not exist there since on Mainframe there is no reordering between buffer and display. You simply display the buffer as-is. Thus, if sttType BidiUtils#VISUAL_* is passed to the API, it should be ignored and input string should be returned to the caller.
Comment 9 Markus Keller CLA 2015-05-05 08:44:17 EDT
(In reply to Tomer Mahlin from comment #8)
> Thus, if sttType BidiUtils#VISUAL_* is
> passed to the API, it should be ignored and input string should be returned
> to the caller.

Really? So why is this different for applyBidiProcessing(Text, String)? There, the VISUAL_* handlingTypes were added to support editing data that is stored on a Mainframe. If the same data is e.g. shown in a Table, then I don't see why it shouldn't be rendered the same way as in an editable Text field, i.e. embedded in RLO/LRO and PDF.
Comment 10 Moshe WAJNBERG CLA 2015-05-05 09:00:30 EDT
Created attachment 253170 [details]
new API applyBidiProcessing(String string, String handlingType)

Hello Markus,

I added a new public method String deprocessBidi(String string) for Bidi deprocessing and added some explanations in the Java doc of the applyBidiProcessing(String string, String handlingType) public API.

Thank you for your review
Comment 11 Tomer Mahlin CLA 2015-05-05 09:55:05 EDT
(In reply to Markus Keller from comment #9)
> (In reply to Tomer Mahlin from comment #8)
> > Thus, if sttType BidiUtils#VISUAL_* is
> > passed to the API, it should be ignored and input string should be returned
> > to the caller.
> 
> Really? So why is this different for applyBidiProcessing(Text, String)?
> There, the VISUAL_* handlingTypes were added to support editing data that is
> stored on a Mainframe. If the same data is e.g. shown in a Table, then I
> don't see why it shouldn't be rendered the same way as in an editable Text
> field, i.e. embedded in RLO/LRO and PDF.

Ok. I see your point. VISUAL_* handles different approaches of text display on mainframe (mostly it has to do with position of origin of coordinates on the green screen). It can be very faintly associated with text direction on Windows. So, it would probably mean that when VISUAL_* are provided we will add RLO/LRO and PDF just like in handlers meant to be used in graphical contexts.

@Moshe, it shouldn't be that complex change should it ? Thanks a lot in advance.
Comment 12 Moshe WAJNBERG CLA 2015-05-05 11:25:51 EDT
Created attachment 253180 [details]
new API applyBidiProcessing(String string, String handlingType)

Hello Markus,

I added the support for VISUAL LTR and RTL and updated the Java doc.

Can you please review the changes ?

Thank you
Comment 13 Markus Keller CLA 2015-05-05 12:07:54 EDT
Is BidiUtils#deprocessBidi(..) really necessary? I think org.eclipse.equinox.bidi.StructuredTextProcessor#deprocess(String) should cover such cases. But that method needs to be updated for the RLE, LRO, and RLO control characters that are now in use.

If an additional method in BidiUtils is kept, the unnecessary StringBuffer creations would have to be fixed.
Comment 14 Moshe WAJNBERG CLA 2015-05-06 05:06:47 EDT
(In reply to Markus Keller from comment #13)
> Is BidiUtils#deprocessBidi(..) really necessary? I think
> org.eclipse.equinox.bidi.StructuredTextProcessor#deprocess(String) should
> cover such cases. But that method needs to be updated for the RLE, LRO, and
> RLO control characters that are now in use.
> 

I'd prefer to have a new deprocessBidi API in BidiUtils since org.eclipse.equinox.bidi.StructuredTextProcess is rather for Structured expression not BTD

> If an additional method in BidiUtils is kept, the unnecessary StringBuffer
> creations would have to be fixed.

OK I have replaced SringBuffer by regular Strings
Comment 15 Moshe WAJNBERG CLA 2015-05-06 05:07:50 EDT
Created attachment 253211 [details]
new API applyBidiProcessing(String string, String handlingType)

Hello Markus,

Can you please review these changes ?

Thank you
Comment 16 Markus Keller CLA 2015-05-12 10:38:26 EDT
The new APIs to process/deprocess Strings also has to implement some debugging aids like the applyBidiProcessing(..) methods.

When InternalPolicy.DEBUG_BIDI_UTILS is enabled, then the processed string should include the given handlingType. deprocessBidi(..) should remove the debugging aids again.
Comment 17 Moshe WAJNBERG CLA 2015-05-13 06:03:51 EDT
Created attachment 253432 [details]
new API applyBidiProcessing(String string, String handlingType)

Hello Markus,

I added debug info if InternalPolicy.DEBUG_BIDI_UTILS is enabled.

Could you please review the changes ?

Thank you
Comment 18 Tomer Mahlin CLA 2016-01-06 09:03:44 EST
Dear @markus, any review comments ?
Comment 19 Markus Keller CLA 2016-04-20 09:48:08 EDT
Sorry, I have to move this out, just due to lack of time and other higher-priority items.