Community
Participate
Working Groups
Currently, BidiSegmentListener provides means of Bidi segmentation aimed at preserving the relative order of text runs. That's a highly valuable function, broadly consumed in Bidi-enabled SW products. However, preserving order of segments is just one kind of Bidi overrides. I think that with a very small effort we could make Bidi segments support more comprehensive and flexible. (The expected changes may be considered "API related" though.) At present, we allow clients to communicate to the layout engine segment offsets, whereas characters applied at these offsets are fixed (well, fixed per paragraph direction) and hardcoded in TextLayout. In order to accomplish any variation to the Bidi algorithm, we just need to let clients also choose code points to be inserted at those offsets. A sound use case in this context is "visually" stored data (practiced e.g. on mainframe systems), which requires nonstandard reordering action. The desired appearance of such data in system environments lacking out of box support for treating data as "visual" (e.g. MS Windows) can be achieved by use of control characters LRO/RLO (U+202C/U+202D). ___________ More information: If BidiSegmentListener enhancements sound acceptable in principle, I can post a detailed proposal of the expected code/API changes.
Actually, no changes would be applied to the BidiSegmentListener per se. I just noticed it's often used as a nickname for the Bidi segments support in general.
Changing version tag to something more believable.
Felipe: I basically completed preliminary changes for win32. I aimed to preserve the existing BidiSegments API and logic to ensure back-compatibility. I have some doubts as to some parts of the new implementation though. 1. I am introducing a new instance variable to TextLayout, BidiSegmentEvent, and StyledTextEvent. It will hold an array of character to be applied at the segment boundaries. And, should the old (and default) Segments mechanism be in effect, this variable will be null. In the real world it will mostly be null or 2-length array. 2. Indication of existance of actual BidiSegments for TextLayout: Until now, |segments.length == 2 && segments[0] == 0 && segments[1] == length| (maybe with some slight variations) used to serve as a sign of absence of Bidi segments. However, this is not valid for the new approach, since there can be just a lone segment in the buffer, no meaningful segments at positions 0 and segments.length, etc. So I am extending the above condition to |*segmentValues == null* && segments.length == 2 && segments[0] == 0 && segments[1] == length| (with some slight variations as well). Any comments please?
I think you should start with TextLayout. My first impression is that this new bidi segments is not compatiable with old one. The old one the entire text (from zero to length) has to be split in segments. So the first offset is always zero, and last always length. Here a example with 4 segments. 0..N1..N2..N3..Length S1 S2 S3 S4 Bidi reordering happens independently within each segment. See javadoc for BidiSegmentEvent. ----- For the new API, the way I understood it so far you have: int[] offsets char[] characters (offsets.length == characters.length) At the offset[i] the characters[i] is inserted. the first offset doesn't need to be zero, the last doesn't need to be length, etc. The only compability is that, when characters == null, the old formula is used: char separator = orientation == SWT.RIGHT_TO_LEFT ? RTL_MARK : LTR_MARK; ----- What is this API going to look like in TextLayout ? public void setSegments(int[] segments, char[] characters) am I missing anything ?
(In reply to comment #4) I think we can find quite much common between old and new Segments if we treat either as some kind of Bidi override (no matter what exact override it is). > For the new API, the way I understood it so far you have: > int[] offsets > char[] characters > (offsets.length == characters.length) > At the offset[i] the characters[i] is inserted. > the first offset doesn't need to be zero, the last doesn't need to be length, > etc. > The only compability is that, when characters == null, the old formula is used: > char separator = orientation == SWT.RIGHT_TO_LEFT ? RTL_MARK : LTR_MARK; > ----- > What is this API going to look like in TextLayout ? > public void setSegments(int[] segments, char[] characters) > am I missing anything ? - Yes
By "yes" I meant that everything you mentioned is implemented
Actually, segments at the edge offsets (0 and text length) seem not mandatory even for the old Segments. They don't do any harm, but also do not add any value from the Bidi perspective.
Created attachment 149826 [details] Patch V1 This is a preliminary patch to give a general impression of the proposed changes... Currently it is missing at least: - a method to obtain both segment offsets and segment values, - some updates for StyledText#getBidiSegmentsCompatibility.
Created attachment 149827 [details] Snippet that leverage the new segments functionality The snippet shows how a visual LTR editor can be created using the new Segments.
Created attachment 149828 [details] Snippet that uses the old Segments
Do we support segments of 0 legth ? For example would it be possible to ahcieve something like this: 0.........5...........10...........15......20 S1 S2 S3 S4 S5 At offset 5 we want to insert LRE At offset 10 we want to insert PDF + RLE At offset 15 we want to insert PDF So, at offset 10 we will have segment (S3) with zero length. Transforming this to the actual call we will get something like: setSegments([0,5,10,10,15,20], [,LRE,PDF,RLE,PDF,]) The question is this kind of call is a supported one ?
(In reply to comment #11) This was aimed by the new Segments and, in general, more advanced complex expressions were one of the primary goals ("classic" complex expressions are supported for a long time already). BTW in your example the base text direction is likely RTL (otherwise, one would just set LTR base direction instead of using the LRE embeddings). So we'd want to add the RLM character somewhere between the segments to avoid their intermixing. E.g. ..<LRE>..<PDF><RLM><LRE>..<PDF>..
I'm fine with the patch, some notes: 1. We need to make sure that bidi segments doesn't start running in cases it didn't run before. In the old code, text=segementText when: if (nSegments <= 1) or if (nSegments == 2) { if (segments[0] == 0 && segments[1] == length) return text; } Now the code runs always when segments != null. 2. The new code allows for segments[i]==segments[i+1] that used to throw an invalid arg exception. I suppose we need to change this in order to allow for the case requested by Tomer in comment 11. 3. two calls to the listener everytime: getBidiSegments, getBidiSegmentValues fix that by returning the event from getBidiSegments. 4. segmentValues not sure I like this name. Maybe segmentSeparators or controlCharacters. 5. ignore getBidiSegmentsCompatibility, it is deprecated. 6. TextLayout#getSegmentSeparator, why don't just inline this code, so simple. 7. setSegments(int[] offsets, char[] values) calls setSegments(int[] segments) That is backwards, put the code all together in setSegments(int[] offsets, char[] values) and make setSegments(int[] segments) used it. Except by item 1, all others are minor details.
(In reply to comment #13) Thanks, Felipe, for the comments. > 1. We need to make sure that bidi segments doesn't start running in cases it > didn't run before. > In the old code, text=segementText when: > if (nSegments <= 1) > or > if (nSegments == 2) { > if (segments[0] == 0 && segments[1] == length) return text; > } > Now the code runs always when segments != null. I am likely missing something but I think no - the code runs when (segments != null) AND (nSegments >= 1) Otherwise, text=segmentsText. See TextLayout#getSegmentsText(): if (segments == null) return text; int nSegments = segments.length; if (nSegments < 1) return text; > 2. The new code allows for segments[i]==segments[i+1] > that used to throw an invalid arg exception. > I suppose we need to change this in order to allow for the case requested by > Tomer in comment 11. segments[i]==segments[i+1] is allowed exactly to enable 2 or more adjacent control characters. Now the exception is only thrown when segments[i] > segments[i+1] > 3. two calls to the listener everytime: getBidiSegments, getBidiSegmentValues > fix that by returning the event from getBidiSegments. Yes, I intended to do so. > 4. segmentValues not sure I like this name. Maybe segmentSeparators or > controlCharacters. > 5. ignore getBidiSegmentsCompatibility, it is deprecated. > 6. TextLayout#getSegmentSeparator, why don't just inline this code, so simple. > 7. setSegments(int[] offsets, char[] values) calls setSegments(int[] segments) > That is backwards, put the code all together in setSegments(int[] offsets, > char[] values) and make setSegments(int[] segments) used it. OK
> Maybe segmentSeparators or controlCharacters Basically it will be possible to use any code points (not necessarily control characters), so I will pick up 'segmentSeparators'... BTW this can be probably leveraged for non-Bidi purposes as well. For example, application can use segments to apply some pattern whereas the formatting codes should be exposed in display only, not being introduced to the buffer.
> 1. We need to make sure that bidi segments doesn't start running in cases *it > didn't run before*. Apologies. I shouldn't have removed the validation but extend it as mentioned in comment 3.
(In reply to comment #15) > > Maybe segmentSeparators or controlCharacters > Basically it will be possible to use any code points (not necessarily control > characters), so I will pick up 'segmentSeparators'... I talked to Silenio about this. He likes segmentChars. Other ideas were characters, separators. We will probably change our minds a few times before we release the code ;-) > BTW this can be probably leveraged for non-Bidi purposes as well. > For example, application can use segments to apply some pattern whereas the > formatting codes should be exposed in display only, not being introduced to the > buffer. Right, maybe it is interesting to have non-bidi usage for this API. Right now the name of the event has Bidi in it, but we will have to add a new event to org.eclipse.swt.events in order to fix bug 230854. I think all new features should go to this new event, and BidiSegmentEvent becomes a (empty) subclass of it. Any suggestions for a name for this Event ? SegementEvent ? (try not to use bidi in the name).
Felipe: Sorry, it seems that by respecting the compatibility we may create a limitation for future users. Cases when the compatibility would be broken are: (1) presence of 2 segments with offsets of 0 and text length, (2) less than 2 segments and segments != null However, those seem to be pretty edge cases, since normally users are not indicating segments unless they are truly present. Another case when the compatibility won't be provided is: (3)segments[i]==segments[i+1] and segmentSeparators == null - and I am going to address this case... What do you think?
> Any suggestions for a name for this Event? Maybe FormattingEvent or FormatEvent?
(In reply to comment #19) > > Any suggestions for a name for this Event? > Maybe FormattingEvent or FormatEvent? I thought of that. Silenio didn't like it too much. Please, don't be stuck waiting for us to decide the names. I think we should finish TextLayout first, on all plaforms. Then we done StyledText.
To sum up expected TextLayout changes: 1. Separators will ALWAYS be NULL shall the old behavior be in effect. 2. Separators will ALWAYS be NOT NULL shall the new behavior be in effect. [So e.g. for setSegments(segments[], null) separators will constitute a zero-length array.] 3. setSegments(int[]) and setSegments(int[], char[]) are actually independent. The first one is for compatibility reasons, new users should call the second one. Users can call them subsequently and the result should be appropriate (consistent with the last call). [It is possible however for the new method to reuse the old one. The only problem seems freeRuns - we don't need to invoke it twice. Perhaps setSegments(int[]) can return boolean to indicate if the runs got freed?) ------------- Consequently, there would be clear criteria which exact Segments logic is effective. And, on one hand, there will be full compatibility with the old Segments, and on the other - maximum flexibility for the new Segments users. Does it sound okay, Felipe? Do you have any comments?
Created attachment 150247 [details] windows patch for TextLayout
Hi Lina, Silenio and I talked a bit more yesterday and we decided to add new API that will work togheter with the existent API. I wrote the code this morning (modified from the first patch). Please, see the code (sorry, I didn't have time to test it at all). calling only setSegmentsChars() doesn't do anything. calling only setSegments() is the old behaviour. calling setSegments() and setSegmentsChars() enables the new behaviour. When setSegmentsChars() is used, the usual recomendations for setSegments() do not apply. In another words, the first element doesn't need to be zero, the last element doesn't need to be length, etc. Lina, please see that this approach works for you. See that it works on all platforms. We will also need some junit testing for this new API. We also need to improve the javadoc. Once we are finished with TextLayout we can move to StyledText.
Hi Felipe, Thank you, this does work for us. Unfortunately we will not be able to test on platforms other whan win32 and gtk (sorry!)
Created attachment 150475 [details] Win32 patch for TextLayout slightly modified The only significant difference with the previous patch is that more than 1 separators are disallowed at the end of the buffer if segmentsChars == null. Felipe, please see if it makes sense.
Created attachment 150476 [details] Incremental diffs Only diffs between 2 last win32 TextLayout versions
> ... more than 1 separators are disallowed at the end of the buffer if > segmentsChars == null Actually, this seems to be redundant, since StyledText#getBidiSegments will reject segments[i] = segments[i - 1]...
- Probably not, since StyledText#getBidiSegments are not used by TextLayout@getSegmentsText.
I don't mind testing the other plaforms for you. Do you have automated junit test case for the new API ? >The only significant difference with the previous patch is that more than 1 >separators are disallowed at the end of the buffer if segmentsChars == null. >Felipe, please see if it makes sense. I won't do it, the doc doesn't say about that.
Felipe, I was planning to upload 2 tests in form of Snippets (and more thorough unit test for entire Segments API after StyledText is complete) (1) For new Segments: - 2 adjacent segments are being set at offset 0 - 2 adjacent segments at the end of the buffer - 1 segment after every second character - 2 adjacent segments after every third characters (as a result more than 2 adjacent segments may occur) (2) For old Segments regression (success): - 1 segment at offset 0 - 1 segment at the end - 1 segment after every second character (3) For old Segments regression - 2 (failure): - 1 single segment in sum (both) - segmentsChars will be something visible in display (e.g. '*', '%') to facilitate testing. Does it sound okay, or each aspect needs a dedicated test? Do you have any other comments / recommendations?
Sounds goot to me.
Created attachment 150521 [details] TextLayout Patch for platforms other than win32 Port of win32 TextLayout changes to other platforms
(In reply to comment #32) > Created an attachment (id=150521) [details] > TextLayout Patch for platforms other than win32 > > Port of win32 TextLayout changes to other platforms the patch causes compiler errors in GTK...
(In reply to comment #33) > the patch causes compiler errors in GTK... The same goes for carbon, the code has some other problems: 1. In GTK, the invalidOffsets (initialized in computeRuns()) is wrong. 2. In Carbon and Cocoa, the code that shrinks the invalidOffets arrays was removed (at the end of getSegmentsText(). 3. In GTK, Carbon, and Cocoa, the return line changed from: return new String(newChars, 0, Math.min(charCount + segmentCount, newChars.length)); to: return new String(newChars, 0, newChars.length); ---- Note, it is possible that 2 and 3 are okay changes cause now multiple offsets at line length are allowed, if that is the case, then change 3 should also be present in windows. (test needed).
Created attachment 150529 [details] Corrected TextLayout Patch for platforms other than win32 I am really sorry. It seems I previously attached an intermediate version of the patch.
(In reply to comment #34) > The same goes for carbon, > the code has some other problems: > 1. In GTK, the invalidOffsets (initialized in computeRuns()) is wrong. Yes, this should be fixed. > 2. In Carbon and Cocoa, the code that shrinks the invalidOffets arrays was > removed (at the end of getSegmentsText(). > 3. In GTK, Carbon, and Cocoa, the return line changed from: > return new String(newChars, 0, Math.min(charCount + segmentCount, > newChars.length)); > to: > return new String(newChars, 0, newChars.length); > ---- > Note, it is possible that 2 and 3 are okay changes cause now multiple offsets > at line length are allowed, Yes, this was intentional, since now segmentCount == nSegments > if that is the case, then change 3 should also be present in windows. (test needed). I am testing it right now.
I found a problem during the unit testing: With 2 or more segments at the same offset, getOffsetAtPoint can result in offset equal to length and trailing = 1. At a glance it looked like (un)translate offset needs to be updated to enable zero-width segments, but now I think it may be a deeper problem.
(In reply to comment #37) > I found a problem during the unit testing: > With 2 or more segments at the same offset, getOffsetAtPoint can result in > offset equal to length and trailing = 1. > At a glance it looked like (un)translate offset needs to be updated to enable > zero-width segments, but now I think it may be a deeper problem. can you post the test case ? is it a new bug ? does it only happen to those who use the new API ?
Created attachment 151170 [details] getOffset test for segmented text This is a minimal test case not involving StyledText that I have for TextLayout#getOffset. Here TextLayout originally hosts 7 characters. One segment is set at the end of text. Segment char is a non-zero-width character. Expected offset at the end of text is 6 with trailing = 1 Actual offset at the end of text is 7 with trailing = 1 Output streams to console..
(In reply to comment #39) Sorry - display.dispose should not be there (although it doesn't harm), it remained from an old test case
(In reply to comment #38) > is it a new bug ? does it only happen to those who use the new API ? Yes, this is a new bug. It's firstly got exposed in StyledText when clicking on the right outside the text bounds. I see today that it only happens when at least one segment offset equals to text length AND the corresponding segment char has non-zero width. I will investigate further..
Created attachment 151171 [details] getOffset test for segmented text - slightly sanitized
Created attachment 151175 [details] getOffset test for segmented text - 2 This is another test case I've used. It is supposed to print selection offset on the fly, on mouse down event.
I verifed the problem. I don't think it is that bad. It only affects user using the new API setSegmentsChars passing regular characters (width>0). It works fine when Unicode Control Characters are used. The doc says the API should be used with Unicode Control Characters. If we have too, I do think we can fix it in: untranslateOffset/translateOffset/validadeOffset I think (didn't try) that if we changed win32 to handle translate/untranslate the same way GTK does, the problem would be fixed. Is there any other problems stopping us ?
It seems that the cause is as follows. (I am looking at win32 TextLayout for now...) The last character (being a segment char) forms a separate run. When TextLayout#getOffset searches for the run at a given point (which is the rightmost edge), this last significant run constitues the best candidate. The the offset of this run is segmented text length and trailing = 1. Then it's passed to untranslateOffset which results in "lean"/normalized text length and trailing = 1. I think TextLayout#getOffset should just skip runs with offsets matching segment offsets when iterating over the runs. Alternatively (or additionally), the problem can be worked in TextLayout#untranslateOffset by modifying its for loop to: for (int i = 0; i < nSegments && offset > 0 && offset >= segments[i]; i++) { offset--; } This seems to pass unit test and even may be more correct than the current: for (int i = 0; i < nSegments && offset > segments[i]; i++) { offset--; } What do you think, Felipe?
(In reply to comment #44) > I verifed the problem. I don't think it is that bad. > It only affects user using the new API setSegmentsChars passing regular > characters (width>0). It works fine when Unicode Control Characters are used. > The doc says the API should be used with Unicode Control Characters. > If we have too, I do think we can fix it in: > untranslateOffset/translateOffset/validadeOffset > I think (didn't try) that if we changed win32 to handle translate/untranslate > the same way GTK does, the problem would be fixed. Felipe, what do you prefer: to comply with the doc or try to address translate/untranslate? > Is there any other problems stopping us ? No, AFAICS...
(In reply to comment #45) > ... The > the offset of this run is segmented text length and trailing = 1. > Then it's passed to untranslateOffset which results in "lean"/normalized text > length and trailing = 1. Meaning, segmented/normalized |text length - 1|)
Okay, I decided to test this a bit, this never works: public void test_getSegments() { TextLayout layout = new TextLayout(display); layout.setText("AB"); String[] messages = {"no segments", "segments", "segments (duplicate at 0)", "segments (duplicate at 1)", "segments (duplicate at 2)"}; int[][] segments = {null, {0,1,2}, {0,0,1,2}, {0,1,1,2}, {0,1,2,2}}; for (int i = 0; i < segments.length; i++) { String m = messages[i]; layout.setSegments(segments[i]); assertEquals(m, 1, layout.getNextOffset(0, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 2, layout.getNextOffset(1, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 2, layout.getNextOffset(2, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 1, layout.getPreviousOffset(2, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 0, layout.getPreviousOffset(1, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 0, layout.getPreviousOffset(0, SWT.MOVEMENT_CLUSTER)); } layout.dispose(); } ---- the current untranslateOffset() fails, and the one you suggested also fails. We need a fix for this before we release the new API. I suggestion is: try the GTK code, if it works we should change all platforms to use the same code.
Added Test_org_eclipse_swt_graphics_TextLayout to our test suite (org.eclipse.swt.tests). the test case I post in comment 48 in there (commented out, of course). I verified that GTK does't have the problems described in comment 48, probably it doesn't have any of the other problems either. Lina, when you post new testcase use the junit format, that way it will be easy for me to merge with the official test suite. Thank you.
Sorry for the slow response - my Linux machine crashed and I just managed to make it working. Felipe, the new TextLayout (with the new segments enabled) passes your test (comment #48) but fails my manual test (like in comment #43). I am trying to figure out what's wrong. Sorry again for the delay.
Created attachment 151416 [details] Win32 patch for TextLayout - another version I think TextLayout#_getOffset should advance the offset by the given step always with respect to the "untranslated" text. It was actually handled by "translate" in conjunction with "validate" - but sometimes the result seems to be inaccurate. Run content inspection needs to still be done WRT to the "translated" text.
Felipe, I am looking for some test *data* to verify MOVEMENT_CLUSTER. Perhaps you have something handy? Thanks!
(translate/untranslate methods in the patch got increased visibility for the testing purposes. Please ingore this.) I verified a similar patch on gtk, it seems to work.
(In reply to comment #52) Felipe, I see now that you added Thai clusters test in test_getNextOffset2
Created attachment 151681 [details] Test_org_eclipse_swt_graphics_TextLayout another segments test JUnit test for verifying embedding levels in segmented text. (This addition needs new segments to be enabled.)
Modified test from comment 48 to validate MOVEMENT_CLUSTER with Thai text: public void test_getSegments2() { TextLayout layout = new TextLayout(display); layout.setText("A\u0E19\u0E49\u0E33B"); String[] messages = {"no segments", "segments", "segments (duplicate at 0)", "segments (duplicate at 1)", "segments (duplicate at 2)", "segments (duplicate at 3)", "segments (duplicate at 4)", "segments (duplicate at 5)"}; int[][] segments = {null, {0, 1, 2, 3, 4, 5}, {0, 0, 1, 2, 3, 4, 5}, {0, 1, 1, 2, 3, 4, 5}, {0, 1, 2, 2, 3, 4, 5}, {0, 1, 2, 3, 3, 4, 5}, {0, 1, 2, 3, 4, 4, 5}, {0, 1, 2, 3, 4, 5, 5}}; for (int i = 0; i < segments.length; i++) { String m = messages[i]; layout.setSegments(segments[i]); assertEquals(m, 1, layout.getNextOffset(0, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 4, layout.getNextOffset(1, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 5, layout.getNextOffset(4, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 4, layout.getPreviousOffset(5, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 1, layout.getPreviousOffset(4, SWT.MOVEMENT_CLUSTER)); assertEquals(m, 0, layout.getPreviousOffset(1, SWT.MOVEMENT_CLUSTER)); } layout.dispose(); }
Created attachment 151813 [details] Updated GTK patch Felipe, in addition to _getOffset, there was also slightly changed untranslateOffset (in the previous win32 patch and this one). I verified both this gtk patch and win32 patch (attachment 151416 [details]) against Test_org_eclipse_swt_graphics_TextLayout (including new TCs for levels). The GTK patch fails with Thai clusters (test_getNextOffset2) but the comment says it should be excluded for gtk (rhel4). BTW it still doesn't work on my rhel5.
I also verified translate/untranslate directly in the following internal informal test, for win32 and gtk: TextLayout layout = new TextLayout(display); String text = "AB"; int textLength = text.length(); layout.setText(text); String[] messages = {"no segments", "segments", "segments (duplicate at 0)", "segments (duplicate at 1)", "segments (duplicate at 2)"}; int[][] segments = {null, {0, 1, 2}, {0, 0, 1, 2}, {0, 1, 1, 2}, {0, 1, 2, 2}}; int[][] translatedOffsets = {{0, 1, 2}, {1, 3, 5}, {2, 4, 6}, {1, 4, 6}, {1, 3, 6}}; int[][] untranslatedOffsets = {{0, 1, 2}, {0, 0, 1, 1, 2, 2}, {0, 0, 0, 1, 1, 2, 2}, {0, 0, 1, 1, 1, 2, 2}, {0, 0, 1, 1, 2, 2, 2}}; for (int i = 0; i < segments.length; i++) { layout.setSegments(segments[i]); layout.getBounds(); for (int j = 0; j <= textLength; j++) { assertEquals(messages[i] + " j = " + j, translatedOffsets[i][j], layout.translateOffset(j)); } for (int j = 0, n = layout.getSegments() == null ? 0 : textLength + layout.getSegments().length; j < n; j++) { assertEquals(messages[i] + " j = " + j, untranslatedOffsets[i][j], layout.untranslateOffset(j)); } }
Created attachment 151856 [details] Updated GTK patch Small update for GTK... computeRuns should just nullify invalidOffsets if their expected count is equal to 0.
Created attachment 151888 [details] Test patch (including all changes) Hi Lina, I put all the contributions to the test case in this patch. I also included one extra test to verify for the problem you found in comment 37.
Created attachment 151889 [details] Patch for win32 Hi Lina, check this version. These are the differences from your lastest win32 patch: 1) I changed back _getOffset() to have the original code. I fixed the problem in validateOffset(), using the your idea. 2) Changed back translateOffset() to my original patch 3) Changed back untranslateOffset() to my original patch + the change you suggested in comment #45. Your patch in comment #51 does not fix the problem in comment #45. Not sure is worth fixing the bug in comment #45, but this patch fixes it. I think we are done with win32. Tomorrow I'll work on the GTK patch.
Felipe, after applying the latest patch untranslate sometimes fails. This didn't happen with the patch I uploaded yesterday. Although untranslate is not part of public API, I think the problem can be exposed indirectly (e.g. in getLineOffsets). I will be out of my workstation during the next few hours and if necessary can provide further details later today.
(In reply to comment #62) > Felipe, after applying the latest patch untranslate sometimes fails. This > didn't happen with the patch I uploaded yesterday. > Although untranslate is not part of public API, I think the problem can be > exposed indirectly (e.g. in getLineOffsets). > I will be out of my workstation during the next few hours and if necessary can > provide further details later today. Do you have a real testcase that shows the problem ? If you think the code is wrong please write a testcase that proves it.
Hi Felipe, Here is a test case: public void test_getLineOffsets2() { TextLayout layout = new TextLayout(display); layout.setText("\nAB"); layout.setSegments(new int[] {0, 1, 3}); int[] expected = {0, 1, 3}; int[] offsets = layout.getLineOffsets(); for (int i = 0; i < offsets.length; i++) { assertEquals(" i = " + i, expected[i], offsets[i]); } layout.dispose(); }
We have three implementations for translate/untranslate: 1) Comment 22 (basically the original code) has the problem described in comment 37. 2) Comment 51 (lina's) has the problem described in comment 37. 3) Comment 61 (Same as comment 22 + change suggested in comment 45) fixes the problem described in comment 37. introduces problem described in comment 64 (regression). ---------------------------------- Note: - (3) is a no go (introduces a regression) - (2) returns exactly the same values as (1), I don't see the point of changing code in this case - We don't have a solution that works for all cases. I'm afraid we will have to live with problem #37
(In reply to comment #65) .... > - (2) returns exactly the same values as (1)... There is one difference: translate/untranslate in (2) compares segments[i] with "untranslated" offset (since segments[] host "untranslated" offsets), while other implementations - consult the "translated" offset...
(In reply to comment #66) > (In reply to comment #65) > .... > > - (2) returns exactly the same values as (1)... > There is one difference: > translate/untranslate in (2) compares segments[i] with "untranslated" offset > (since segments[] host "untranslated" offsets), while other implementations - > consult the "translated" offset... Does it or does it not return the same values as (1) ?
It returns a different value for untranslate, e.g.: text = AB segments = {0, 1, 2} segmentsText = |A|B| expected for translate = {1, 3, 5} expected for untranslate = {0, 0, 1, 1, 2, 2} Variation (3) returns: translateOffset(0) = 1 translateOffset(1) = 3 translateOffset(2) = 5 untranslateOffset(0) = 0 untranslateOffset(1) = 0 untranslateOffset(2) = 0 (not OK) untranslateOffset(3) = 1 (not OK) untranslateOffset(4) = 1 (not OK) untranslateOffset(5) = 2 (not OK) Variation (2) returns: translateOffset(0) = 1 translateOffset(1) = 3 translateOffset(2) = 5 untranslateOffset(0) = 0 untranslateOffset(1) = 0 untranslateOffset(2) = 1 untranslateOffset(3) = 1 untranslateOffset(4) = 2 untranslateOffset(5) = 2
> untranslateOffset(3) = 1 (not OK) > untranslateOffset(5) = 2 (not OK) sorry - these are OK
Felipe, sorry for the confusion. I thought you meant (2) vs (3). Yes, (1) returns the same results as (2).
For gtk computeRuns, invalid offset count could be initialized as int offsetCount = segmentsText.length() - text.length();
Created attachment 152092 [details] Patch for carbon Felipe, I only verified there is no compilation errors in the carbon patch by replacing the classpath. Sorry in advance for any inconvenience.
It's possible that translate, untranslate, _getOffset need to be modified for carbon. I didn't want to touch them at this point. Please let me know if you expect me to do anything in this regard though.
Review of GTK patch in comment#59 The code in computeRuns() is wrong: In here: if (segments != null) { while (segmentCount < segments.length && i == segments[segmentCount]) { invalidOffsets[offsetCount++] = i; segmentCount++; } } Note that "i" is internal offset and "segments[segmentCount]" is a client offset. Here is a test case: layout.setText("word word word"); layout.setSegments(new int[] {0, 5, 10}); int offset = 0; offset = layout.getNextOffset(offset, SWT.MOVEMENT_WORD_START); assertEquals(5, offset); offset = layout.getNextOffset(offset, SWT.MOVEMENT_WORD_START); assertEquals(10, offset); offset = layout.getNextOffset(offset, SWT.MOVEMENT_WORD_START); assertEquals(14, offset); layout.setWidth(layout.getBounds().width / 2); layout.setAscent(20); layout.setDescent(6); offset = 0; offset = layout.getNextOffset(offset, SWT.MOVEMENT_WORD_START); assertEquals(5, offset); offset = layout.getNextOffset(offset, SWT.MOVEMENT_WORD_START); assertEquals(10, offset); offset = layout.getNextOffset(offset, SWT.MOVEMENT_WORD_START); assertEquals(14, offset); layout.setWidth(-1); ----------------------------------------- Please fix it.
Created attachment 152244 [details] Patch for gtk
Patch for gtk (attachment #152244 [details]) works for me against all available test cases, including that given by Felipe in comment #59. Compared to the previous version of the patch, it includes: - fix for the problem described in comment #59, - revert translateOffset back, - changed untranslateOffset to a bit more compact form (however, both original and modified versions seem to work correctly).
Created attachment 152384 [details] Patch for motif
Created attachment 152386 [details] Patch for emulated platforms
The remaining platforms are cocoa and wpf. I think that for cocoa we need gtk changes to be finalized, and for wpf - win32 ones.
Gtk is still broken, same place as before (see comment 74) Here is the testcase: layout.setText("MNMNMN"); layout.setSegments(new int[] {0,1,6}); layout.getBounds(); assertEquals(layout.getText().length() - 1, layout.getOffset(layout.getBounds().width, 0, null)); layout.setAscent(9); assertEquals(layout.getText().length() - 1, layout.getOffset(layout.getBounds().width, 0, null)); Use Test_org_eclipse_swt_graphics_TextLayout for testing (I've released all test cases there).
Created attachment 152414 [details] Patch for gtk The patch is verified with the latest Test_org_eclipse_swt_graphics_TextLayout.
Created attachment 152419 [details] Patch for gtk The same patch with a very minor change.
Released the code in all platforms. win32, cocoa, and gtk have full support motif, wpf, and emulated have a no-op API carbon right now is no-op, I plan to finish it tomorrow. Now we can decide on the API for StyledText and finish the work.
(In reply to comment #83) > Now we can decide on the API for StyledText and finish the work. Hi Felipe, I'll try to outline the changes. - class BidiSegmentEvent will be added a new field |public char[] segmentChars| (Also, the javadoc should be significantly updated.) - class StyledTextEvent will have such a field (|char[] segmentChars|) as well (used by BidiSegmentEvent) - StyledText will be added 3 methods: * StyledTextEvent getLineGetSegments(int lineOffset, String line) * int[] getBidiSegments(StyledTextEvent event, int lineOffset, String line) * char[] getBidiSegmentChars(StyledTextEvent event, int lineOffset, String line) - which will be invoked by StyledTextRenderer and communicated to the TextLayout. Also, it may be necessary to introduce a field holding segmentChars to StyledTextRenderer#LineInfo. What do you think?
(In reply to comment #84) > - class BidiSegmentEvent will be added a new field |public char[] segmentChars| > (Also, the javadoc should be significantly updated.) Yes, that is only API change. > - class StyledTextEvent will have such a field (|char[] segmentChars|) as well > (used by BidiSegmentEvent) yes, this is implementation detail. StyledTextListener it also be extented the same way. > - StyledText will be added 3 methods: > * StyledTextEvent getLineGetSegments(int lineOffset, String line) > * int[] getBidiSegments(StyledTextEvent event, int lineOffset, String line) > * char[] getBidiSegmentChars(StyledTextEvent event, int lineOffset, String > line) Why is that? The only change I expected to StyledText is changing the current: int [] getBidiSegments(int lineOffset, String line) to: StyledTextEvent getBidiSegments(int lineOffset, String line) > Also, it may be necessary to introduce a field holding segmentChars to > StyledTextRenderer#LineInfo. Yes, for bidi segments, this is only used by the printer. The code is easy, I'll okay the API with Silenio and the rest is easy.
(In reply to comment #82) > Created an attachment (id=152419) [details] > Patch for gtk > > The same patch with a very minor change. GTK is still broke, we broke the build this time.... Bug 295513
(In reply to comment #85) > (In reply to comment #84) > > - StyledText will be added 3 methods: > > * StyledTextEvent getLineGetSegments(int lineOffset, String line) > > * int[] getBidiSegments(StyledTextEvent event, int lineOffset, String > > line) > > * char[] getBidiSegmentChars(StyledTextEvent event, int lineOffset, String > > line) > Why is that? > The only change I expected to StyledText is changing the current: > int [] getBidiSegments(int lineOffset, String line) > to: > StyledTextEvent getBidiSegments(int lineOffset, String line) The other two were to address the case when StyledText is Bidi and it is not listening for segments. If we need to back such a scenario, then (alternatively) some logic related to BidiSegmentCompatibility should be migrated to the caller code, or StyledTextEvent getBidiSegments(int lineOffset, String line) would have to create some fake StyledTextEvent. Or probably BidiSegmentCompatibility doesn't need to be addressed?
(In reply to comment #87) > Or probably BidiSegmentCompatibility doesn't need to be addressed? I'm not 100% sure I understood the scneario. Nevertheless, we don't need to touch the bidi segment compatibility, it is only there to support the deprecated setBidiColoring() API. Please attach the patch with the changes for StyledText.
Created attachment 152617 [details] Patch for org.eclipse.swt.custom Felipe, please review the patch. Please note that I started changing the BidiSegmentEvent doc but have not yet finished it.
I just discovered some issue and will resubmit the patch. (In reply to comment #88) > (In reply to comment #87) > > Or probably BidiSegmentCompatibility doesn't need to be addressed? > I'm not 100% sure I understood the scneario. Nevertheless, we don't need to > touch the bidi segment compatibility, it is only there to support the > deprecated setBidiColoring() API. StyledText#getBidiSegments currently has the following code: if (!isBidi()) return null; if (!isListening(LineGetSegments)) { return getBidiSegmentsCompatibility(line, lineOffset); } // else emit event Is it still necessary to back the case when isBidi() = true and !isListening(LineGetSegments), obtaining segments from getBidiSegmentsCompatibility? If so, I think this can be handled at the caller side.
Another question... Same place, getBidiSegments, used to always set segments start to 0, and end index - to the line length: if (event == null || event.segments == null || event.segments.length == 0) { segments = new int[] {0, lineLength}; } Also, the doc says about segments indices: "Always starts with 0 and ends with the line length". Felipe, should this behavior be maintained intact? Or it is okay to assume that segments = null in the above case (even when segmentChars = null)? Thanks.
Created attachment 152635 [details] Patch for org.eclipse.swt.custom (Felipe) This what I expected. I hope the code answer your questions. Let me know if there is any problems in the code. (I got your code and I modified a couple places). If that is code is okay, the only thing left is fixing the java doc.
(In reply to comment #92) > Created an attachment (id=152635) [details] > Patch for org.eclipse.swt.custom (Felipe) > This what I expected. I hope the code answer your questions. Thanks for the answers :) I tested the patch on win32 and gtk and it worked fine for me. I have one question. You mentioned earlier that the case when |segments[i] = segments[i+1] and segmentsChars = null| may not be addressed, since the doc doesn't say anything about that (although trailing segments only were discussed explicitly). And this is currently permissible in TextLayout. However, StyledText#getBidiSegments will invalidate this case: if (event.segmentChars == null) { .... if (segments[i] <= segments[i - 1] ...) { SWT.error(SWT.ERROR_INVALID_ARGUMENT); } .... } Should this be permitted in StyledText as well?
(In reply to comment #93) > Should this be permitted in StyledText as well? I think not, when event.segmentChars == null it runs the old code and all the old rules apply. I'll try to release the final patch tomorrow.
Fixed in HEAD > 20091124, please verify Thank you Lina for all the work.
Verified in HEAD. Felipe, thanks a lot for your incredible support.
