Bugzilla – Full Text Bug Listing
| Summary: | [block selection] Block selection editing does not work with Korean encoding | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Szymon Ptaszkiewicz <sptaszkiewicz> | ||||||||||||||||||
| Component: | Text | Assignee: | Szymon Ptaszkiewicz <sptaszkiewicz> | ||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||
| Priority: | P3 | CC: | daniel_megert | ||||||||||||||||||
| Version: | 3.6.2 | Keywords: | helpwanted | ||||||||||||||||||
| Target Milestone: | 3.6.2+ | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Created attachment 219475 [details]
Image showing the problem
Works in StyledText. There's at least 1 bug in org.eclipse.jface.internal.text.SelectionProcessor.COLUMN_IMPLEMENTATION.new Implementation() {...}.visualSizeIncrement(char, int).
Created attachment 219515 [details]
Appropriate Korean fixed-width font
I have investigated this case further and the problem is that default font used in block selection mode is Courier and it does not handle Korean characters properly. That is why Korean characters are not fixed-width as they should be. If someone is using characters that are not supported by a given font, then block selection may not work properly. The right solution is to use dedicated Korean fixed-width font that can handle Korean characters properly. One of the fonts that I found is "GulimChe" and the test case works fine when using this font. See the attached image how it looks like.
Closing as WORKSFORME. Reopening. It turns out that I already tweaked the code before investigating thus I received misleading results. The bug is indeed in visualSizeIncrement(..) as Dani mentioned in comment 2. Created attachment 219589 [details]
Patch
Comment on attachment 219589 [details]
Patch
There are lots of characters > 255 that only use 1 visual character.
Does your incomplete patch really fix the scenario from comment 0? Note that said scenario starts in the middle of the Korean character. Created attachment 219595 [details]
Fix for visualSizeIncrement
As outlined in the previous comment, I doubt that this is enough to fix the bug.
Created attachment 219602 [details]
showing a problem
I don't think this is enough to fix the bug. After applying a fix for visualSizeIncrement, I found a case which exhibits it doesn't work completely. Please have a look at a new attchment.
I think that there may be a couple of more problems.
(In reply to comment #10) > Created attachment 219602 [details] > showing a problem > > I don't think this is enough to fix the bug. After applying a fix for > visualSizeIncrement, I found a case which exhibits it doesn't work > completely. Please have a look at a new attchment. > I think that there may be a couple of more problems. Yes, I know. Like I said in comment 1: "There's at least 1 bug in ...". (In reply to comment #8) > Does your incomplete patch really fix the scenario from comment 0? Note that > said scenario starts in the middle of the Korean character. No, it just makes it work a little better but it doesn't solve all problems. I tried to find more complete fix for this problem, but it turns out that proper handling of editing in block selection mode with characters of different width and in the middle of another character is not trivial. In particular, should the new character be put before or after the existing character? Or, if we select in block selection half of the Korean character and try to replace it with another character, should the replace work in spite of which half (left or right) of the character we selected? I guess that the root problem is that block selection mode assumes that all characters are of equal *visual* width, which in this case is not true. "GulimChe" is only partially fixed-width as it has two fixed widths: 1 for single-byte characters and 2 for double-byte characters. So comment 3 is not complete as the problem would be solved if we used truly fixed-width font, where the fixed-width means all characters are of the same width. This would make us unable to select half of the character and there would be no difference between single- and double-byte characters. I guess this is also the reason why the hint says "A monospace font should be used." Dani, what do you think? (In reply to comment #13) > I guess that the root problem is that block selection mode assumes that all > characters are of equal *visual* width, which in this case is not true. Just to be clear: I think there is nothing wrong with this assumption, it's just the way is works, so proper font is needed to make sure we don't get strange results. (In reply to comment #14) > (In reply to comment #13) > > I guess that the root problem is that block selection mode assumes that all > > characters are of equal *visual* width, which in this case is not true. > > Just to be clear: I think there is nothing wrong with this assumption, it's > just the way is works, so proper font is needed to make sure we don't get > strange results. Correct. Created attachment 222508 [details]
Patch v2
Some explanation:
- All characters are added on the left side of the cursor.
- If the cursor is in the middle of a character, then this character is assumed to be before the cursor, as if it was completely on its left side.
- Delete is peculiar because it removes characters from the right side of the cursor, so it requires special treating. Previously it didn't work if cursor was between two wider characters.
Patch fixes editing in block selection mode and makes it compliant with SWT TextEditor in all editing scenarios that I found.
(In reply to comment #16) > Created attachment 222508 [details] [diff] > Patch v2 > > Some explanation: > - All characters are added on the left side of the cursor. > - If the cursor is in the middle of a character, then this character is > assumed to be before the cursor, as if it was completely on its left side. > - Delete is peculiar because it removes characters from the right side of > the cursor, so it requires special treating. Previously it didn't work if > cursor was between two wider characters. > > Patch fixes editing in block selection mode and makes it compliant with SWT > TextEditor in all editing scenarios that I found. The patch goes into the right direction but now needs some polish. Please 1. only create the GC when needed (i.e. for block selection implementation) 2. dispose the GC when no longer needed - currently we leak it! 3. verify that callers of SelectionProcessor.SelectionProcessor(IDocument, int) are safe i.e. don't need the new code in order to work. (In reply to comment #17) > The patch goes into the right direction but now needs some polish. Please > 1. only create the GC when needed (i.e. for block selection implementation) > 2. dispose the GC when no longer needed - currently we leak it! These two are related but it seems there is no place where we would definitely know that GC is no longer needed. I think the best option would be to create and dispose it within visualSizeIncrement(char, int) and only when really needed i.e. only for chars > 255. > 3. verify that callers of SelectionProcessor.SelectionProcessor(IDocument, > int) > are safe i.e. don't need the new code in order to work. What kind of verification do you have in mind? There are actually two methods calling this constructor: org.eclipse.jface.text.BlockTextSelection.getRegions() org.eclipse.jface.text.BlockTextSelection.getText() So far block selection was not dependent on any graphics stuff because it assumed all characters are of equal width. Now we need at least StyledText (or any Drawable to be precise) and, as far as I can see, it is not possible to get it from IDocument or BlockTextSelection. We would need e.g. to add additional field inside BlockTextSelection to keep reference to StyledText so that we can use it later on to create SelectionProcessor and then GC. Is that at least near to what you meant? (In reply to comment #18) > (In reply to comment #17) > > The patch goes into the right direction but now needs some polish. Please > > 1. only create the GC when needed (i.e. for block selection implementation) > > 2. dispose the GC when no longer needed - currently we leak it! > > These two are related but it seems there is no place where we would > definitely know that GC is no longer needed. I think the best option would > be to create and dispose it within visualSizeIncrement(char, int) and only > when really needed i.e. only for chars > 255. Yes, I also had something like this in mind. > > 3. verify that callers of SelectionProcessor.SelectionProcessor(IDocument, > > int) > > are safe i.e. don't need the new code in order to work. > > What kind of verification do you have in mind? Verify whether there are problems if we don't change those callers. (In reply to comment #19) > > > 3. verify that callers of SelectionProcessor.SelectionProcessor(IDocument, > > > int) > > > are safe i.e. don't need the new code in order to work. > > > > What kind of verification do you have in mind? > > Verify whether there are problems if we don't change those callers. I think we're OK regarding this bug here but wanted you to double-check. (In reply to comment #18) > We would need e.g. to add > additional field inside BlockTextSelection to keep reference to StyledText > so that we can use it later on to create SelectionProcessor and then GC. Is > that at least near to what you meant? To clarify: I assumed we need StyledText because that's how I already modified the code - to have fStyledText instead of fGC inside SelectionProcessor. I will attach new version in a moment. Created attachment 222518 [details]
Patch v3
(In reply to comment #21) > (In reply to comment #18) > > We would need e.g. to add > > additional field inside BlockTextSelection to keep reference to StyledText > > so that we can use it later on to create SelectionProcessor and then GC. Is > > that at least near to what you meant? > > To clarify: I assumed we need StyledText because that's how I already > modified the code - to have fStyledText instead of fGC inside > SelectionProcessor. I will attach new version in a moment. Yes. (In reply to comment #20) > (In reply to comment #19) > > > > 3. verify that callers of SelectionProcessor.SelectionProcessor(IDocument, > > > > int) > > > > are safe i.e. don't need the new code in order to work. > > > > > > What kind of verification do you have in mind? > > > > Verify whether there are problems if we don't change those callers. > > I think we're OK regarding this bug here but wanted you to double-check. Regarding those callers, I am not familiar with lifecycle of SelectionProcessor and BlockTextSelection objects yet, but depending on their lifecycle, we may need to change them as well, so that we will always have fStyledText != null. Comment on attachment 222518 [details]
Patch v3
Thnaks Szymon, the patch is good and I think we can leave the other callers as is for now.
Note to testers: in order to test this you need to use a font that contains non-proportional version of the Korean characters, e.g. 'GulimChe'.
. Fixed in 3.6.2+ with: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=68e461273a0d70c7f3ccad86b52e496d7910a4dd http://git.eclipse.org/c/platform/eclipse.platform.releng.maps.git/commit/?id=82c6d04af4b056cfca16993dcb772eb2a6c0ccb6 Fixed in 3.6.2+J7 by updating the map file: http://git.eclipse.org/c/platform/eclipse.platform.releng.maps.git/commit/?id=82c6d04af4b056cfca16993dcb772eb2a6c0ccb6 Fixed in 3.7.2+ with: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=69f6b5859880de0ce5745d3f0d7a7d45d227e528 Fixed in 3.8.2 with: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=dee7737a1150e2e99d85bae2e35d735a1ec13bdb |
Created attachment 219474 [details] Test case project When using Korean encoding MS949 editing a text file in block selection mode does not work in the same way as it works for other encodings e.g. UTF-8. Steps to reproduce: 1. Import the attached project. MS949 is already set as default encoding. 2. Open a.txt file in Text Editor. 3. Enable Block Selection Mode. 4. Put the cursor at the end of "Block" in the first line and extend the selection to the bottom. Selection should cover several lines but no character. 5. Press Space key several times. => Space is added properly in all lines except for 2. In line 2, space is added after Korean characters. Note also that the width of space added in line 2 is different than in other lines.