Community
Participate
Working Groups
3.4 See bug 196085 and others: We have an text input fields for element labels that should be rendered LTR in an RTL environment. As suggested by Tomer, it would be possible to replace the text widget with a StyledText: StyledText myField = new StyledText(shell, SWT.BORDER | SWT.NONE | SWT.SINGLE); myField.addBidiSegmentListener(new BidiCESegmentListener(BidiComplexExpression.FILE_PATH)); It would be good to have similar API on the text widget so we can continue to use the native widget here. We can also decide that the correct solution is to the Styled Text widget here and we (and other) cleints would change our implementations from Text to StyledText. (for 3.5 or later)
This defect blocks also bug 231195.
From bug 196085: Felipe wrote: > If I recall correctly you offered two solutions: > 1) replace Text with StyledText which already supports bidi segments > 2) add bidi segment support for Text (230854) > IMO, it would be much easier to use option 1 in a few places (new package > dialog, rename dialog package, etc). In the case of Java this wouldn't be just a few places, but quite a lot, if not all: Path input fields, packages, file names, type names for array or generic types (special characters []<>?,), fields for patterns (*.java). I even believe we would have to do it for simple Java identifiers as it's not clear how BIDI languages handle _ and $ and numbers that are allowed in Java identifiers. We are a bit concerned that changing from the native Text widget to the non-native StyleText widget will introduce new bugs as we loose the native functionality offered by the OS for text field: Things like native drag and drop, accessibility, how the widget is rendered. If solution 2.) is not feasible and SWT can calm down our concerns we will consider solution 1.)
Path input fields and file names ? Hmm, all desktop applications will have this is problem. Do they all fix it ? FileDialog and FolderDialog are native in windows32, they don't to anything special here. We will never the able to change the behaviour for them. And by the sounds of it you would also need bidi segments in Combo. Solution 2) I'm not sure this needs to be implemented in SWT (platform offers no support). Anyone can add control characters to the widget. You can implement this in one class in JFace, or I'll have to implemented in 3 to many classes (win32, gtk, mac, wpf, cocoa, flex, etc). The problem is to make sure the control characters are not deleted by backspace or don't get in the way in arrow left and right. So many places will need special handling. It will be super hard to do it, maybe impossible (cause the lack of API).
See Bug 87368 - It is the same problem, I'd like to close it as a duplicate of this. It has interesting information on it (bug 87368 comment #11) and an alternative solution (bug 87368 comment #15) I found more information here: http://wiki.eclipse.org/New_Bidi_APIs It has yet another solution, which I think it works for all read-only controls. See bug 146220, there they mention the need of a solution for editable controls.
*** Bug 87368 has been marked as a duplicate of this bug. ***
I just wanted to clarify following points: 1. Basic approach - The fundamental approach for handling this issue in all mentioned so far and all known to me contexts was always the same - use Unicode Control Characters in order to affect the result of application of Unicode Bidi Algorithm on text. 2. Introduction of Unicode Control Characters - In TextProcessor (described at http://wiki.eclipse.org/New_Bidi_APIs) the Unicode Control Characters are introduced into the text buffer. Thus this solution is good in static contexts. If the buffer is altered by editing we need to control cursor movement, deletion of control characters, distinguish between explicit (introduced by user) and implicit (introduced by us) control characters etc. This functionality is beyond what TextProcessor offers. In StyledText widget case control characters are NOT introduced into buffer. Thus you don't need to take care of cursor movement and other mentioned above aspects. From this perspective StyledText is an attractive solution. 4. Missleading simplifications - I saw that in some cases it was suggested simply to change the direction of widget in order to handle the problem of complex expressions. While for some cases (i.e. pure English Java code) this might be adequate, in general case (i.e. when Bidi text is used) this does not provide correct results. Thus introduction of Unicode Control Characters is innevitable. 5. The solution - It seems that the solution should include 2 parts. Part 1: SWT should provide some basic support (similar to what we have in StyledText via BidiSegementListener). This support can be an enhacement of Text widget (solution 2 in Martin's note), or support for replacement of Text widget by StyledText widget (solution 1 in Martin's note). The latter should address concerns expressed earlier - " ... functionality offered by the OS for text field: Things like native drag and drop, accessibility, how the widget is rendered ... ". Part 2: Each component (i.e. JDT) should use the support from SWT to address specific type of complex expression. This approach is very similar to what was done in case of StyledText - JDT Java code editor. SWT provided support in StyledText via BidiSegmentListener, while JDT added correct segmentation of Java code and through BidiSegmentListener supported correct display of Bidi text as part of Java code in Java code editor (based on StyledText).
Felipe is simply gathering information and merging bug reports.
Win32's RICHEDIT supports a EM_SETBIDIOPTIONS, I tested it setting the options BOE_NEUTRALOVERRIDE and it seems to do what we need. Tomer Mahlin: Are you familiar with this API ? (sorry I'm still playing with different ideas)
Created attachment 105752 [details] testcase Try this snippet. I'd like to know if it is possible to solve this problem outside SWT or with a custom widget (with a native text/combo inside). Please let me know if this snippet is doing the right thing. Notes: 1. getText() returns the 'processed' version of the text 2. copy copies the 'processed' version of the text to the clipboard, etc 3. If a second Verify listener is necessary that can cause a big conflict with the first (untest)
Comment #1 mentions a 'BidiCESegmentListener(BidiComplexExpression.FILE_PATH))' Does this class actually exist (is it implemented) ? Can you send it to me. I'd like to try to write the code what would use this bidi listener for the native text. NOTE: If you tried my snippet in comment #9 on Linux you might want my modified version of TextProcessor (it fixes the alignment problem in GTK, see 236513). Ask me for more information.
NOTE: We have been doing some exploration to find out "What do the platforms do?" and "Do we need custom code to handle BIDI control characters?". Felipe has attached his explorations.
In response to comment #8: Unfortunately I am not familiar with this API. However after reading the description of this API it seems that it is not flexible enough for our purposes. Please notice that the list of separators for different types of complex expressions might be different. TextProcessor from this perspective is configurable since it has a signature of process function which accepts list of separators as argument. It seems that this API is not smart enough. Moreover, for certain types of complex expressions (i.e. XPath, SQL, Regular Expression, XML, Java code etc.) the separator need to be interpreted differently depending on the context in which it appears inside the expression. For example, a comma can be considered as a separator in many cases. However, inside comments or constants it should not be considered as a separator (i.e. XML, Java, SQL). In regular expressions, in general, Bidi text should be displayed from right to left (as it is usually displayed on Windows platform). However, inside range specification (i.e. [ABC]) it should be displayed from left to right since inside range specification each letter stands on it own and is not considered as part of a word or a longer than one letter long segment. In short, parsing of complex expression might not always be as simple as segmentation based on list of separators. In response to comment #9: The approach in the attached code seems to be: a. Introduce Unicode control characters into text buffer of Text control in order to address correct text display b. Handle dynamic text entry via adding listener to Text control and addressing events associated with copy/paste/cursor movement/deletion etc. Implementation of this approach was already suggested by Bidi centers in October 2007. I believe it was Tod who reviewed it. The major reservation was related to the use cases. Let us consider following cases: 1. GUI developer creates a stand alone Text control and does not attach any listener to it. 2. GUI developer creates a Text control, attach to it several listeners which modify the content of text and has complex logic associated with text content (i.e. code assist in JDT). The proposed solution will work fine in case 1. However it might not work at all (or even worse - ruin current implementation) in case 2. This is since order in which code associated with listeners added to Text control is called is not guaranteed. Thus the functionality of proposed solution might not work properly. Moreover, it might be mixed with already present functionality and thus also might negatively affect it. I believe that Tod's comment on this approach was as follows: "...The addition of listeners to process the typed text and then re-insert will activate other listeners. As the ordering of listeners is not deterministic this will be extremely hard to code to. ..." In response to comment #10: Yes. Those (BidiCESegmentListener & BidiComplexExpression) are implemented classes. I will send you the code.
Was there some follow up to this issue to address issues mentioned in comment #12?
This defect has not been targetted yet. Is this likely going to be fixed in Eclipse 3.5?
I'm still investigating alternative solutions for this problem. Do you know of any toolkit/library that offers a solution for this problem ?
All solutions I am aware of on the Eclipse level use the same technique discussed above. StyledText offers the best solution which I know. The same technique was implemented for fixing a similar problem on different technology. For example in Dojo.
(In reply to comment #16) > For example in Dojo. Can you send me the link ? I tried it myself but I couldn't find it.
The API is defined at: http://api.dojotoolkit.org/jsdoc/dojox/1.2/dojox.string.BidiComplex The code is available at: http://trac.dojotoolkit.org/browser/dojox/trunk/string/BidiComplex.js?rev=14184 The technique used in this implementation is the same as suggested by you in comment #9.
I'm joining the party a little late. Is the issue isolated to how the text is displayed only or is the string that the control delivers to the underlying code incorrect? That is, if I type "abc.def.hij" as a package name and it displays incorrectly as "jih.fed.cba" instead of "cba.fed.jih" but I press OK to create the package, does the actual package creation do the right thing?
(In reply to comment #19) > I'm joining the party a little late. Is the issue isolated to how the text is > displayed only or is the string that the control delivers to the underlying > code incorrect? That is, if I type "abc.def.hij" as a package name and it > displays incorrectly as "jih.fed.cba" instead of "cba.fed.jih" but I press OK > to create the package, does the actual package creation do the right thing? Yes - it will shows up right in the package view. Provide that the view insert bidi controls characters before displaying the text, which in this case (static text) can be done easily using TextProcessor. The application, or whomever insert the bidi controls characters, have to stripout the controls characters before creating the file in the filesystem.
Indeed the issue is only with text display. However, it should be emphasised that it is not a matter of mere convenience. In many cases (even if what is displayed in the input field is a package name, while in general it can be XPath expression with much more complex syntax) incorrect display may make the user task of working with the text impossible since incorrect display makes the text incomprehensible. Here are a couple of examples (notice, that capital letter stand for Bidi chars): ABC.DEF.ibm.com ABC.DEF.IBMmine.com The display of the patterns above will be as follows: FED.CBA.ibm.com In this case the relative order of subpackages in the hierarchy is violated. MBI.FED.CBAmine.com In this case, not only the relative order of subpackages names is violated but the integrity of single subpackage name is not preserved (instead of MBImine and CBA we have MBI and CBAmine). Please notice that having both English and Arabic/Hebrew text in the same pattern is very common. This is why the text is called by the way BI-directional. Meaning it includes portion of text which follows left-to-right direction and other portion which follows right-to-left direction.
If we're serious about this shouldn't we also be fixing how TextProcessor works by removing the "peformance optimization" that cripples it in non-rtl locales? static { Locale locale = Locale.getDefault(); String lang = locale.getLanguage(); if ("iw".equals(lang) || "he".equals(lang) || "ar".equals(lang) || "fa".equals(lang) || "ur".equals(lang)) { //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ String osName = System.getProperty("os.name").toLowerCase(); //$NON-NLS-1$ if (osName.startsWith("windows") || osName.startsWith("linux")) { //$NON-NLS-1$ //$NON-NLS-2$ IS_PROCESSING_NEEDED = true; } } } Note that it only gets enabled in a small number of locales.
>Note that it only gets enabled in a small number of locales. Well that's the point/idea of the current implementation. Anyway, if you see a problem with that then please file a new bug regarding the TextProcessor.
There is no need to open a separate defect. It was already opened. Please see bug 179724 for details.
The solution for this issue is based on the assumption that there is a generic extendable framework of parsers which can handle tokenization of text having internal structure (i.e. SQL query, file path, XML content etc.). Such generic framework was already developed and was published via bug 183164. This framework should be leveraged in order to address all types of known complex expressions.
Tomer: Is the current bidi segments API that is available in StyledText sufficient for you to handle all different types of complex expression you need to support (Bug 183164)? Note, the current bidi segments support in StyledText is being challenged in Bug 241482.
Bidi segments API that is available in StyledText seems to use hardcoded Unicode control character - LRM. This control character is appropriate for addressing complex expressions with LTR direction of syntax (i.e. Java code, file path etc.). Since overwhelming majority of complex expressions types have LTR direction of syntax, indeed, current capabilities of bidi segments API in StyledText are sufficient. Having said that, there are types of complex expressions with syntax having RTL direction. To properly address them RLM (instead of LRM) Unicode control character should be used. Please notice that the package I attached to bug 183164 does address such types of complex expressions for static cases. To make sure that dynamic cases are covered as well (and this bug (bug 230854) is about dynamic cases) indeed, the Bidi segments API in StyledText should be extended to allow explicit specification of Unicode control character to be used for separating successive segments (exactly what bug 241482 is talking about). However, I would not block resolution of this bug (bug 230854) till bug 241482 (talking about extension of Bidi segments API in StyledText) is solved, since even without extension of Bidi segments API in StyledText, resolution of this bug (bug 230854) will cover more than 95 % of problematic cases. Please notice that extension suggested in bug 241482 was not primarily aimed at complex expressions. Nevertheless it is applicable for them (even though for insignificant amount of cases).
(In reply to comment #27) > Bidi segments API that is available in StyledText seems to use hardcoded > Unicode control character - LRM. This control character is appropriate for > addressing complex expressions with LTR direction of syntax (i.e. Java code, > file path etc.). Since overwhelming majority of complex expressions types have > LTR direction of syntax, indeed, current capabilities of bidi segments API in > StyledText are sufficient. StyledText is capable of addressing both LTR and RTL directions of syntax already. Bidi segments support in StyledText involve 2 control character - LRM and RLM, for LTR and RTL base direction respectively. > Please notice that extension suggested in bug 241482 was not primarily aimed > at complex expressions. Nevertheless it is applicable for them (even though > for insignificant amount of cases). Bug 241482 aims to address flexible presentation of various expressions, including complex expressions in the first place. However, it didn't imply introducing the computation logic for control characters insertiion points - this was still meant to be client responsibility.
... And of course those clients would notably benefit from resolution of bug 183164.
(In reply to comment #27) > However, I would not block resolution of this bug (bug 230854) till bug 241482 > (talking about extension of Bidi segments API in StyledText) is solved, since > even without extension of Bidi segments API in StyledText, resolution of this > bug (bug 230854) will cover more than 95 % of problematic cases. Why would I add a new API that I know it will fail for 5% the cases ? If we add this new API to StyledText (Bug 241482), we can assume that soon or later somebody will ask the same support for Text and Combo. Question: Do you need support only for single line text input widgets ? The multi line text widget might require special handling.
I believe there were 3 points in last comment: 1. According to comment 28 currently StyledText supports both LTR and RTL syntax directions (via LRM and RLM) and thus we don't depend on bug 241482 (talking about extension of Bidi segments API in StyledText) for resolving dynamic problem of complex expressions using StyledText and general parsing framework for complex expressions (bug 183164). 2. I would prefer that the scope of new API to StyledText (Bug 241482) is discussed in bug 241482 since currently bug 241482 and this one are completely independent. 3. We need support for single line input fields. The only contexts I am aware of in which we have problem of complex expressions in multi line text widgets are Java / XML code editors which are based on StyledText widget. The solution in those cases is integrated into the code of editors themselves. For example for Java code editor the solution was provided via bug 106638 and bug 229226 .
Hi Felipe, As you mentioned in comment https://bugs.eclipse.org/bugs/show_bug.cgi?id=241482#c17?, we have to add a new class offering BidiSegmentEvent functionality to org.eclipse.swt.events (and perform some consequent refactoring on BidiSegmentEvent itself). Can I go ahead with such a change? Have you decided regarding the name of the new event?
(In reply to comment #32) > Can I go ahead with such a change? Yes > Have you decided regarding the name of the new event? No, use any name you like, if needed we can change the name easily before we release the changes.
Created attachment 157717 [details] SegmentEvent patch Hi Felipe, this is a tiny patch for SegmentEvent/BidiSegmentEvent only. Please see if it matches your expectations. _____ It seems that we will also need a new SegmentListener class to be used by Text. And both SegmentListener and BidiSegmentListener should probably extend SWTEventListener directly (without immediate relationship between the BidiSegmentListener and "just" SegmentListener)
(In reply to comment #34) > Created an attachment (id=157717) [details] > SegmentEvent patch > Hi Felipe, this is a tiny patch for SegmentEvent/BidiSegmentEvent only. Please > see if it matches your expectations. That is fine, I not sure about moving lineOffset up because it is a StyledText concept. It all depends if multiline text need to support segments listener. We already have a text (type String) in Event, so maybe we can use that instead of adding a new lineText field. In BidiSegmentEvent both fields would be always set (text==lineText). > _____ > It seems that we will also need a new SegmentListener class to be used by Text. > And both SegmentListener and BidiSegmentListener should probably extend > SWTEventListener directly (without immediate relationship between the > BidiSegmentListener and "just" SegmentListener) Yes. I think so.
(In reply to comment #35) > That is fine, I not sure about moving lineOffset up because it is a StyledText > concept. It all depends if multiline text need to support segments listener. I believe we need to support multiline text. However, that's a matter of interpretation to some degree. Assuming that multiline (or just large amount of text) is not that broadly used in Text, we can process all the content at once (i.e. multiline as a single-line) without significant penalty. What do you think? > We already have a text (type String) in Event, so maybe we can use that > instead of adding a new lineText field. In BidiSegmentEvent both fields would > be always set (text==lineText). It seems to me that (Bidi)SegmentEvent doesn't extend Event. (BTW increasing the visibility of the constructor in the patch was completely accidental, will revert back to the existing code.)
(In reply to comment #36) > single-line) without significant penalty. What do you think? Yes, I like the idea. > It seems to me that (Bidi)SegmentEvent doesn't extend Event. No, it does not. Event is "untyped" event, SegmentEvent is the "typed" event. For example: control.addListener(SWT.MouseDown, listener); //untyped control.addMouseListener(mouseListener); //typed If we decide to have the same design for the new event you are adding, then you want the same name for the fields in the typed event and in the untyped event. Event already has text in it, so it makes sense to add "text" to the typed event (SegmentEvent). See TypedListener and StyledTextListener. Don't worry about it right now, I'm not sure if we should/can have an untype listener for the new event.
Created attachment 158504 [details] Preliminary patch for Text These are very preliminary changes for win32 only (in the Text part). I am posting them though for initial feedback. Please let me know if I should change the strategy radically, revisit some parts of the implementation, go on with others, etc... The major difference between the approach taken for Text as opposed to that in StyledText, is that we introduce segment characters to the buffer, in order to let such operations as selection, caret movement and positioning be handled natively (with the exception of some adjustments in presence of segments). Segments support should be transparent to clients, so I think we should follow the convention that all API retrieves or communicates data with respect to unsegmented (deprocessed) text. E.g. getSelection would return "untranslated" selection point. setSelection would pass selection offsets also WRT to the unsegmented text (and then Text implementation will adjust selection points as needed internally).
Created attachment 158513 [details] Visual text example implemented via segments Canonical visual text widget (without 3270 functionality, but can be used for 5250 terminal emulation).
(In reply to comment #38) > Created an attachment (id=158504) [details] > Preliminary patch for Text I would like to specify a list of items that are still missing or need to be modified in my opinion: 1. Case |!OS.IsUnicode && OS.IsDBLocale| was not addressed. I was not sure whether we should enable segments in a non-Unicode mode at all, since anyway, clients will be able to use quite a limited range of segmentChars in that mode. 2. Instead of getText(boolean) and setText(boolean) it's may be preferable to introduce a boolean member variable intended to control various operations (e.g. selection related ones, getText, setText), so that they be applied either to segmented or unsegmented text, as needed. 3. Message text is not handled yet. 4. Some public methods have not yet been adapted to accommodate segments support, e.g.: - getCaretLocation - getCharCount - setOrientation (and listening for the associated hotkey on Windows) 5. Left/right key processing: I am not sure whether it should be refined or not. Probably not, since in case of additional key listeners, it may be a right approach to operate as at present: normalize text, let all other handlers to process the key, and then revert back to the segmented text. 6. Double buffering - need to force it when segmented text is in a deprocessed state. 7. I am going to give another try to sending EM_REPLACESEL message instead of using setText (OS.SetWindowText). So far (without double buffering) it used to cause noticeable text flickering.
Created attachment 158881 [details] Slightly sanitized win32 Text patch I added segments processing on setOrientation/Ctrl+Shift key press, tried to cover additional public methods, and made some misc. code clean up.
Created attachment 158882 [details] Another visual text example This test case addressed both left-to-right and right-to-left base text directions. The immediate effect of segments is exposed in LTR with Hebrew/Arabic text (which will progress from left to right) and in RTL with English text (which will progress from right to left).
Hi Lina, I checked your patch last Friday. As you know, it still has a number of problems and it is somewhat far from being ready. The big problem I found in your patch is that the bidi segments are applied asynchronously, in a post event. I'm not sure we can win with this strategy. For one thing, it must flash as you allows the text be render without the segments and then you add the segments causing the control to draw again. You will also have all classic problems caused by doing work asynchronously, for example: time 0 - the control has some text and some segments in it. time 1 - setText(newText) is called. time 2 - getText() is called, you have the newText but the segments are old. You won't be able use getNormalizedText().
(In reply to comment #43) Felipe, thanks a lot for the feedback. Yes, I know there are many problems. First of all, the painting issue which I am aware of and am trying to resolve right now. However, I don't think that applying segments asynchronously would cause those classic problems, because no matter how text content and text segments are up-to-date, they should be mutually synchronized always.. - setText updates text and segments synchronously, - before posting the asynchronous segment event the text is deprocessed and segments are removed synchronously too. > time 0 - the control has some text and some segments in it. > time 1 - setText(newText) is called. Here SegmentEvent is send and text and segments get updated synchronously, > time 2 - getText() is called, you have the newText but the segments are old. > You won't be able use getNormalizedText(). I don't think so, the segments and text should be updated (and even if the segments are old, they are still in sync with the text).
I realized that setText in the patch calls sendEvent(LineGetSegments) *after* OS.SetWindowText and the text update doesn't have any effect. This is totally unintentional - sendEvent(LineGetSegments) was supposed to take place before setting text into the control. (I changed this locally but this change is not yet reflected in the patch.)
I tested a bit more, here are some bugs that need fixing: public static void main (String[] args) { final Display display = new Display(); final Shell shell = new Shell(display); shell.setLayout(new GridLayout()); final Text text = new Text (shell, SWT.SINGLE); text.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); // text.getCharCount(); //Bug 1 - Causes NPE text.addSegmentListener(new SegmentListener() { public void lineGetSegments(SegmentEvent event) { int length = event.lineText.length(); event.segments = new int [] {0, length}; event.segmentsChars = new char [] {'\u202E', '\u202C'}; } }); text.setText("latin"); display.addFilter(SWT.KeyDown, new Listener() { public void handleEvent(Event event) { if (event.keyCode == SWT.F1) { String oldText = text.getText(); System.out.println(oldText + " " + oldText.length() + " " + text.getCharCount()); //Bug 4 - before arrow key: oldText.length() != text.getCharCount(); after the 'l' goes missing text.setText("hi"); oldText = text.getText(); System.out.println(oldText + " " + oldText.length() + " " + text.getCharCount()); //bug 5 - set "hi" but "i" is returned //bug 6 - text.getCharCount() returns zero } } }); shell.setSize(300, 300); shell.open();//Bug 2 - text start wrong, shows latin instead of nital //Bug 3 - hit a arrow key, the text is reorder but the 'l' goes missing while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); }
(In reply to comment #46) Felipe, Sorry, I haven't yet tested the API thoroughly thinking that first of all I need to have your confirmation on the principal strategy. All the problems referenced in comment 46 (except for the NPE of course) are consequences of the glitch mentioned in comment 45 and they are fixed now. These problems do not lay with the way the event is emitted (synchronously or not).
(In reply to comment #47) > Sorry, I haven't yet tested the API thoroughly thinking that first of all I > need to have your confirmation on the principal strategy. read comment #34, I think applying the segments asynchronously is a bad idea - you will be chasing bugs for a long time. > All the problems referenced in comment 46 (except for the NPE of course) are > consequences of the glitch mentioned in comment 45 and they are fixed now. > These problems do not lay with the way the event is emitted (synchronously or > not). Yes, they do. In Text#setText(), if you replace sendEvent() by postEvent() you get all the bugs back, right ? sendEvent() -> synchronous -> no bugs in setText() postEvent() -> asynchronous -> bugs in setText() The question is: what other places would you have the same scenario ? Text#append() and Text#paste() for sure. Most likely you will also need it in WM_CHAR and/or EN_CHANGE. Do you agree ? If you need, I can write another snippet exposing more bugs in the code.
(In reply to comment #48) > (In reply to comment #47) > > Sorry, I haven't yet tested the API thoroughly thinking that first of all I > > need to have your confirmation on the principal strategy. > read comment #34, I think applying the segments asynchronously is a bad idea - > you will be chasing bugs for a long time. > > All the problems referenced in comment 46 (except for the NPE of course) are > > consequences of the glitch mentioned in comment 45 and they are fixed now. > > These problems do not lay with the way the event is emitted (synchronously or > > not). > Yes, they do. > In Text#setText(), if you replace sendEvent() by postEvent() you get all the > bugs back, right ? > sendEvent() -> synchronous -> no bugs in setText() > postEvent() -> asynchronous -> bugs in setText() In Text#setText() there was initially sendEvent(), not postEvent(). It just has no effect on the Text content because it got called too late. > The question is: what other places would you have the same scenario ? > Text#append() and Text#paste() for sure. Most likely you will also need it in > WM_CHAR and/or EN_CHANGE. > Do you agree ? > If you need, I can write another snippet exposing more bugs in the code. I haven't faced problems in these places so far. However, I have a previous version of the patch that only uses sendEvent(). After better testing of this patch and making some corrections I would like to upload it for your feedback.
Created attachment 159820 [details] Preliminary patch for Text II Felipe, please find another version of the patch. It's still a preliminary version. It has at least some problems mentioned in comment 40. Some code refactoring would also be required.
I see now that segments should be addressed in win32 Text#windowProc rather than than in callWindowProc.
(In reply to comment #48) > The question is: what other places would you have the same scenario ? > Text#append() and Text#paste() for sure. Most likely you will also need it in > WM_CHAR and/or EN_CHANGE. > I don't think there was a problem though. Here is a scheme according to which it used to work, e.g. for Text#append(): A. Prior to the append operation text gets deprocessed. As a result: getText = normalized text, segments = null; B. SegmentEvent got appended to the message queue (within the same thread); C. Actual append (on a deprocessed text) is performed; D. Next message (either sync or async - if any) is processed; ........ Z. SegmentEvent that was posted in step B is handled, while the text is still in a normalized form.
Created attachment 160175 [details] Preliminary patch for Text II - modified Modified patch.
Created attachment 160176 [details] Another testcase Testcase given in Comment 46 with some addition
Created attachment 160483 [details] Another version of win32 patch Another version of win32 patch that manipulates segments via sending EM_REPLACESEL message.
I tested patch in comment 53, here are my notes: I tested using my snippet in comment 46. TextSegmentListener should not exist (you just need to change TypedListener to handle the new event). LineGetSegments should to to SWT. Renamed it to something else ? do you really need to keep a Clipboad around all the time? NPE in windowProc WM_UNDO (Ctrl+Z) Ctrl+V is not working Do you need all these different types of check before doing bidi work ? if (hooks(LineGetSegments) || filters(LineGetSegments)) if (segments == null || segments.length == 0) if (segments != null) if (!ignoreSegments && segments != null) if (!ignoreSegments) if (!ignoreSegments && (hooks(LineGetSegments) || filters(LineGetSegments))) I think you should be able to remove the ignoreSegments flag, it makes the code a lot more messy. In callWindowProc and processText you can replace getText() by GetWindowText(). In getCaretLocation(), you can call position = translateOffset(position) When the text is empty text.getCharCount() still returns -2 The text is very flashy, use my snippet and enter a long text (20 chars), hold left or arrow key down.
(In reply to comment #56) > I tested patch in comment 53, here are my notes: > I tested using my snippet in comment 46. > TextSegmentListener should not exist (you just need to change TypedListener to > handle the new event). I extracted the inline implementation into a separate class so the changed TypedListener can be accessible to other controls. > LineGetSegments should to to SWT. Renamed it to something else ? I don't know, please decide. > do you really need to keep a Clipboad around all the time? Probably not. However, it is not kept around if no copy takes place. > NPE in windowProc WM_UNDO (Ctrl+Z) > Ctrl+V is not working These should be fixed in the patch from comment 55. > Do you need all these different types of check before doing bidi work ? > if (hooks(LineGetSegments) || filters(LineGetSegments)) > if (segments == null || segments.length == 0) > if (segments != null) > if (!ignoreSegments && segments != null) > if (!ignoreSegments) > if (!ignoreSegments && (hooks(LineGetSegments) || filters(LineGetSegments))) > I think you should be able to remove the ignoreSegments flag, it makes the > code a lot more messy. > In callWindowProc and processText you can replace getText() by GetWindowText(). > In getCaretLocation(), you can call position = translateOffset(position) Basically ignoreSegments flag was to eliminate 2 subsequent (and both redundant) calls for translate and untranslate. > When the text is empty text.getCharCount() still returns -2 I think this is fixed in the patch referenced in comment 55. > The text is very flashy, use my snippet and enter a long text (20 chars), hold > left or arrow key down. Yes, this is the biggest problem. I do not see any visible difference here between the implementations using sendEvent and postEvent. Probably because paint itself occurs asynchronuosly and Text manages to get painted when the segments are being cleared temporarily. As a workaround I used setRedraw (false before clearance and true after apply). It helps but drawing becomes too slow. Another workaround may be setting text color to background color and restoring it back in clear/apply segments respectively.
Created attachment 160652 [details] Another version of win32 patch Please review this modified patch. It contains the following changes (compared to the previous version): 1. Moved LineGetSegments to SWT. Renamed it to GetSegments. 2. Discarded setSegments and setSegmentsChars methods. 3. Used WM_SETREDRAW message to reduce the flashing. I am still looking for solutions for EM_UNDO message handling. Regarding multiple types of check before doing bidi work, I think this can be justified. E.g. before stripping segments we need to know if segments != null. And before sending a segment event we need to know whether we are listening or filtering segments (regardless of presence of actual segments).
Created attachment 160692 [details] Test_org_eclipse_swt_widgets_Text addition I added segments test to Test_org_eclipse_swt_widgets_Text.
Created attachment 160693 [details] Changes for Eclipse SWT/win32/org/eclipse/swt/widgets/Text.java Proposed changes for Eclipse SWT/win32/org/eclipse/swt/widgets/Text.java only by results of the test.
Created attachment 160984 [details] Patch - review of the event framework note: always put all the code in the patch (last patch had only changes to Text) Deleted TextEvent, moved fields to Event (why was TextEvent public?) Deleted TextSegmentListener, moved functionality to TypedListener. (why was it public?) SegmentListener move to org.eclipse.swt.events Rename textLine to text in SegmentEvent (put textLine back to BidiSegementEvent) ? The name of the field should be the same in the untyped event and in the typed event. Text should not use SegmentEvent, do all the work using Event instead API (subject to change) SWT#GetSegments SegmentEvent { String text; int [] segments; char [] segmentsChars; } SegmentListener { lineGetSegments(SegmentEvent) } Text#addSegmentListener(SegmentListener) Text#removeSegmentListener(SegmentListener) //------------------------------ Note: For future reference, if you need to add a new event to the toolkit just look at an exist event and do the same.
I started reviewing the changes for Text. Here are some of my notes so far: Try to minimize the use of SWT public API from our internal implementation (reduce overhead). Do not overwrite dispose(), use releaseWidget() Why can't handle WM_COPY the same way you handle WM_CUT ? That way you could remove your reference for Clipboard. Note, alternatively, instead of using Clipboard you could have implemented setClipboardText(). Similar to Control#getClipboardText() During windowProc(), why set processSegments=true for VK_LEFT and VK_RIGHT whithout hooks for GetSegments ? The use of set redraw: You always need to make sure that redraw is turn back on after turning it off. In your code, it is easy to run in cases where you turn it off but you don't turn it back on. For example: Add a verify listener that sets doit to false in some case and call append with that case. Win32 has some bugs in WM_SETREDRAW, see Text#setRedraw() and Control#setRedraw(), make sure it is okay for you to call WM_SETREDRAW directly. Also, having set redraw off in clearSegments() and set redraw on in applySegments() is confusing, I always like to see the two being called in the same place. Undo is completely broken. Please, remove that ignoreSegments flag, you can do without it. You have two ways to insert control characters in the edit (you should have only one): 1) clearSegments()/applySegments() (uses EM_REPLACESEL) - used by append/insert/windowProc 2) getSegmentsText() (adds to the char buffer) - used by setText() I kind of like option 2, you only calls the OS once with the correct data. But I see why you need option (1), specially during windowProc(). I find option (1) has a huge overhead. See the case for Text#append(): between [ ] are normal calls, everything else is segments overhead: append() clearSegments() getSelection() getText() redraw off removes segments from OS using EM_SETSEL/EM_REPLACESEL setSelection() [verify] [EM_SETSEL] [EM_REPLACESEL] [EM_SCROLLCARET] getText() - again getSegments() applySegments() getSelection() - again sets segments in OS using EM_SETSEL/EM_REPLACESEL redraw on InvalidateRect - not needed This same scenario repeats itself in windowProc() for every WM_KEYDOWN (and some other messages). I'd never accept that for normal case, but I guess for bidi+segments it is okay...
Created attachment 161174 [details] Updated win32 Text patch Includes Event framework patch (comment 61) and changes per comment 62
(In reply to comment #62) > Why can't handle WM_COPY the same way you handle WM_CUT ? > That way you could remove your reference for Clipboard. > Note, alternatively, instead of using Clipboard you could have implemented > setClipboardText(). Similar to Control#getClipboardText() I changed to handle in the same way. The difference however is that WM_CUT, in contrast with WM_COPY, updates the buffer, so WM_CUT really requires segments adjustment. > During windowProc(), why set processSegments=true for VK_LEFT and VK_RIGHT > whithout hooks for GetSegments ? It checks if segments != null instead. If hooks but no segments, nothing to remove/add.. > The use of set redraw: > You always need to make sure that redraw is turn back on after turning it off. > In your code, it is easy to run in cases where you turn it off but you don't > turn it back on. For example: > Add a verify listener that sets doit to false in some case and call append > with that case. Yes. And sorry - it was not yet fixed in the patch I just sent. > Win32 has some bugs in WM_SETREDRAW, see Text#setRedraw() and > Control#setRedraw(), make sure it is okay for you to call WM_SETREDRAW > directly. Since after redraw was turned on setSelection is called, I think this should be okay. > Also, having set redraw off in clearSegments() and set redraw on in > applySegments() is confusing, I always like to see the two being called in the > same place. I will move it. Sorry - forgot to > Undo is completely broken. Yes, it is still broken, I am looking at it > Please, remove that ignoreSegments flag, you can do without it. Done. > You have two ways to insert control characters in the edit (you should have > only one): > 1) clearSegments()/applySegments() (uses EM_REPLACESEL) - used by > append/insert/windowProc > 2) getSegmentsText() (adds to the char buffer) - used by setText() > I kind of like option 2, you only calls the OS once with the correct data. > But I see why you need option (1), specially during windowProc(). > I find option (1) has a huge overhead. I tried to reduce the overhead.
(In reply to comment #64) > (In reply to comment #62) > > In your code, it is easy to run in cases where you turn it off but you don't > > turn it back on. For example: > > Add a verify listener that sets doit to false in some case and call append > > with that case. > Yes. And sorry - it was not yet fixed in the patch I just sent. No, I hope this WAS fixed in the patch from comment 63.
1- call clearSegments() from removeSegmentListener() if (hooks(SWT.GetSegments) && filters(SWT.GetSegments))==false ? => What happens if we have more than one SegmentListener ? 2- Call applySegments() from addSegmentListener() ? -> Note: StyledText has all the sames problems, we should be consistent and fail the same way IMO 3- Is WM_SETREDRAW off/on really helping ? cause it is still broken: add SWT.GetSegments that does nothing (or sets segments with a empty array), clearSegments() does not call WM_SETREDRAW FALSE but applySegments does call WM_SETREDRAW TRUE. 4 - clearSegments()/applySegments() used to call EM_REPLACESEL several time, you changed to call it just once, why ? 5 - add if (segments!=null) in getCaretLocation()/getCaretPosition()/getPosition() it doesn't change the result, but you used the test in all other places. Note: the code is getting alot better.
(In reply to comment #66) > 1- call clearSegments() from removeSegmentListener() if (hooks(SWT.GetSegments) > && filters(SWT.GetSegments))==false > ? I think yes and immediately call applySegments() ? > => What happens if we have more than one SegmentListener ? This should be okay if we call applySegments(). Do you prefer to disallow more than one SegmentListener ? > 2- Call applySegments() from addSegmentListener() ? Yes but before that call cleanSegments(). What do you think ? > -> Note: StyledText has all the sames problems, we should be consistent and > fail the same way IMO > 3- Is WM_SETREDRAW off/on really helping ? > cause it is still broken: add SWT.GetSegments that does nothing (or sets > segments with a empty array), clearSegments() does not call WM_SETREDRAW FALSE > but applySegments does call WM_SETREDRAW TRUE. Yes. Would it be okay if clearSegments() returned a boolean value indicating if the segments got removed indeed? > 4 - clearSegments()/applySegments() used to call EM_REPLACESEL several time, > you changed to call it just once, why ? This was done to (1) reduce the segments overhead, (2) for the Undo work. Only the last EM_REPLACESEL can be undone. And if EM_REPLACESEL is called straight off at once, the entire buffer change will be undoable. > 5 - add if (segments!=null) in > getCaretLocation()/getCaretPosition()/getPosition() > it doesn't change the result, but you used the test in all other places. > Note: the code is getting alot better.
Test case for add/remove SegmentListener (more cases can be added).. final Display display = new Display(); final Shell shell = new Shell(display); shell.setLayout(new GridLayout()); final Text text = new Text(shell, SWT.SINGLE); text.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); text.setText("abc."); final SegmentListener sl1 = new SegmentListener() { public void lineGetSegments(SegmentEvent event) { event.segments = new int [] {0}; event.segmentsChars = new char [] {'\u202e'}; } }; final SegmentListener sl2 = new SegmentListener() { public void lineGetSegments(SegmentEvent event) { event.segments = new int [] {0}; event.segmentsChars = new char [] {'\u202b'}; } }; try { text.addSegmentListener(null); } catch (IllegalArgumentException e) {} text.addSegmentListener(sl1); // result = ".cba" ? display.addFilter(SWT.KeyDown, new Listener() { public void handleEvent(Event event) { if (event.keyCode == SWT.F1) { text.addSegmentListener(sl2); // result = ".abc" ? } else if (event.keyCode == SWT.F2) { text.removeSegmentListener(sl2); // result = ".cba" } else if (event.keyCode == SWT.F3) { text.removeSegmentListener(sl1); // result = "abc." } } }); shell.setSize(300, 300); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose();
I noticed some problem. Here is a snippet: final Text text = new Text(shell, SWT.SINGLE); text.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); text.addSegmentListener(new SegmentListener() { public void lineGetSegments(SegmentEvent event) { event.segments = new int[] {0, event.text.length()}; event.segmentsChars = new char[] {'\u202e', '\u202c'}; } }); text.setText("latin"); display.addFilter(SWT.KeyDown, new Listener() { public void handleEvent(Event event) { if (event.keyCode == SWT.BS) { text.append(" text"); event.doit = true; display.asyncExec(new Runnable() { public void run() { System.out.println("actual: " + text.getText() + " " + text.getCharCount() + "expected: latin tex 9"); // "actual: latin text 10" } }); } } }); Here 2 operations that require segments treatment (Text#append and SWT.BS key press) are invoked subsequently as a reaction to a single backspace key press. However, only in the first (append) call segments are handled appropriately, while in the second, the default KeyDown handler doesn't address them at all. As a result: 1. Before SWT.BS, segments are not cleared and the backspace gets applied on the last character in the segmented text ('\u202c'), instead of the last character of the normalized text ('t'). 2. After SWT.BS, segments are added to a segmented, not clean, text. (This second problem can be easily fixed though - by adding clearSegments call to applySegments, preceding the actual apply operation). To fix these problems, I think we can do one of the following: 1. In addition to windowProc, handle segments also in callWindowProc. This way, e.g. SWT.BS from the above example will be addressed in callWindowProc. 2. Use post SWT.GetSegments event, applying segments only after the entire execution chain associated with a primary event is handled. 3. Use 'ignoreSegments' flag to omit segment processing as needed..
Regarding the post event option, I'd like to add that the event queue will be examined for the presence of previously posted segment event. If such an event is present, the new event won't be posted.
Created attachment 162293 [details] Updated win32 Text patch with trial changes for undo/redo Modified patch. 1. Attempted to fix the problem described in comment 69 via clearing segments in callWindowProc as needed. 2. Added clear/apply segments to Text#addSegmentListener and Text#removeSegmentListener 3. Added trial changes for undo/redo functionality: after some undoable operation takes place, send two subsequent WM_UNDO messages and cache the state (text content and selection) after each. When an actual undo should be performed, load one of the previously saved states. This method is a bit tricky, but I do not see a better alternative at this point..
Created attachment 163180 [details] win32 Text patch with trial undo changes - another version There is still a problem with undo - because of sending the WM_UNDO simulation messages, the native undo buffer gets reset and loses ability to accumulate undoable changes incrementally. E.g., after 2 consecutive character deletions undo should revert both deleted characters, while it only reverts the last character deleted. I tried to rewrite this part resulting in totally home-grown code for undo..
Lina, is undo is only known problem left in the code ?
(In reply to comment #73) Felipe, yes: undo is the only known problem for now.
(In reply to comment #66) > => What happens if we have more than one SegmentListener ? Perhaps we should update the Event fields segment/segmentsChars before sending the event. This way, an application will know the current segments status and will be able to coordinate several |SegmentListener|s working in conjunction.
Felipe, looks like a bug opened by our Symphony team has a dependency on this bug. Is this on the table for 3.7?
(In reply to comment #76) > Felipe, looks like a bug opened by our Symphony team has a dependency on this > bug. Is this on the table for 3.7? It was in the plan for 3.6, but is was deferred. I didn't see a work item for this bug in the plan for 3.7.
I see that the 3.7 plan contains the bullet "Improvements to BIDI support" which includes this bug (230854).
Thank you!
Created attachment 178628 [details] win32 patch including Combo Sorry for the long idle.. I attach a new version of win32 patch with preliminary changes for Combo widget. Other than Text and Combo, changes are kept intact. For Text widget, modifications include: - reducing persistence of segmentsChars, no longer keeping them in Text. segmentsChars are only necessary when computing segments text. (Nevertheless, storing segmentsChars and communicating them in SegmentEvent may have some value for callers, should they consult segmentsChars that had been added previously.) - setText now uses clearSegments/applySegments scheme. Segments are cleared before SetWindowText is called and are applied only after the new text value is adopted (possibly with some transformations) natively. In the Combo part, I tried to address segments in both edit component and list box items. Changes for edit control are pretty similar to those in Text. In list box, segments are only needed for deprocessing items' text (in contrast with Edit which makes multipurpose usage of segments). There are several possibilities of caching information on the item segments. Currently segments are stored as array of integers at item level.
Created attachment 178635 [details] Combo example Manual test for Combo. Based on the test results so far, text limit needs to be adjusted on CB_RESETCONTENT (accompanying e.g. Combo#removeAll() ).
Created attachment 179537 [details] Initial GTK Text patch Trial patch containing cross-platform changes (included with earlier uploads) and changes specific to GTK. Event handling associated with segments should be probably incorporated into the common event framework, but for the time being these changes are isolated to a particular area of code to facilitate review. Felipe, I will truly appreciate your feedback.
Created attachment 179540 [details] Initial GTK Text patch
Created attachment 179583 [details] Initial GTK Text patch - including Text and Combo Updated GTK patch with changes for Text and Combo. Apparently, it still needs some reorganization, at least in the area of event handling.
Felipe is on vacation until Friday next week. He will review the patches when he is back.
Created attachment 180564 [details] win32 and gtk patch I merged win32 and gtk changes into a single patch. Please note that undo on win32 still doesn't work well. If having home-grown code here is acceptable, I will try to provide some improvements in that area.
Created attachment 185633 [details] Updated win32 + gtk patch Patch is synced with the trunk and contains some updates in the GTK part.
For gtk, I am also adding handling for the "direction-changed" signal, which is emitted e.g. when the setOrientation method is called. I will include these changes with a next bunch of updates.
Linna, can you attach the patch again. I tried to use the last patch you added and it was missing code (TypedListener for example, I think it could be missing more).
Lina, the code that creates the callbacks in GTK is still wrong. Can the signals you need be handled like any other signal ? Please remove the special processing for the segmentsProcs if possible.
Felipe, I didn't want to instantiate callbacks, unless they are really needed in bidi scenarios. Also, I preferred not to use GClosures-based approach, as this would significantly increase a number of callbacks (7 signals * 2 [before and after] = 14 callbacks - vs. 4 callbacks as at present).
Sorry, new closures could reuse existing callbacks. I recalled that the main reason for not using GClosures was inability to determine whether a call occurred *before* or *after* handling an event by default handlers. Anyway, I'll look at this part again.
(In reply to comment #91) > Felipe, I didn't want to instantiate callbacks, unless they are really needed > in bidi scenarios. You don't need to create callbacks at all (just use the ones that are there already). (looking at comment 92 I think you got the idea). > Also, I preferred not to use GClosures-based approach, as this would > significantly increase a number of callbacks (7 signals * 2 [before and after] > = 14 callbacks - vs. 4 callbacks as at present). The last patch you create 4 callbacks *for each* text and combo there are, it will be a problem for sure. >I recalled that the main reason for not using GClosures was inability to >determine whether a call occurred *before* or *after* handling an event by >default handlers. Not true, we done that before. Check out BUTTON_PRESS_EVENT and BUTTON_PRESS_EVENT_INVERSE.
(In reply to comment #93) > The last patch you create 4 callbacks *for each* text and combo there are, it > will be a problem for sure. Callbacks got created by Text or Combo, but added to *Display* (in case they were not there yet). > >I recalled that the main reason for not using GClosures was inability to > >determine whether a call occurred *before* or *after* handling an event by > >default handlers. > Not true, we done that before. Check out BUTTON_PRESS_EVENT and > BUTTON_PRESS_EVENT_INVERSE. I will check that out, thanks for the pointers.
(In reply to comment #94) > Callbacks got created by Text or Combo, but added to *Display* (in case they > were not there yet). I see, you are correct. But the point is: we have almost 70 signal in GTK and they all follow the same pattern. To implement your code you had to add 4 signas and you designed a whole new way for doing that. Imagine that everyone that works in the code comes up with a new design to work with signals - what would that code look after a few years...
(In reply to comment #95) > But the point is: we have almost 70 signal in GTK and they all follow the same > pattern. To implement your code you had to add 4 signas and you designed a > whole new way for doing that. Imagine that everyone that works in the code > comes up with a new design to work with signals - what would that code look > after a few years... Felipe, I understand that the same pattern should be followed. I did try to do so (please see //closures [Widget.MOVE_CURSOR] = OS.g_cclosure_new (windowProc5, Widget.MOVE_CURSOR, 0); line accidentally remained as a result of this attempt). However, this didn't work for me. Likely I was doing something wrong. I will go though the gtk changes again and will let you know of the result by the beginning of the next week.
Created attachment 188252 [details] win32 patch Felipe, here is a patch with win32 changes only + common code.
First review (didn't review Combo, just Text). javadoc wrong in SegmentEvent/SegmentListener, missing or wrong @since, some descriptions are wrong too What is SWT.RemoveSegments ? Garbage ? --callWindowProc/windowProc You have code in windowProc, then you have some more similar code in callWindowProc. I find it confusing. I don't think the code in callWindowProc is doing anything. I believe windowProc is the only caller (calling with msg==KEY|CHAR), and it already scopes out the segments before callWindowProc. So callWindowProc() never has any work to do. Please let me know what cases the code in callWindowProc is fixing. in windowProc, why did you change: if (msg == OS.EM_UNDO) { ... } if (msg == Display.SWT_RESTORECARET) { ... } to: if (msg == OS.EM_UNDO) { ... } else if (msg == Display.SWT_RESTORECARET) { ... } why ? --clearSegments maybe not use setSelection/getSelection API ? (use PI instead) why calling checkWidget() ? some place you check if (segments == null) before calling clearSegments other places you check if (hooks(SWT.GetSegments) || filters(SWT.GetSegments)) other places you don't check anything Please, make that consistent. Since inside checkSegments has all the check you need maybe no check before the call is the right pattern. clearSegments() is called sometimes before verifyText() (see append and insert) , sometimes after (see setText). I'd say clearSegments() has always to be called before verifyText. --applySegments maybe not use setSelection/getSelection API ? (use PI instead) maybe inline getSegmentsText() (it would avoid some code) why checkWidget() ? some place you check if (segments == null) before calling applySegments other places you check if (hooks(SWT.GetSegments) || filters(SWT.GetSegments)) other places you don't check anything Please, make that consistent. Since inside applySegments has all the check you need maybe no check before the call is the right pattern. --addSegmentListener/removeSegmentListener Do we really need the pattern: clearSegments(true); add/remove listener applySegments(); StyledText never need that, clearSegments(true) is likely useles since it it already called in applySegments(). I think the question is, how does the client forces the text to re apply bidi segments (in styledText that can be done calling redraw()). I think that for Text, the app has to call setText() to force the new bidi segments to be used. -- I would not bother with start==end in getSelection/setSelection and other places (see that mbcsToWcsPos does handle the case) Overall the code is fine, some places (clearSegments/applySegments) you convert TCHAR to String back and forward maybe more than you need.
(In reply to comment #98) > --callWindowProc/windowProc > You have code in windowProc, then you have some more similar code in > callWindowProc. I find it confusing. > I don't think the code in callWindowProc is doing anything. I believe > windowProc is the only caller (calling with msg==KEY|CHAR), and it already > scopes out the segments before callWindowProc. So callWindowProc() never has > any work to do. > Please let me know what cases the code in callWindowProc is fixing. > in windowProc, why did you change: Please find a test case in Comment 69. (Felipe, I am now going through all your comments.)
(In reply to comment #99) > Please find a test case in Comment 69. That is terrible (complicated problem). Just by looking at the code I would never have guessed what problem it was fixing. Anyway, I do not think the fix is correct (it only handle a few cases). Try this: text.setText("latin"); text.addVerifyListener(new VerifyListener() { public void verifyText(VerifyEvent e) { if (e.text .equals( "a") ){ text.append("b"); } } }); text.append("a"); System.out.println("actual: " + text.getText() + " " + text.getCharCount()); I think the way to solve this type of problem is using a count (like redrawCount used in setRedraw()).
(In reply to comment #98) > javadoc wrong in SegmentEvent/SegmentListener, missing or wrong @since, some > descriptions are wrong too Yes, will try to fix these. > What is SWT.RemoveSegments ? Garbage ? Yes, sorry. > in windowProc, why did you change: > if (msg == OS.EM_UNDO) { > ... > } > if (msg == Display.SWT_RESTORECARET) { > ... > } > to: > if (msg == OS.EM_UNDO) { > ... > } else if (msg == Display.SWT_RESTORECARET) { > ... > } > why ? This was not intentional, I added some changes in that area, then discarded these changes and intended to revert the old code. However, can msg be equal to both OS.EM_UNDO and Display.SWT_RESTORECARET? (even though SWT_RESTORECARET code is obtained through RegisterWindowMessage) > --clearSegments > maybe not use setSelection/getSelection API ? (use PI instead) I think setSelection/getSelection do not introduce noticeable overhead (maybe except checkWidget call). Can be changed anyway. > why calling checkWidget() ? Sorry, I am not sure why it was used. Can be changed. > some place you check if (segments == null) before calling clearSegments > other places you check if (hooks(SWT.GetSegments) || filters(SWT.GetSegments)) (segments == null) is faster than (hooks(SWT.GetSegments) || filters(SWT.GetSegments)), and it is sufficient for clearSegments (but not for applySegments). > other places you don't check anything > Please, make that consistent. Since inside checkSegments has all the check you > need maybe no check before the call is the right pattern. OK, sure. I think the check was added per some former comment. > clearSegments() is called sometimes before verifyText() (see append and > insert), sometimes after (see setText). > I'd say clearSegments() has always to be called before verifyText. I agree. I thought it had no importance for setText, as veryfy is called for the text to be set. However, I see now that it uses OS.GetWindowTextLength - which can be affected by segments. Will change this to clear segments before. > --applySegments > maybe not use setSelection/getSelection API ? (use PI instead) > maybe inline getSegmentsText() (it would avoid some code) OK. > why checkWidget() ? > some place you check if (segments == null) before calling applySegments > other places you check if (hooks(SWT.GetSegments) || filters(SWT.GetSegments)) > other places you don't check anything I think that before applySegments I never check if (segments == null). > Please, make that consistent. Since inside applySegments has all the check you > need maybe no check before the call is the right pattern. OK, sure. > --addSegmentListener/removeSegmentListener > Do we really need the pattern: > clearSegments(true); > add/remove listener > applySegments(); > StyledText never need that, > clearSegments(true) is likely useles since it it already called in > applySegments(). This aims to address multiple listeners. The full pattern is: clearSegments(true); // clears segments associated with the current listener, say L1 <Add or remove segment listener L1> applySegments(); // apply segments associated with another listener L2 (if any) Alternative is probably to merge L1 and L2 segments.. What do you think? > I think the question is, how does the client forces the text to re apply bidi > segments (in styledText that can be done calling redraw()). I think that for > Text, the app has to call setText() to force the new bidi segments to be used. Yes, but setText disables undo.. Recalculating segments can be also forced by calling removeSegmentListener and then addSegmentListener. > I would not bother with start==end in getSelection/setSelection and other > places (see that mbcsToWcsPos does handle the case) OK. > Overall the code is fine, some places (clearSegments/applySegments) you > convert TCHAR to String back and forward maybe more than you need. I will revisit these places, thanks.
(In reply to comment #98) > Since inside checkSegments has all the check you > need maybe no check before the call is the right pattern. In some places I would leave the check before the call though, e.g.: |processSegments = segments != null && OS.GetKeyState(OS.VK_MENU) >= 0;| in windowProc. - There is no need to check the key state if segments == null.
Actually, for applySegments and clearSegments all check is outside, like that for verifyText. Only clearSegments has null checking for segments (which still may remain IMO).
(In reply to comment #100) > (In reply to comment #99) > > Please find a test case in Comment 69. > That is terrible (complicated problem). Just by looking at the code I would > never have guessed what problem it was fixing. > Anyway, I do not think the fix is correct (it only handle a few cases). Try > this: > text.setText("latin"); > text.addVerifyListener(new VerifyListener() { > public void verifyText(VerifyEvent e) { > if (e.text .equals( "a") ){ > text.append("b"); > } > } > }); > text.append("a"); > System.out.println("actual: " + text.getText() + " " + text.getCharCount()); > I think the way to solve this type of problem is using a count (like > redrawCount used in setRedraw()). I have been trying to find out more examples but only dicovered key/char event. Thank you so much for the comment, will use a count.
(In reply to comment #104) > Thank you so much for the comment, will use a count. I remove the code from callWindowProc and I implemented the count just to test the idea, it fixed in problem of comment 100 and 69. For the count to work you always need to call clearSegments/applySegments with any check in front. So append, for exampe, will look like this: public void append (String string) { checkWidget (); if (string == null) error (SWT.ERROR_NULL_ARGUMENT); string = Display.withCrLf (string); clearSegments(true); int length = OS.GetWindowTextLength (handle); if (hooks (SWT.Verify) || filters (SWT.Verify)) { string = verifyText (string, length, length, null); if (string == null) return; } OS.SendMessage (handle, OS.EM_SETSEL, length, length); TCHAR buffer = new TCHAR (getCodePage (), string, true); /* * Feature in Windows. When an edit control with ES_MULTILINE * style that does not have the WS_VSCROLL style is full (i.e. * there is no space at the end to draw any more characters), * EM_REPLACESEL sends a WM_CHAR with a backspace character * to remove any further text that is added. This is an * implementation detail of the edit control that is unexpected * and can cause endless recursion when EM_REPLACESEL is sent * from a WM_CHAR handler. The fix is to ignore calling the * handler from WM_CHAR. */ ignoreCharacter = true; OS.SendMessage (handle, OS.EM_REPLACESEL, 0, buffer); ignoreCharacter = false; OS.SendMessage (handle, OS.EM_SCROLLCARET, 0, 0); applySegments(); } clearSegments() will start like this: void clearSegments(boolean applyText) { sCount++; if (sCount != 1) return; if (segments == null) return; ... applySegments() will start like this: void applySegments() { sCount--; if (sCount != 0) return; if (!(hooks(SWT.GetSegments) || filters(SWT.GetSegments))) return; int length = OS.GetWindowTextLength (handle); Notes: I removed the bogus checkWidget() from clearSegments()/applySegments() I removed the code from callWindowProc I removed the 'if (segments != null) clearSegments(true);' from applySegments I added 'if (!(hooks(SWT.GetSegments) || filters(SWT.GetSegments))) return;' to applySegments Do you think this pattern will work (for all cases) ?
(In reply to comment #105) I also implemented this (nearly identically, just using a different variable name for the count) and it fixed both the cases. > Do you think this pattern will work (for all cases) ? I hope so.
Created attachment 188611 [details] win32 Text + common changes Felipe, I tried to address your comments. Javadoc will definitely require improvements. Javadoc for SegmentEvent duplicates that of BidiSegmentEvent with some modifs, these need to be consolidated.
Regarding the problem with arrow selection that you pointed out, we probably cannot use the common clear/apply segments pattern here, but - while moving over segments - sendKeyEvent instead. What do you think?
Created attachment 188635 [details] win32 Text + common changes Patch for win32 Text with minor updates in removeSegmentListener and applySegments methods.
---addSegmentListener I still think it is wrong call clearSegments/applySegments here Javadoc is not very informative (just a copy from the method above changing one word ?) missing since tag ---applySegments why event.segments = segments; isn't segments always going to be NULL at this point ? Isn't it a output only field in the event anyway ? in the for: for (int i = 1, lineLength = string == null ? 0 : string.length (); i < nSegments; i++) { why testing string == null ? can't you just use length in the next line ? line 474, length = string.length (); is that necessary ? do you need the oldChars array ? can you just use buffer.tcharAt(charCount++) ? can you rewrite: if (OS.SendMessage (handle, OS.EM_CANUNDO, 0, 0) != 0) { OS.SendMessage (handle, OS.EM_REPLACESEL, 1, buffer); } else { OS.SendMessage (handle, OS.EM_REPLACESEL, 0, buffer); } to: int undo = OS.SendMessage (handle, OS.EM_CANUNDO, 0, 0) != 0); OS.SendMessage (handle, OS.EM_REPLACESEL, undo, buffer); That EM_SCROLLCARET will cause problems (I will explain in the text post). ---clearSegments if (++clearSegmentsCount != 1) return; can you do: if (clearSegmentsCount++ != 0) return; deprocessText should not take a String and/or return a String It should take TCHAR and return TCHAR (stop creating String all the time) same change for EM_REPLACESEL (as applySegments) ---getCharCount can you just use if (segments != null) length = untranslateOffset (length) I know you are trying to be optimize this case maybe I prefer if you use the same code everywhere (makes it simpler) ---getText(int, int) same comment as getCharCount (length) ---getTextLimit why do you need the check for "limit < LIMIT" ? ---removeSegmentListener Javadoc is not very informative (just a copy from the method above ?) missing since tag it should have done what I said in comment 105, always call clearSegments/applySegments (no hooks test) that way your count will always be right and you will be able to remove all this hack code from removeSegmentListener ---setTextLimit why the limit > 0 test Do you really need to call Math.min(). Note that the old code does not check for this case, it assumes windows handles this case for us (I would think). could you just haved used if (segments != null) limit = untranslateOffset (limit), what happens to segments offset above the text limit ?
Lina, here is couple bugs you need to look at: 1) Run the example in comment 46. enter some more text in the control click somewhere hold shift down hold left arrow down the selection does not expand as it should 2) Run the example in comment 46. enter a lot more text in the control, so that the control has to scroll to show all the content. try to navigate from the start to end of the text and back using the arrow keys the scrolling is all funny, it scrolls at locations where it should not 3) Run the example in comment 46. enter a lot more text in the control, so that the control has to scroll to show all the content. try to scroll the text using the mouse the selection jumps like crazy 4) Run the example in comment 46. enter a lot more text in the control, so that the control has to scroll to show all the content. scroll the content either using the arrow keys or the mouse if you used the arrow keys, now click in the control to place the caret in another offset type something, the control scrolls moving the caret offset to the position of the offset before the click. (this is caused by the call to EM_SCROLLCARET that I mention in the past).
(In reply to comment #111) > Lina, here is couple bugs you need to look at: > 1) > Run the example in comment 46. > enter some more text in the control > click somewhere > hold shift down > hold left arrow down > the selection does not expand as it should This is the same problem as mentioned in comment 106. Do you think we can skip segments by firing key event? > 2) > Run the example in comment 46. > enter a lot more text in the control, so that the control has to scroll to > show all the content. > try to navigate from the start to end of the text and back using the arrow > keys the scrolling is all funny, it scrolls at locations where it should not This is a native problem. You can reproduce it in Edit control or notepad, with RLO added in the start of text. > 3) > Run the example in comment 46. > enter a lot more text in the control, so that the control has to scroll to > show all the content. > try to scroll the text using the mouse > the selection jumps like crazy This is native behavior in presence of the RLO character. > 4) > Run the example in comment 46. > enter a lot more text in the control, so that the control has to scroll to > show all the content. > scroll the content either using the arrow keys or the mouse > if you used the arrow keys, now click in the control to place the caret in > another offset > type something, the control scrolls moving the caret offset to the position of > the offset before the click. > (this is caused by the call to EM_SCROLLCARET that I mention in the past). okay, removed.
(In reply to comment #112) > > 1) > > Run the example in comment 46. > > enter some more text in the control > > click somewhere > > hold shift down > > hold left arrow down > > the selection does not expand as it should > This is the same problem as mentioned in comment 106. I meant the problem was mentioned in comment 108.
Regarding problem 4 in comment 111: As we turn redraw on and off to eliminate flickering, we may still need to send EM_SCROLLCARET after setting redraw on (following the comment in Text#setRedraw), or find another workaround for the problem.
(In reply to comment #110) > ---addSegmentListener > I still think it is wrong call clearSegments/applySegments here You mean that only clearSegments is not necessary? I believe it is, given that a caller should recieve a clean/normalized text. Otherwise, as in applySegments we obtain text thru OS.GetWindowText, while the text can contain segments at that point, the caller will get text with segment chars.
(In reply to comment #115) Alternate approach is not to clear segments and indicate segment offsets in the event. (In reply to comment #110) > ---applySegments > why event.segments = segments; > isn't segments always going to be NULL at this point ? Isn't it a output only > field in the event anyway ? That's was exactly to communicate segment information to the caller in case we do not clear segments.
Felipe, I think we have 2 options WRT to handling old segments (if any) in addSegmentListener: 1. Clear old segments and pass clean text to the caller (used at present). 2. Do not clear segments and pass segmented text and information on segment offsets to the caller. Can you please indicate which option is preferable? Do you see additional options here?
(In reply to comment #112) > This is the same problem as mentioned in comment 108. > Do you think we can skip segments by firing key event? Do you want to fake a wm_keydown when the caret offset is at a segment offset ? not sure I understand your idea.
(In reply to comment #114) > Regarding problem 4 in comment 111: > As we turn redraw on and off to eliminate flickering, we may still need to send > EM_SCROLLCARET after setting redraw on (following the comment in > Text#setRedraw), or find another workaround for the problem. I think this is a major problem. When you reset the text to insert the segments chars you have to restore the selection (which you are doing) -and- we also have to restore the horizontal (and vertical in case of multiline) scrolling offset of the control. That what EM_SCROLLCARET was (incorrectly in most cases) doing. You need a fix for this problem.
(In reply to comment #117) > Felipe, I think we have 2 options WRT to handling old segments (if any) in > addSegmentListener: > 1. Clear old segments and pass clean text to the caller (used at present). > 2. Do not clear segments and pass segmented text and information on segment > offsets to the caller. > Can you please indicate which option is preferable? Do you see additional > options here? what about 3: you don't do anything the user call removeSegmentListener, you remove the listener and nothing else. At this point the text in the native widget still has the segments chars, but that is fine because it will get removed the next time the user calls setText() or any other API that changes the text. Note that you can change the windowProc to be a bit smart and remove the segments for you in this case. Same goes for addSegmentListener, it only adds the listener. Next time an event happens the winproc updates the widget. That is my opnion.
(In reply to comment #120) > (In reply to comment #117) > > Felipe, I think we have 2 options WRT to handling old segments (if any) in > > addSegmentListener: > > 1. Clear old segments and pass clean text to the caller (used at present). > > 2. Do not clear segments and pass segmented text and information on segment > > offsets to the caller. > > Can you please indicate which option is preferable? Do you see additional > > options here? > what about 3: you don't do anything > the user call removeSegmentListener, you remove the listener and nothing else. > At this point the text in the native widget still has the segments chars, but > that is fine because it will get removed the next time the user calls setText > () or any other API that changes the text. > Note that you can change the > windowProc to be a bit smart and remove the segments for you in this case. > Same goes for addSegmentListener, it only adds the listener. Next time an > event > happens the winproc updates the widget. > That is my opnion. I think that (1) is a cleaner and more straightforward approach. 1. We do not introduce dependency on application in cleaning up internal stuff. 2. Suppose user calls getText() before some event that is (usually) handled for segments occurrs. In such a case it will get segments text. 3. After the last listener is removed |if (hooks (SWT.GetSegments) || filters (SWT.GetSegments))| will be false, which means and that segments should not be addressed in windowProc (unless we change the conditioning).
> 2. Suppose user calls getText() before some event that is (usually) handled > for segments occurrs. In such a case it will get segments text. Sorry, this is not true, since the segments will be still memorized. But again |if (hooks (SWT.GetSegments) || filters (SWT.GetSegments))| would not be sufficient in getText and other places.
(In reply to comment #118) > (In reply to comment #112) > > This is the same problem as mentioned in comment 108. > > Do you think we can skip segments by firing key event? > Do you want to fake a wm_keydown when the caret offset is at a segment > offset ? Yes, but it would be probably better to play around repeat count in lParam. Currently, the cause of the problem is that after sending OS.EM_SETSEL the shift indication is being reset, and the widget behaves as though shift is getting released and pressed again after each move.
(In reply to comment #122) > > 2. Suppose user calls getText() before some event that is (usually) handled > > for segments occurrs. In such a case it will get segments text. > Sorry, this is not true, since the segments will be still memorized. But again > |if (hooks (SWT.GetSegments) || filters > (SWT.GetSegments))| would not be sufficient in getText and other places. All these problems you pointed out are easy to solve. The way I see it is: adding a paint listener does not fire a paint event. adding a resize listener does not fire a resize event. adding a mouse down listener does not fire a mouse down event. In StyledText, adding a segment listener does NOT fire a segment event. In your code, adding a segment listener does fire a segment event... If I were you, I would be looking for a fix for the problem described in comment 119.
(In reply to comment #123) > Yes, but it would be probably better to play around repeat count in lParam. > Currently, the cause of the problem is that after sending OS.EM_SETSEL the > shift indication is being reset, and the widget behaves as though shift is > getting released and pressed again after each move. Not sure I believe this, how come the problem does not happen when I hold shift+right arrow down ?
(In reply to comment #125) > (In reply to comment #123) > > Yes, but it would be probably better to play around repeat count in lParam. > > Currently, the cause of the problem is that after sending OS.EM_SETSEL the > > shift indication is being reset, and the widget behaves as though shift is > > getting released and pressed again after each move. > Not sure I believe this, how come the problem does not happen when I hold > shift+right arrow down ? You're right, it depends on logical progression of selection. Actually, the widget IS concerned with shift being down, but disregards selection direction and only remembers the least selection offset (in buffer). E.g., if selection if done within an RTL run of characters (as in the example in comment 46), LShift works and RShift does not. And, if selection if done within an LTR run of characters, RShift does work and LShift does not. I forgot, I've been seeing these symptoms not once in the past.
(In reply to comment #126) > And, if selection if done within an LTR run of characters, RShift does work > and LShift does not. Here is a test case. Segment chars added are LRO/PDF ), which makes text be LTR (as opposed to RLO/PDF in comment 46, which forces text to be RTL). public static void main (String[] args) { final Display display = new Display(); final Shell shell = new Shell(display); shell.setLayout(new GridLayout()); final Text text = new Text (shell, SWT.SINGLE); text.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); text.addSegmentListener(new SegmentListener() { public void lineGetSegments(SegmentEvent event) { event.segments = new int [] {0, event.text.length()}; event.segmentsChars = new char [] {'\u202D', '\u202C'}; } }); text.setText("latin"); shell.setSize(300, 300); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } }
(In reply to comment #127) My apollogies: shift behavior depends of course on widget orientation but not directional properties of characters. If selection is done in an LTR control, RShift works and LShift does not. And, if selection is done in an RTL control, RShift does work and LShift does not. Anyway, repeat count may help here I think.
(In reply to comment #119) > (In reply to comment #114) > > Regarding problem 4 in comment 111: > > As we turn redraw on and off to eliminate flickering, we may still need to send > > EM_SCROLLCARET after setting redraw on (following the comment in > > Text#setRedraw), or find another workaround for the problem. > I think this is a major problem. > When you reset the text to insert the segments chars you have to restore the > selection (which you are doing) -and- we also have to restore the horizontal > (and vertical in case of multiline) scrolling offset of the control. > That what EM_SCROLLCARET was (incorrectly in most cases) doing. > You need a fix for this problem. I will try to use GetScrollInfo and SetScrollInfo functions. The problem is caused by turning redraw off/on (if these are commented out, it doesn't occur). So the workaround should be also adjusted in setRedraw method?
(In reply to comment #124) > The way I see it is: > adding a paint listener does not fire a paint event. > adding a resize listener does not fire a resize event. > adding a mouse down listener does not fire a mouse down event. > In StyledText, adding a segment listener does NOT fire a segment event. > In your code, adding a segment listener does fire a segment event... > If I were you, I would be looking for a fix for the problem described in > comment 119. What makes Text stand alone is that it introduces segments characters to the buffer and doesn't handle painting. After SegmentListener is added to StyledText, segments are taking effect "immediately" (on next rendering event). To get consistent with StyledText, I update the buffer, so the native control paints for us as needed "immediately" too. An alternative is to handle WM_PAINT in Text#windowProc. I would prefer the former...
(In reply to comment #130) > After SegmentListener is added to StyledText, segments are taking effect > "immediately" (on next rendering event). "immediately" != next rendering event in a retained mode system (like windows 7 and cocoa), the next rendering event will only occur (most likely) on the text time the text changes.
(In reply to comment #129) > I will try to use GetScrollInfo and SetScrollInfo functions. Try it, not sure it will work. Usually it is not a good thing to interfere with scrolling of a control by reaching in and calling SetScrollInfo. > The problem is caused by turning redraw off/on (if these are commented out, it > doesn't occur). I'd like to know why turning redraw off/on creates this problem. > So the workaround should be also adjusted in setRedraw method? I need to know what is happening before I can answer that.
(In reply to comment #132) > (In reply to comment #129) > > I will try to use GetScrollInfo and SetScrollInfo functions. > Try it, not sure it will work. Usually it is not a good thing to interfere > with scrolling of a control by reaching in and calling SetScrollInfo. Yes, but this is not the only problem. Using these API (in conjunction with sending WM_HSCROLL and WM_VSCROLL messages) we can recreate the control scrolling indeed. However, the major problem is that control doesn't scroll when redraw is turned off. > > The problem is caused by turning redraw off/on (if these are commented out, > > it doesn't occur). > I'd like to know why turning redraw off/on creates this problem. > > So the workaround should be also adjusted in setRedraw method? > I need to know what is happening before I can answer that. The problem can be reproduced when text content changes (probably under other circumstances too). Here is a test case. Steps: 1. Load the following example: public static void main (String[] args) { final Display display = new Display (); final Shell shell = new Shell (display); shell.setLayout(new GridLayout ()); final Text text = new Text (shell, SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL); text.setLayoutData (new GridData (GridData.FILL_HORIZONTAL)); text.setText ("frag1 frag2 frag3 frag4 frag5 frag6 frag7 frag8 frag9 frag10 frag11 frag12 frag13 frag14 frag15"); final int sel = 60; text.setSelection (sel); display.addFilter (SWT.KeyDown, new Listener () { public void handleEvent (Event event) { if (event.keyCode == SWT.F1) { text.setRedraw (false); text.selectAll (); text.setSelection (sel); text.setRedraw (true); // Result: no visible changes } else if (event.keyCode == SWT.F2) { text.setRedraw (false); text.setText (text.getText ()); text.setSelection (sel); text.setRedraw (true); // Result: caret position is correct but control scrolling occurs } } }); shell.setSize(300, 300); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } 2. Scroll the content horizontally to the end of text. 3a. Press F1. Result: No visible changes (as expected). 3b. Press F2. Result: Caret offset remains intact as expected, but the control scrolls unexpectedly.
(In reply to comment #133) > 2. Scroll the content horizontally to the end of text. In step 2 of the test case, please use the scroll thumb for scrolling, otherwise, after the scrolling, place the caret manually somewhere inside a visible portion of text (e.g. in "frag10").
(In reply to comment #131) > (In reply to comment #130) > > After SegmentListener is added to StyledText, segments are taking effect > > "immediately" (on next rendering event). > "immediately" != next rendering event > in a retained mode system (like windows 7 and cocoa), the next rendering event > will only occur (most likely) on the text time the text changes. I meant that StyledText content will get updated without any forcible action at the application/user side. I think that the common scenario is to add BidiSegmentListener before a corresponding window is shown up. So, given Windows (inclusing Windows 7) always sends WM_PAINT first time the window is shown, segments will work "immediately" from the user perspective. I am not sure if cocoa calls drawRect when a window is showing for the first time. Anyway, I will change this in the way you prefer.
(In reply to comment #110) > ---applySegments > in the for: > for (int i = 1, lineLength = string == null ? 0 : string.length (); i < > nSegments; i++) { > why testing string == null ? can't you just use length in the next line ? > line 474, length = string.length (); is that necessary ? No, it is absolutely not necessary. Previously, I had a 'processText' method which used to be called in that place. Later, I replaced the call with inline code chunks but as it appears now have not cleaned up the code completely. Thanks.
(In reply to comment #136) > deprocessText should not take a String and/or return a String > It should take TCHAR and return TCHAR (stop creating String all the time) > same change for EM_REPLACESEL (as applySegments) I think we would not win due to this change, as some operations on TCHAR are much more expensive than those on String. E.g. TCHAR#strlen() vs. String#length() or TCHAR assigment operations. Also, in applySegments, segmentChars would need to be transformed to TCHAR anyway. Instead, I suggest that before converting to TCHAR the caller ensures null termination (if required) and calls new TCHAR (cp, newChars, false); (i.e. with the 'terminate" argument set to 'false').
(In reply to comment #137) > (In reply to comment #136) > > deprocessText should not take a String and/or return a String > > It should take TCHAR and return TCHAR (stop creating String all the time) > > same change for EM_REPLACESEL (as applySegments) > I think we would not win due to this change, as some operations on TCHAR are > much more expensive than those on String. > E.g. TCHAR#strlen() vs. String#length() or TCHAR assigment operations. You are creating new objects, allocating new (potentially large) char arrays, moving memory from one array to the other. ..and you are concern that length() in TCHAR is too slow ? I'm afraid you chose a bad example, see the code: public int length () { if (OS.IsUnicode) { return chars.length; } else { return byteCount; } } (not sure what you mean by assigment operations, both String and TCHAR are immutable)
(In reply to comment #138) > ..and you are concern that length() in TCHAR is too slow ? I'm afraid you > chose a bad example, see the code: > public int length () { > if (OS.IsUnicode) { > return chars.length; > } else { > return byteCount; > } > } I meant TCHAR#strlen() (not TCHAR#length() ) which seems to be equivalent to String#length() in terms of obtaining a character count. > (not sure what you mean by assigment operations, both String and TCHAR are > immutable) I meant indirect assigment. In case of TCHAR we should constantly consider if the character set is DBCS or not. The underlying code of tcharAt(index) checks for Unicode version each time a character is retrieved. I think it is faster to convert the entire text at a time to a character array and then work with the latter. BTW, in either case, after segments addition/removal is done, a new character array should be reallocated and converted to a new TCHAR again as the array length changes. As for the old chars array (oldChars[]) it of course can be removed.
Created attachment 191558 [details] win32 Text + common changes Felipe, this patch attempts to address the issue with reverse selection that you pointed out. 1. Setting reverse selection. I tried a number of solutions (such as repositioning the caret after setting a regular selection, playing around EN_UNDO), but the only working one appears to be synthesizing SHIFT + arrow keystrokes. 2. Getting reverse selection. Turned out to be not trivial task as the control doesn't update caret position when redraw is off. A workaround is to temporarily enable painting for detecting the caret position.
Created attachment 191583 [details] win32 Text + common changes Slightly modified win32 patch.
I noticed a problem in deprocessText (initially, it used to return the entire text with substring indicated by offsets being normalized; later, I changed it to return the normalized substring only - but have not applied the new approach everywhere). I'll include a fix for that with a next patch.
Created attachment 191823 [details] win32 Text + common changes win32 patch with modified deprocessText and some minor updates.
Created attachment 191860 [details] win32 + gtk Text patch Updated patch for both Win32 and GTK Text
Can the above attached patch applied against SWT 3.6.2?
(In reply to comment #144) > Created attachment 191860 [details] > win32 + gtk Text patch > > Updated patch for both Win32 and GTK Text Linna, can you upload the patch again please ? I tested, on win32, the lastest patch and it is not working at all for me (selection save/restore part is wrong). Maybe I have the wrong code.
(In reply to comment #146) > I tested, on win32, the lastest patch and it is not working at all for me > (selection save/restore part is wrong). Maybe I have the wrong code. Felipe, can you please indicate scenario when it fails? I'd like to test it before uploading the patch. Thanks.
(In reply to comment #147) > Felipe, can you please indicate scenario when it fails? I'd like to test it > before uploading the patch. Thanks. Sure, try this code: final Text text = new Text(shell, SWT.SINGLE| SWT.BORDER); text.addSegmentListener(new SegmentListener() { public void getSegments(SegmentEvent event) { int length = event.text.length(); event.segments = new int [] {0, length}; event.segmentsChars = new char [] {'\u202E', '\u202C'}; } }); text.setText("latin more text is needed"); run it, hold shift key down and use arrow keys to select text, the selection dances around with the caret jumping from the end to the start offset and back.
Created attachment 193759 [details] win32 + gtk patch for Text Felipe, here is an updated patch that addresses Shift + Arrow keystrokes.
(In reply to comment #149) > Created attachment 193759 [details] > win32 + gtk patch for Text > > Felipe, here is an updated patch that addresses Shift + Arrow keystrokes. There is an infinite loop in your code. Just run the simple test case (see comment 148). Hold shift and press left arrow.
Created attachment 194112 [details] win32 + gtk patch for Text Sorry, this didn't happen with examples I tested. I hope the new patch fixes the problem.
Is the latest patch something that can be used?
(In reply to comment #152) > Is the latest patch something that can be used? I recommend you to wait for this patch to make to Eclipse first, only then it will get tests as it should.
Lina, we are currently moving all our source code to GIT. Once this works is complete I will create a branch in git and release all the code there. When we are happy with all the changes in the branch I will merge the branch to master. If you are not familiar with git you should consider reading about git (and egit).
(In reply to comment #154) > Lina, we are currently moving all our source code to GIT. Once this works is > complete I will create a branch in git and release all the code there. When we > are happy with all the changes in the branch I will merge the branch to master. Sounds good, thanks!
(In reply to comment #155) > Sounds good, thanks! Lina, please see http://eclipse.org/swt/git.php I have created a branch bug230854 http://git.eclipse.org/c/platform/eclipse.platform.swt.git/log/?h=bug230854 I have started the branch with the patch from comment 149 and added the extra fix in comment 151 (as an incremental change). Every new patch you send should be just the incremental change relative to the last commit, not the entire patch all over (which made very difficult for me to see how the code was changing). For example, to see just changes between the patch in comment 149 and comment 151 use 'git diff b9e679d..15597fa' Please let me know when you get git setup and ready to do.
Just pushed more changes in the branch http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=3e073b9871efe7c2196de8b30afc17b6de158c3c fix @since tags fix move caret left/right -- moved out of windowproc (too much was happening in that method) -- old code was sending multiple SWT.KeyDown events -- old code did not handle VK_UP,VK_DOWN
(In reply to comment #157) > Just pushed more changes in the branch > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=3e073b9871efe7c2196de8b30afc17b6de158c3c > Thanks! _____________ I tried the Text from the bug230854 branch and noticed a problem with SHIFT + LEFT/RIGHT keys. For example, when running a test case from comment 148: -- Locate the caret at home and press just VK_RIGHT. Result is OK. -- Locate the caret at home and press VK_SHIFT + VK_RIGHT. Result: the caret moves to the end and the entire text gets selected. (In RTL Text, a similar problem occurs with VK_SHIFT + VK_LEFT.) > fix @since tags > fix move caret left/right Can you please indicate what problems were fixed? > -- moved out of windowproc (too much was happening in that method) > -- old code was sending multiple SWT.KeyDown events > -- old code did not handle VK_UP,VK_DOWN Why is it required?
(In reply to comment #158) > For example, when running a test case from comment 148: > -- Locate the caret at home and press just VK_RIGHT. Result is OK. > -- Locate the caret at home and press VK_SHIFT + VK_RIGHT. Result: the caret > moves to the end and the entire text gets selected. > (In RTL Text, a similar problem occurs with VK_SHIFT + VK_LEFT.) Okay, fixed. Put back your code (I understood it finally). > Can you please indicate what problems were fixed? see comment 157 or browse the changes in git. > > -- old code did not handle VK_UP,VK_DOWN > Why is it required? the native controls does move the cursor during arrow up and down (for single line text at least), so we have to fix it too. right ? Do you remember what else is in the TODO list for the windows implementation ?
(In reply to comment #159) > Okay, fixed. Put back your code (I understood it finally). http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=6ac9b2722ec47c39865e69ca6ed181b7e475b6f8
(In reply to comment #159) > (In reply to comment #158) > > > -- old code did not handle VK_UP,VK_DOWN > > Why is it required? > the native controls does move the cursor during arrow up and down (for single > line text at least), so we have to fix it too. right ? For me it doesn't move in a single-line Edit. I am testing on Windows 7 and XP. > Do you remember what else is in the TODO list for the windows implementation ? This definitely include UNDO treatment. Do you have any pointers as to its implementation?
Felipe, I did some manual testing for the new edition and so far have not encounter any problem (except for missing UNDO capability).
(In reply to comment #162) > Felipe, I did some manual testing for the new edition and so far have not > encounter any problem (except for missing UNDO capability). We have agreed that UNDO would be missing, at least in the first cut, right ? The only problem is, what happens when undo is used ? - nothing or - breaks everything (does something unexpected)
(In reply to comment #163) > We have agreed that UNDO would be missing, at least in the first cut, right ? Yes of course. Sorry, forgot about it. > The only problem is, what happens when undo is used ? > - nothing > or > - breaks everything (does something unexpected) Currently (provided segments were actually inserted/removed), it undoes and redoes the last insertion/removal of the segments. Apparently, it is preferable to totally disable undo if |(hooks (SWT.GetSegments) || filters (SWT.GetSegments))| at this point. What do you think?
(In reply to comment #161) > (In reply to comment #159) > > (In reply to comment #158) > > > > -- old code did not handle VK_UP,VK_DOWN > > > Why is it required? > > the native controls does move the cursor during arrow up and down (for single > > line text at least), so we have to fix it too. right ? > For me it doesn't move in a single-line Edit. I am testing on Windows 7 and XP. weird, it did for me. Anyhow, the code is not causing any problems for you, right ?
(In reply to comment #165) > (In reply to comment #161) > > (In reply to comment #159) > > > (In reply to comment #158) > > > > > -- old code did not handle VK_UP,VK_DOWN > > > > Why is it required? > > > the native controls does move the cursor during arrow up and down (for single > > > line text at least), so we have to fix it too. right ? > > For me it doesn't move in a single-line Edit. I am testing on Windows 7 and XP. > > > weird, it did for me. Anyhow, the code is not causing any problems for you, > right ? Definitely not.
(In reply to comment #164) > Apparently, it is preferable to totally disable undo if |(hooks > (SWT.GetSegments) || filters (SWT.GetSegments))| at this point. > What do you think? disabling would be safer, please provide the patch. (try to use git format-patch to create the patch, see http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git/)
(In reply to comment #167) > (In reply to comment #164) > > Apparently, it is preferable to totally disable undo if |(hooks > > (SWT.GetSegments) || filters (SWT.GetSegments))| at this point. > > What do you think? > > disabling would be safer, please provide the patch. > (try to use git format-patch to create the patch, see > http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git/) Felipe, I am going to add the following lines to windowProc (int, int, int, int) when (hooks (SWT.GetSegments) || filters (SWT.GetSegments)): case OS.EM_CANUNDO: return 0; It works, just can you please confirm if i's okay (or maybe you prefer that we handle WM_UNDO, EM_UNDO instead)?
(In reply to comment #168) > It works, just can you please confirm if i's okay (or maybe you prefer that we > handle WM_UNDO, EM_UNDO instead)? I would use the methods, if both where there. Adding the code to windowProc is fine. I will merge the change myself. If we are happy with win32, lets move to gtk. Is the code in the branch ready for review ?
(In reply to comment #169) > disabling would be safer, please provide the patch. > (try to use git format-patch to create the patch, see > http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git/) Should this be a patch against the master or against bug230854 branch?
(In reply to comment #170) > Should this be a patch against the master or against bug230854 branch? the bug230854 branch, where I will be releasing the code. Once we finish the whole thing, I'll merge the changes in the branch to master.
Created attachment 204679 [details] Git patch per comment 171 Felipe, this patch is to track 2 commits. Please let me know if it's in the expected format.
(In reply to comment #172) > Created attachment 204679 [details] > Git patch per comment 171 > > Felipe, this patch is to track 2 commits. > Please let me know if it's in the expected format. [felipe@wsfheidrichlnx eclipse.platform.swt]$ git am patch.txt Patch format detection failed. Not sure what is wrong but it failed. This will not fix it, but I think you didn't need to have two commits for this one simple change. Besides, you should set your name and email in git. If you look at the head of the file you will see: From 3bbfa58a32c1dfabe7b824b7f277cad8382349d1 Mon Sep 17 00:00:00 2001 From: bdl <bdl@lkemmel-XXXXXX> Date: Tue, 4 Oct 2011 19:17:36 +0200 That is actually the info that will be added to the git repo when I push the changes. Please use $ git config --global user.name "Lina Kemmel" $ git config --global user.email lkemmel@il.ibm.com See http://progit.org/book/ch1-5.html
Created attachment 204835 [details] Git patch per comments 171 & 173 Sorry for the mess. Here is a new patch.
Created attachment 204836 [details] Git patch per comments 171 & 173 Fixed misindentation in the code.
(In reply to comment #175) > Created attachment 204836 [details] > Git patch per comments 171 & 173 > > Fixed misindentation in the code. That worked great: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=97a564863cb3520bd1597a176dc964c37d571187 Note that I removed the special case for multiline (basically the multi line + bidi segments is broken, no point fixing just the EM_UNDO). http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=9eaccc1cb16d9d4692cc745cb6ccf9b2b5e4c1f8 Or is there another reason for that code to be there ? ---- Please let me know when to review the GTK part of the work.
(In reply to comment #176) > Note that I removed the special case for multiline (basically the multi line + > bidi segments is broken, no point fixing just the EM_UNDO). I remember of the potential performance issue. Are there any additional problems in multiline? > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=9eaccc1cb16d9d4692cc745cb6ccf9b2b5e4c1f8 > > Or is there another reason for that code to be there ? I just tried to follow the doc, which says that in case of EM_UNDO "For a single-line edit control, the return value is always TRUE." http://msdn.microsoft.com/en-us/library/windows/desktop/bb761670%28v=vs.85%29.aspx > > ---- > > Please let me know when to review the GTK part of the work. The gtk part included with Attachment #194112 [details] was basically ready from my perspective, but probably it makes sense that I verify it once again. I can do that right after the holiday, in the beginning of the next week.
*** Bug 365228 has been marked as a duplicate of this bug. ***
I started to review the gtk patch the first review is here http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=a5940b9047be0b1097fccd45ae95a1acafaba63b add constant GTK_MOVEMENT_VISUAL_POSITIONS to OS and remove the connect/disconnect code from addSegmentListener/removeListener as it fails for the cases display#addFilter(SWT.GetSegments, listener) and text.addListener(SWT.GetSegments, listener). Lina, in the windowProc()s you used: int /*long*/ windowProc (int /*long*/ handle, int /*long*/ arg0, int /*long*/ arg1, int /*long*/ user_data) { if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { switch ((int)/*64*/user_data) { case DELETE_FROM_CURSOR: { if (segments != null) clearSegments (true); break; } case DELETE_FROM_CURSOR_INVERSE: { applySegments (); return 0; } } } return super.windowProc (handle, arg0, arg1, user_data); } why in one case used a break inside the case statement and in the other 'return 0' ? Any reason for that ?
GTK question about copy()/paste()/cut() none of the above methods are doing any work for SINGLE case (only MULTI). Do you rely that the signals are sent for GtkEntry but not for GtkTextView ? The code also is not consistent amont these tree methods: cut: if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { if (segments != null) clearSegments (true); OS.gtk_text_buffer_cut_clipboard (bufferHandle, clipboard, OS.gtk_text_view_get_editable (handle)); applySegments (); } else { OS.gtk_text_buffer_cut_clipboard (bufferHandle, clipboard, OS.gtk_text_view_get_editable (handle)); } copy: if (segments != null) { clearSegments (true); OS.gtk_text_buffer_copy_clipboard (bufferHandle, clipboard); applySegments (); } else { OS.gtk_text_buffer_copy_clipboard (bufferHandle, clipboard); } paste: if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { if (segments != null) clearSegments (true); OS.gtk_text_buffer_paste_clipboard (bufferHandle, clipboard, null, OS.gtk_text_view_get_editable (handle)); applySegments (); } else { OS.gtk_text_buffer_paste_clipboard (bufferHandle, clipboard, null, OS.gtk_text_view_get_editable (handle)); } Is there any reason copy is different ? I thought the pattern was: if (segments != null) clearSegments (true); //do the work if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { applySegments (); }
(In reply to comment #179) Hi Felipe, Thanks very much for the review. > I started to review the gtk patch > the first review is here > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854&id=a5940b9047be0b1097fccd45ae95a1acafaba63b > add constant GTK_MOVEMENT_VISUAL_POSITIONS to OS > and remove the connect/disconnect code from addSegmentListener/removeListener > as it fails for the cases > display#addFilter(SWT.GetSegments, listener) and > text.addListener(SWT.GetSegments, listener). Definitely, thanks. > Lina, in the windowProc()s you used: > int /*long*/ windowProc (int /*long*/ handle, int /*long*/ arg0, int /*long*/ > arg1, int /*long*/ user_data) { > if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { > switch ((int)/*64*/user_data) { > case DELETE_FROM_CURSOR: { > if (segments != null) clearSegments (true); > break; > } > case DELETE_FROM_CURSOR_INVERSE: { > applySegments (); > return 0; > } > } > } > return super.windowProc (handle, arg0, arg1, user_data); > } > > > why in one case used a break inside the case statement and in the other > 'return 0' ? Any reason for that ? I think the only reason was to indicate that in DELETE_FROM_CURSOR_INVERSE we do not want to do anything else. Anyway, break in both cases would be better.
(In reply to comment #180) > GTK question about copy()/paste()/cut() > none of the above methods are doing any work for SINGLE case (only MULTI). Do > you rely that the signals are sent for GtkEntry but not for GtkTextView ? I believe MULTI is addressed in another signal handler. Will recheck this shortly. > The code also is not consistent amont these tree methods: > > cut: > if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { > if (segments != null) clearSegments (true); > OS.gtk_text_buffer_cut_clipboard (bufferHandle, clipboard, > OS.gtk_text_view_get_editable (handle)); > applySegments (); > } else { > OS.gtk_text_buffer_cut_clipboard (bufferHandle, clipboard, > OS.gtk_text_view_get_editable (handle)); > } > > copy: > if (segments != null) { > clearSegments (true); > OS.gtk_text_buffer_copy_clipboard (bufferHandle, clipboard); > applySegments (); > } else { > OS.gtk_text_buffer_copy_clipboard (bufferHandle, clipboard); > } > > paste: > if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { > if (segments != null) clearSegments (true); > OS.gtk_text_buffer_paste_clipboard (bufferHandle, clipboard, null, > OS.gtk_text_view_get_editable (handle)); > applySegments (); > } else { > OS.gtk_text_buffer_paste_clipboard (bufferHandle, clipboard, null, > OS.gtk_text_view_get_editable (handle)); > } > > Is there any reason copy is different ? I thought the pattern was: > if (segments != null) clearSegments (true); > //do the work > if (hooks (SWT.GetSegments) || filters (SWT.GetSegments)) { > applySegments (); > } Copy doesn't cause any modifications to text, so applySegments () can be omitted when segments = null.
(In reply to comment #182) > Copy doesn't cause any modifications to text, so applySegments () can be > omitted when segments = null. Of course, we have all checks inside applySegments, however the null check on segments occurs a bit late. clearSegments is also not necessary unless segments != null. However, these are pretty minor things. Would you prefer to make copy be consistent with other cases?
(In reply to comment #180) > GTK question about copy()/paste()/cut() > none of the above methods are doing any work for SINGLE case (only MULTI). Do > you rely that the signals are sent for GtkEntry but not for GtkTextView ? Yes, according to the doc those signals are received by GtkEntry , e.g.: http://developer.gnome.org/gtk/2.24/GtkEntry.html#GtkEntry-copy-clipboard
Done: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854 I will review everything again next week. Let me know if that is anything you'd like to change.
(In reply to comment #185) > Done: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=bug230854 > > I will review everything again next week. Let me know if that is anything you'd > like to change. Thanks, I have nothing to change.
Is the fix on radar for 3.8?
(In reply to comment #187) > Is the fix on radar for 3.8? yes, we hope to merge the feature branch to master for 3.8.
Hi Lina, do we have any test case using this support that is actually doing something useful ? I guess what I'm asking is a implementor of SegmentListener that does something interesting like formatting paths or uris. The test case I have here only adds '\u202E' at zero and '\u202C' at length. Not very interesting.
(In reply to comment #189) Hi Felipe, > Hi Lina, do we have any test case using this support that is actually doing > something useful ? > I guess what I'm asking is a implementor of SegmentListener that does something > interesting like formatting paths or uris. Yes, I will upload them. > The test case I have here only adds > '\u202E' at zero and '\u202C' at length. Not very interesting. Despite the trivial implementation, it's very useful for enforcing "visual" ordering scheme of text. In case of multiline the '\u202E' (or '\u202E') and '\u202C' pairs should be added per line basis.
Here is a basic implementor for preserving the structure of paths. It is of course subject to improve (e.g. by doing finer analysis of text content, only indicating the "bare minimum" of segments). text.addSegmentListener(new SegmentListener() { public void getSegments(SegmentEvent event) { char[] chars = event.text.toCharArray(); int nChars = chars.length; if (nChars > 1) { int nSegments = 1; int[] segments = new int[nChars + 1]; segments[0] = 0; for (int i = 0; i < nChars; i++) { if (chars[i] == '.' || chars[i] == '/' || chars[i] == '\\' || chars[i] == ':') { segments[nSegments++] = i; } } if (nSegments > 1) { event.segmentsChars = new char[nSegments]; event.segmentsChars[0] = '\u202a'; // Add LRE for (int i = 1; i < nSegments; i++) { event.segmentsChars[i] = '\u200e'; // Add LRM } if (nSegments == nChars + 1) { event.segments = segments; } else { event.segments = new int[nSegments]; System.arraycopy(segments, 0, event.segments, 0, nSegments); } } } }
I believe it would be a good idea to start using toolkit (org.eclipse.equinox.bidi) introduced via bug 183164. This toolkit includes very smart parsers which can be used for parsing out text based on the type of structured text (in this case file path).
(In reply to comment #192) > I believe it would be a good idea to start using toolkit > (org.eclipse.equinox.bidi) introduced via bug 183164. This toolkit includes > very smart parsers which can be used for parsing out text based on the type of > structured text (in this case file path). I tried that, is this code correct ? class TestListener implements BidiSegmentListener, SegmentListener { STextEnvironment env = null;//Bug 183164 public TestListener () { env = new STextEnvironment(null, false, STextEnvironment.ORIENT_UNKNOWN); } void handleSegements(Event event) { event.segments = STextEngine.leanBidiCharOffsets(new STextFile(), null, env, event.text, null); if (event.segments != null & event.segments.length > 0) { if (event.segments [0] != 0) { int[] src = event.segments; event.segments = new int[src.length + 1]; System.arraycopy(src, 0, event.segments, 1, src.length); } } } public void getSegments(SegmentEvent event) { Event e = new Event(); e.text = event.text; handleSegements(e); event.segments = e.segments; event.segmentsChars = e.segmentsChars; } public void lineGetSegments(BidiSegmentEvent event) { Event e = new Event(); e.text = event.lineText; handleSegements(e); event.segments = e.segments; event.segmentsChars = e.segmentsChars; } } I'm using the same listener on a Text and StyledText: TestListener listener = new TestListener(); Text text = new Text(shell, SWT.SINGLE| SWT.BORDER); text.addSegmentListener(listener); StyledText st = new StyledText(shell, SWT.SINGLE| SWT.BORDER); st.addBidiSegmentListener(listener); Am I using the toolkit from bug 183164 correctly ?
(In reply to comment #193) > (In reply to comment #192) > > I believe it would be a good idea to start using toolkit > > (org.eclipse.equinox.bidi) introduced via bug 183164. This toolkit includes > > very smart parsers which can be used for parsing out text based on the type of > > structured text (in this case file path). > > I tried that, is this code correct ? > This code uses an old version of the STructured Text package. I wonder where it found this old version. Equivalent code for the current version could be: import org.eclipse.equinox.bidi.STextTypeHandlerFactory; import org.eclipse.equinox.bidi.advanced.STextExpertFactory; import org.eclipse.equinox.bidi.advanced.ISTextExpert; . . . ISTextExpert expert = STextExpertFactory.getExpert(STextTypeHandlerFactory.FILE); . . . event.segments = expert.leanBidiCharOffsets(event.text); There is no need to create or refer an environment object.
(In reply to comment #194) > This code uses an old version of the STructured Text package. I wonder where it > found this old version. In the CVS, my bad. I didn't realize equinox had moved to git. > Equivalent code for the current version could be: > > import org.eclipse.equinox.bidi.STextTypeHandlerFactory; > import org.eclipse.equinox.bidi.advanced.STextExpertFactory; > import org.eclipse.equinox.bidi.advanced.ISTextExpert; > . . . > ISTextExpert expert = > STextExpertFactory.getExpert(STextTypeHandlerFactory.FILE); > . . . > event.segments = expert.leanBidiCharOffsets(event.text); > Using this code I get this exception: Exception in thread "main" java.lang.ExceptionInInitializerError at org.eclipse.equinox.bidi.STextTypeHandlerFactory.getHandler(STextTypeHandlerFactory.java:132) at org.eclipse.equinox.bidi.advanced.STextExpertFactory.getExpert(STextExpertFactory.java:95) at dbcs.PR230854b$1TestListener.getSegments(PR230854b.java:52) Note that I just running a simple snippet (I'm not running the entire workbench). I had a similar bug,in the old code, when I used: STextEngine.leanBidiCharOffsets(ISTextTypes.FILE, null, env, event.text, null); Please advise, thank you
(In reply to comment #195) > (In reply to comment #194) > Using this code I get this exception: > Exception in thread "main" java.lang.ExceptionInInitializerError > at > org.eclipse.equinox.bidi.STextTypeHandlerFactory.getHandler(STextTypeHandlerFactory.java:132) > at > org.eclipse.equinox.bidi.advanced.STextExpertFactory.getExpert(STextExpertFactory.java:95) > at dbcs.PR230854b$1TestListener.getSegments(PR230854b.java:52) > > > Note that I just running a simple snippet (I'm not running the entire > workbench). > I had a similar bug,in the old code, when I used: > STextEngine.leanBidiCharOffsets(ISTextTypes.FILE, null, env, event.text, null); > > Please advise, thank you It seems that the problem is with initialization / registration of the bidi plug-in. Do you run your program as a Plug-in Test?
(In reply to comment #196) > It seems that the problem is with initialization / registration of the bidi > plug-in. Do you run your program as a Plug-in Test? No, not a plugin. Just a regular snippet. It seems it is impossible to use the toolkit without running eclipse along. Feels like a big restriction in my opinion. Other detail that is a bit of inconvenience is that StyledText (for history reason) needs the first element to be zero. Forcing every caller to have this sort of code at end: if (event.segments != null & event.segments.length > 0) { if (event.segments [0] != 0) { int[] src = event.segments; event.segments = new int[src.length + 1]; System.arraycopy(src, 0, event.segments, 1, src.length); } } See the javadoc for: org.eclipse.swt.custom.BidiSegmentEvent
I moved the code to master, please file new bugs for all the known limitations (cocoa implementation, combo) and other issues (no undo stack in windows, etc). Thanks very much to everybody that helped.
(In reply to comment #197) > (In reply to comment #196) > > > It seems that the problem is with initialization / registration of the bidi > > plug-in. Do you run your program as a Plug-in Test? > > No, not a plugin. Just a regular snippet. It seems it is impossible to use the > toolkit without running eclipse along. Feels like a big restriction in my > opinion. The current design of the SText support is as a plugin, which allows to expand it with additional types of structured text without having to modify the existing source code. A consequence is that you must have some OSGi (I think) initializations in order for the plugin to run properly. It has been my experience that running my test programs as regular apps would fail at the same place as in your test. It is not difficult to remove the dependency on "running eclipse along" (I have done it to make run some tests that I did not know how to run otherwise). The price is that it becomes impossible to provide support for new types of structured text for all apps to use without having to modify some code in the SText packages. An application would still be able to add support for a new structured text type for its own consumption without modifying the SText code. Personally I would favor such a change in design, but that would have to be approved by my betters. > > Other detail that is a bit of inconvenience is that StyledText (for history > reason) needs the first element to be zero. Forcing every caller to have this > sort of code at end: > if (event.segments != null & event.segments.length > 0) { > if (event.segments [0] != 0) { > int[] src = event.segments; > event.segments = new int[src.length + 1]; > System.arraycopy(src, 0, event.segments, 1, src.length); > } > } > > See the javadoc for: org.eclipse.swt.custom.BidiSegmentEvent It should be easy enough to do that within StyledText itself after checking if the first element is a zero or not.
I was able to simplify my test case to this: class TestListener implements BidiSegmentListener, SegmentListener { public void getSegments(SegmentEvent event) { ISTextExpert expert = STextExpertFactory.getExpert(); event.segments = expert.leanBidiCharOffsets(event.lineText); } public void lineGetSegments(BidiSegmentEvent event) { getSegments(event); } } But for it to work as simple snippet I had to hack the code. The problem there is the class initialization of the DEFAULT field. In my case that failed cause of a NPE inside of the constructor, causing the class not to be loaded. It failed because the STextActivator is not initialized. As far as I can tell the code *only needs* the default locale... Forcing the client to run an entire eclipse just to get the locale is bad (for my case at least). >It should be easy enough to do that within StyledText itself after checking if the first element is a zero or not. Done Bug 372760
The problem with running standalone vs. as OSGi bundle has been solved before. E.g. org.eclipse.jface.util.Policy#setStatusHandler(StatusHandler) is initialized from org.eclipse.ui.internal.WorkbenchPlugin#start(BundleContext). If there's no bundle, it uses a default handler. Something similar can be done at locations where org.eclipse.equinox.bidi accesses other bundles.
