Apache OpenOffice (AOO) Bugzilla – Issue 60594
Kashidas painted into space between two Arabic words
Last modified: 2013-08-07 14:43:11 UTC
When use the "Writer" programe to write an Arabic paragraph, it works well. But when make this paragraph justified, some words split with noticed spacing. Thanks
When useing "Writer" to write a Justified Arabic paragraph some words splits with a spaceing. Thanks.
SBA: Please re-verify your findings with OOo 2.2 (out NOW...) and comment here. If the problem still persists, please attach a bugdoc showing this behavior. Thank you.
Hi ahmed_aldesouky, any news on this issue?
This problem still exists. I'm testing on Debian SID (OOo 2.2). There are also additional unnecessary Kashidas added. Check the screenshot.
Created attachment 45156 [details] Showing Arabic text problem when text is justified.
Just to clarify things: This issue is about the kashidas painted between two Arabic words in justified alignment. There might be two reasons for this bug: 1. Normal Western justified alignment is applied to the text because the language attribute of the CTL text is not set to an Arabic language. This is a very common problem, since many users do not set the language attribute correctly. We have issue 28203 which should make the justification mode (Westers vc. Kashida) independent from the language attribute by evaluating the unicode code points. 2. A problem in the underlying layout engine. This issue is for 2. See issue 28203 for case 1.
The problem still exists with 2.3 though the rendering is a bit better now.
Created attachment 48739 [details] Spacing in Arabic Justified text
Adjusted title, see attached Screenshot.png. There is an other issue for gaps inside Arabic words: issue 34139.
.
Please see my comments and patches for issue 77976. For Windows: When you request to draw a justified Arabic word, but a glyph is stretched by a value smaller than the minimal kashida width allowed by the font, you get the gaps within the words. I suggested to correct that behavior in winlayout.cxx, but hdu mentioned problems involved with that. So I haven't come up with a patch for that.
Created attachment 52432 [details] patch for file vcl\win\source\gdi\winlayout.cxx
I posted a patch for winlayout.cxx, which adjusts character positions in Arabic text according to the iMinKashida value returned by ScriptGetFontProperties. The text appears now without the gaps inside the words. Sideeffects: 1. When applying color to single letters within a word, gaps may still appear. 2. The impact on cursor placement is minimal, as characters are moved by very few pixels 3. PDF export is entirely broken. 4. Of course, this is a Windows only solution. I understand that it would be better to adjust the character placement right away in writer, maybe in KashidaJustify. But I have to get the minimum kashida width value over there from vcl. But I really didn't know how to do that (surely not without messing up a lot of things). I'd be happy to help out with a more permanent solution. See also my patch in issue 87688 for the placing of kashidas.
fme->hdu: Since this patch affects your playground, I suggest you take over. Please have a look.
@hennerdrewes: thanks for the patch! Since the related VCL issue from issue 77976 is currently being integrated (probably into DEV300_m7, maybe even into m6) I'd like you to check your patch with this newer milestone. The file winlayout.cxx will probably look like in revision 1.108.206.2 then.
@hdu: I can do that, but that'll take some time. A more general question: - Do you want to integrate the patch in this or a similar form? - Or should this check move to writer where KashidaJustify is applied in the first place? The vcl should somewhere provide a MinKashidaWidth then. Related to my remarks from yesterday: PDF export with kashida justification hasn't worked before. This probably should go to a separate issue.
I'd prefer the second alternative to make the value for MinKashidaWidth available to the application layer that does the kashida justification and act on it in that layer.
@hdu: some additional thoughts I made some more experiments. I spare you the patches until they will be more useful or you explicitly request to get them. But I think I have some important hints to find a reasonable approach. - It seems that - no matter if writer will respect a minimal kashida width in its justification or not - the approach taken in my previous patch remains valid. Probably due to rounding errors from logical units to device units and due to a tendency of ScriptApplyLocigalWidths to be just off by one pixel in its results, I still observed the gaps breaking the words. Less frequently though. - There is another problem with vowel placement on a stretched letter. This also seems to require some extra manipulation of the justified glyph widths. (not implemented in the previous patch yet) - I saw, that there is a GenericSalLayout::KashidaJustify method, which also respects a minimal kashida width. Does this work? Or does this require debugging? This issue was reported on all platforms! Anyhow, the minimal kashida width used over there seems to be much smaller than the one returned by usp. - When I moved the code to correct the character widths to writer, I discovered that it's pretty useless to pad the blanks as I did in winlayout.cxx. As said before, the rounding errors force me to do it again over there anyhow, and so there is not much to gain. After looking at ScriptJustify once more, I switched to reduce the number of kashida insertion points in the line, and recalculated the space to add for each kashida. With this, the justification looks really clean now. The fixing in winlayout.cxx has not a lot to do, except for some rare cases (e.g. very short lines). But I encountered one problem: The cursor placement is affected, and I have a "chicken and the egg" problem there. Cursor placement relies on the correct number and identification of kashida insertion points. But I manipulated there, so the cursor placement is off in some cases. So there is some work to be done there, but I left that for the moment...
Thanks for looking into this! The kashida width currently used for GenericSalLayout::KashidaJustify() is just the logical width of an isolated glyph for the font's U+0640 (arabic tatweel). The idea is that it doesn't make sense to insert justification glyphs if they are too wide. Do you happen to know how the member iKashidaWidth from usp's FontProperties and this glyph width correlate? I'm sure that GenericSalLayout::KashidaJustify() could benefit from more debugging. I'd delay this task though until we are happy with the justification result on one platform (USP?) and then port it to the other platforms. GenericSalLayout is mainly used on our Unix ports. Writer does its cursor placement usually depending on the result of OutputDevice::GetTextArray() which is very related to UniscribeLayout::FillDXArray(). But when you override the resulting DXArray with your improved justification method then the cursor positioning needs to be adjusted too. FME can probably point you to the matching code in Writer. The problem with vowel placement on a stretched letter is probably caused because a glyph cluster is not properly detected. Only glyph clusters themselves should get adjusted, the positioning inside a glyph cluster is supposed to be unchanged. Hope this helps.
The iMinKashida is in most of the fonts I checked 3/5th of the width of the tatweel glyph (Arial, Times New Roman, Arabic Typesetting). In Tahoma the two values are equal. Wouldn't it make also sense to add the approach from GenericSalLayout::KashidaJustify() to UniscribeLayout:GetNextGlyphs? Currently, if I do a PDF export with Arabic justified text, we get huge gaps instead of the kashidas.
@fme: I'll attach another version of the porlay.cxx patch. As discussed before with hdu, I am searching for a way to modify writer's decision on kashida positions that respects a minimal allowed kashida width. More details in the previous comments. Can you look at the changes of the KashidaJustify method? The patch works great for text display, but it disrupts cursor movement. When called without the nSpaceAdd or nMinKashidaWidth parameters and no pKernArray is provided, it returns the number of all possible kashida postions as before. But if they are present, kashida positions are dropped and nSpaceAdd is adjusted accordingly. I could catch, when running the process of cursor placement through the debugger, that KashidaJustify is called with a nLen value just up to the present cursor position. But in my version I needed to look a the whole text portion and available space to determine, if a kashida position is valid or not. I thought it might be a good idea, to mark kashida positions as valid or invalid once, so that the correct information would available to the cursor context. But for that I needed to know a lot more on the lifetime of the SwScriptInfo object. Also important: when do I need to initialize and reset this information? Any suggestions?
Created attachment 52673 [details] patch draft for porlay.cxx
fme->hennerdrewes: Thank you for your patch. I see. If called from the text formatting, the code still returns the number of kashida positions inside the given text range., because pKernArray = 0, nSpaceAdd = 0, nMinKashidaWidth = 0. The second case - the function is called from fntcache.cxx for cursor calculation of text painting - works as follows: First (like in the first case), we calculate the number of kashidas in the text range. If we find, that the space provided for the kashidas is smaller than a nMinKashidaWidth and there is more than one kashida in the text range, we start discarding the kashida positions at the beginning of the text range, as long as the nSpaceAdd value for the remaining kashidas is < nMinKashidaWidth. Sounds reasonable. I would suggest to proceed as follows: In CalcNewBlock(), the nSpaceAdd value for each text line is stored at the SwLineLayoutPortion. After this calculation has been performed, we know the nSpaceAdd value for this line based on the assumption, that each possible kashida position can be used. Here we need to insert some new code that iterates over the text portions in the current line (SwTxtAttrIterator), checks which font will be used for the portion, get the minimum kashida width for that font. If the minimum kashida width is smaller than the calculated nSpaceAdd value stored at the SwLineLayout, we should start to mark some of the kashida positions in the SwScriptInfo structure as invalid, just as you proposed. Then the nSpaceAdd value for storing at the SwLineLayout will be calculated again until we can make sure that nSpaceAdd > the minimum kashida width for all the text portions in the current line. If we proceed this way, I think we can keep the original version of KashidaJustify(). One more note: Each time CalcNewBlock is executed, we first have to reset the invalid flags for the kashida postions located in the current line. What do you think?
@fme: sounds promising... I'll give it a try.
@fme: I had a look at the place at CalcNewBlock(), where the new code should go. But I am afraid that I don't understand enough details on all the portion and info objects involved. Would you find some time to write this iteration over the text portions? I'll attach all (hopefully) useful changes I made so far, especially the changes in vcl to enable querying the kashida width. I added to OutputDevice the following methods, the first one queries the current selected font (analogous to GetFontMetric()). long GetMinKashida() const; long GetMinKashida( const Font& rFont ) const; I added to SwScriptInfo the code necessary to mark kashida positions as invalid. KashidaJustify returned to the original format, but now skips the invalid kashida positions.
Created attachment 52754 [details] patches for files in vcl/inc
Created attachment 52755 [details] patches for files in vcl/source
Created attachment 52756 [details] patches for files in vcl/win
Created attachment 52757 [details] patch for file vcl\win\source\gdi\winlayout.cxx (on version 1.108.206.2)
Created attachment 52758 [details] patch for sw/core/source/inc/scriptinfo.hxx
Created attachment 52759 [details] patch for sw/source/core/text/porlay.cxx
fme->hennerdrewes: Thank you for you patch. I'll try to have a look at this and the text iteration issue next week.
Created attachment 53207 [details] new patch proposal
@fme: I think I finally figured it out. Will you have a look? The justification works fine now. There is one new problem, though. If I write an Arabic line, apply justification and LTR paragraph, the line is offset to the right. The offset apparently correlates to the adjustment of the kashida spacing. A text line without kashida positions marked as invalid is painted correctly. In a RTL paragraph everything is fine though. I'll post a screenshot for clarification.
Created attachment 53208 [details] Screenshot of LTR paragraph:
Created attachment 53209 [details] Screenshot of RTL paragraph: OK
fme->hennerdrewes: Thank you for your patch. I'll have a look at it as soon as possible, promised.
@fme: sorry to bother you again... I think I understand the remaining problem. BidiPortion::CalcSpacing works on the stored number of blanks / kashidas, which is set before we drop kashida positions. The nSpaceAdd parameter, on the other hand, is the updated value after kashida positions are dropped. TxtPortion::CalcSpacing recalculates its number of blanks on the fly, so the dropped kashida positions are taken into account. Any hints or suggestions?
fme->hennerdrewes: No, not yet. I'll have to dig into this somewhat deeper, but unfortunately I'm pretty busy right now. I'll have a look as soon as possible.
fme->hennerdrewes: I had a look at the problem with the bidi portions. You are right, for common text portions, CalcSpacing() as well as GetSpaceCnt() both calculate the number of blanks on-the-fly whereas the SwBidiPortion stores the number of blanks found during the formatting of the multi portion and makes use of this value later on. Thinking this over, I cannot imagine a reason why this has to be like this. I think the bidi portion can also calculate the number of blanks on demand. So the code in pormulti.cxx that starts with the comment "// Calculate number of blanks ..." can be moved to SwBidiPortion::GetSpaceCnt(). What do you think?
@fme: In the mean time I did some experimenting with adjusting the values via SwBidiPortion::SetSpaceCnt(). But if the Bidi Portion can calculate the number of blanks on the fly, it would be certainly the more elegant solution. Anyhow, after solving this I expect the patches to work just fine. Except for this case: If a font is applied, that substitutes certain character combinations with glyph ligatures (like "Arabic Typesetting"), kashidas may be inserted at illegal positions (and words will break again). To fix this, writer would have to query for the actual behavior of the current font. I think this could be done right before the MinKashidaWidth related dropping of kashida positions. So there would be a second reason to drop kashida positions. But I am not sure how to get to that font dependent information. It would certainly require new interfaces in vcl to provide that information. (Maybe this should move to a separate issue?) I will post some updated version of the patches soon.
Created attachment 54111 [details] new patch for writer
@fme: Added a new patch for writer. Tried to implement everything as discussed. SwBidiPortion::GetSpaceCnt() takes now a SwTextSizeInfo parameter, so I also had to make some changes, when this function is called in other places. If you could check that again... Anyhow, I haven't noticed any problems during my tests. I also added some lines in fntcache.cxx to prevent displaced kashidas, when the language attribute is not Arabic (issue 28203). But maybe the solution proposed with issue 28203 (checking unicode character values and not the language attribute) should be implemented now?
@hennerdrewes: Sounds good. I'll have a look.
fme->hennerdrewes: Thank you for your patch! Quite a lot of changed code ;-) Since issue 87688 already has been integrated, I guess some of the code from the patch is obsolete, is this correct? [...] I also added some lines in fntcache.cxx to prevent displaced kashidas, when the language attribute is not Arabic (issue 28203). [...] Please also remove these lines from the patch. This makes it easier to review the patches. fme->hdu: Would you like to set up a cws for this?
@fme: I tested the changes on OOH680_m12 code, and the patch files also reflect the changes to that version. So the patch for porlay.cxx definitely contains code, which already has been integrated. Just haven't had the time to transfer it to and test it with DEV300. Can you manage with the patch as it is?
> fme->hdu: Would you like to set up a cws for this? I'd love to, but I'm completely under water... Since the remaining patch is all in the SW module it would be a good idea to reassign it, ok?
I'll create a cws ('kashidafix') based on DEV300m19 and commit the patches.
fme->hennerdrewes/hdu: I found that some of the vcl_win.patch already made its way into the current version (m19). Is this correct?
@fme: Yes, that's correct. On Apr 11 I also attached a separate patch for winlayout.cxx, which should include only the recent changes related to this issue.
fme->hennerdrewes: I tried to move the patch DEV300m19 but failed with the vcl part. Please transfer the patch to DEV300m19, at least the vcl part the patch, that would help a lot.
@fme: OK
Created attachment 54451 [details] patch for vcl reworked
@fme: hope this will be OK
@hennerdrewes: I just had a very quick look at the vcl patch - please use the USP library loaded in winlayout.cxx, loading it in salgdi3 too is IMHO not needed - eleven levels of indentation... maybe they are needed but they make the already complex LayoutText even more difficult - only the Windows part of VCL is changed, I'm worried about the other ports: Do they need work too
@hdu: I am aware that this version of the patch can be seen as a draft only. There ary many things that should be cleaned up. For example: if ImplWinFontEntry was accessible in salgdi3.cxx, the ScriptCache of ImplWinFontEntry could be used in the call to ScriptGetFontProperties. Until now, I just tried to get things working, changing header files only when absolutely necessary. - For the other ports: They need work. Basically they should provide a value for the minimal kashida width as done in WinSalGraphics::GetFontMetric. If nothing is implemented though, calls to OutputDevice::GetMinKashida will return 0. The new code in writer is executed only if a positive GetMinKashida value is provided, so the patches should be safe even if not fully implemented on all platforms. I have no idea to what extent changes are still needed in the layout process on the other platforms. In UniscribeLayout::ApplyDXArray I had to add some new stuff to correct results of ScriptApplyLogicalWidth (of course, this also needs cleaning, and I might have some more suggestions for that..).
I committed the patches to cws kashidafix, based on DEV300m19. Any further patches should be relative to this cws. I did not modify the vcl patch but made some minor changes to the sw patch. fme->hennerdrewes/hdu: To judge from your last comments I guess that work on the vcl part has not yet been finished, is this correct?
Created attachment 54588 [details] patch correction
@fme: I made some corrections to your patch modifications. @fme/hdu: I think it will be necessary to clearly define the scope of this issue. Just after doing this, it will be possible to answer your questions, how much further work is needed. The patches in their current state considerably improve the kashida justifications on Windows. But here is a list of further things we should think about: 1. General cleanup of vcl patches (as pointed out by hdu) 2. Support for other platforms. 3. Support for fonts with glyph ligatures (as mentioned in my comment from May 28) 4. Fix PDF export (Windows) Additional comments for 1: I will do that... for 2: Basically, the other platforms need to provide a value for the minimal kashida width. Most fonts I checked on windows use a value of 3/5th of the width of the tatweel glyph. So maybe this could be used also on the other platforms. But as hdu pointed out on April 8: > I'm sure that GenericSalLayout::KashidaJustify() could benefit from more > debugging. I'd delay this task though until we are happy with the > justification result on one platform (USP?) and then port it to the > other platforms. So there actually might be a lot more work left for the other platforms. But please take into account that I probably won't be able to help much for this one. for 3: This is my most urgent question: Extend the current patches to support fonts, where certain character combinations are replaced by a single glyph? Actually the new kashida dropping mechanism could be used for that, too. In CalcNewBlock(), just before the checking of the minimal kashida width, we would have to ask vcl, if each of our kashida positions fall on two separate glyphs or if they are replaced by one glyph. In the latter case, we need to drop the kashida position. for 4: PDF export on Windows needs manual insertion of tatweel glyphs.
I can do 1. 2. 3. and 4. but certainly not in the timeframe for OOo3.0 and probably not for 3.1. On the other hand it looks like the current status is quite an improvement and we could try to get this into 3.0 as it is and handle the other tasks as followup tasks with a different target milestone.
fme->hdu/hennerdrewes: So let me sum this up. The current state of the patch improves the kashida handling on Windows significantly, other platforms are not affected at all. So besides from the fact that there's still some work left, is there any good reason *not* to accept the patch? So I suggest that Henner continues to work on task 1 (General cleanup of vcl patches) and after that we'll try to integrate the patch for 3.1 (3.0 target is too close I think).
@fme: yes, that's exactly the way I see it (OOo 3.1)
@hdu/fme: OK, understood. Hopefully will be posting something next week.
Created attachment 54935 [details] cleaned up patch for vcl
@hdu: posted the reworked vcl patch. I moved the declaration of ImplWinFontEntry from winlayout.cxx to salgdi.h, which helped to eliminate duplicate code. Hope this is OK.
retarget issue to 3.1
@hennerdrewes: the new patch is much better, thank you! With the new patch the USP integration does no longer sprawl into other *.cxx files, just into the central salgdi.h. I don't remember why maScriptCache became a member of ImplWinLayout instead of UniscribeLayout, so I have no right to complain... but I'd rather have USP not go into salgdi.h. If there is a little "breathing" time between 3.0 and 3.1 I'll change it accordingly, else the patch can go in as it is.
fme->hennerdrewes: Thanks again for your patch. I had a closer look at the Writer stuff. Here are some comments: 1. The invalidation of kashidas has to be done per portion. Image this case: The line starts with one huge Arabic character, the font of which reports a minimum kashida width of 4000 twips. Then the kashida positions in the line are all marked as invalid, although the portion which requires the 4000 twips does not even have a kashida position. 2. I think using a sorted array (or a more 'modern' data structure like std::set) is more suitable for the kashida array, since right now IsKashidaInvalid/ClearKashidaInvalid always have to iterate the whole array. 3. if for some mysterious reason if(rSI.MarkKashidaInvalid(aItr.GetStart(), aItr.GetLength()) fails, we have an infinite loop in itradj.cxx.
@fme: Thanks for your comments. In the mean time, I have prepared some additional changes to support fonts with glyph ligatures. I hope that I can post a patch later this week, and hopefully I can include some of your hints. Other questions / comments: I have the feeling that since I made the changes to SwBidiPortion::GetSpaceCnt(), there is a inaccuracy in the cursor placement, when the cursor is placed on the boundary between LTR and RTL runs. Are you really sure, there was no deeper intention behind the way GetSpaceCnt() was implemented? I haven't found the time yet to revert the changes and see, if this is the actual cause. But as Hebrew text is also affected, and all other changes shouldn't affect Hebrew, I'd be surprised if the cause is elsewhere.
@hennerdrewes: [...] Other questions / comments: I have the feeling that since I made the changes to SwBidiPortion::GetSpaceCnt(), there is a inaccuracy in the cursor placement, when the cursor is placed on the boundary between LTR and RTL runs. Are you really sure, there was no deeper intention behind the way GetSpaceCnt() was implemented? [...] No, of course I'm not ;-) Have a look at the code following this comment in itrcrsr.cxx: // we came from inside the bidi portion, we want to blink... There, we call SwBidiPortion::CalcSpacing() with rInf.nIdx already set to the start of the next portion. Now that the number of blanks in the bidi portion is calculated on-the-fly we have to make sure that rInf.nIdx is set to the start of the bidi portion, just like with a regular SwTxtPortion::GetSpaceCntnt call. I suggest to store the start of the bidi portion if we set pLastBidiPortion and set this value temporarily at rInf before calling CalcSpacing().
@hennerdrewes: Or better: Store with width of the bidi portion.
@hdu: I just reviewed how my last patch was integrated. And I have one question on that: In my version I managed to call ScriptGetFontProperties() with the same SCRIPT_CACHE from ImplWinFontEntry. This was the reason for moving things to salgdi.h. Are you still planning to clean this up, or should it be left as it is? As I prepared some further changes, it would help to know from where to start additional patches.
It took some more glances until I understood that the last vcl.patch hasn't been integrated. Anyhow, my last question is still relevant: how to prepare additional patches?
@hennerdrewes: the vcl.patch not being commited into CVS was an oversight; it is in now. I still intend to clean this up but cannot start it for at least the next two weeks. As for how to prepare the patch I'd suggest to do the diff against the tag CWS_DEV300_KASHIDAFIX. What would be even better is to get you commiter rights. You have proven that you know what you are doing. If you are interested in that I'd support you getting commit rights. I'd wait for the SVN migration to settle before http://wiki.services.openoffice.org/wiki/OOo_and_Subversion#SSH_Setup is done.
correction to above: the tag is CWS_DEV300_KASHIDAFIX_ANCHOR
Created attachment 56772 [details] vcl.patch (task #3)
Created attachment 56773 [details] sw.patch (task #3)
@fme, @hdu: just added new patches implementing the support for fonts with glyph ligatures. patches are relative to cws_dev300_kashidafix. The sw.patch also includes the changes of issue 91465. The new code of this issue relies on issue 91465. @fme: I considered your comments #1 and #3. I haven't had the time to work on your comment #2, although the new implementation will already save some iterations, as kashidas are not marked necessarily one by one anymore. Also fixed the cursor traveling. I'll post some more comments and questions, but I need to go over the changes once more. That might take a couple of days.
@hdu: I added the methods KashidItemFix() and KashidaWordFix() to UniscribeLayout. ApplyDxArray() looks a lot nicer, now that all the kashida related post-processing has moved. But when I look at what I did in KashidaWordFix(), I cannot believe that I had to write so many lines to fix the gaps between the letters. Although the justification values requested by writer are now much more appropriate, the values returned by ScriptApplyLogicalWidth are very often just one or two pixels below the minimal kashida width, causing the nasty gaps. The new code seems to fix that quite well, but honestly, I don't like it. But I had another idea: It should be possible to insert kashida glyphs manually. If we were a little bit more tolerant regarding the minimal kashida width than ScriptTextOut, we should be able to produce proper text output without this width fixing. Anyhow, as far as I understood this is what is happening on linux and what we have to add for PDF output on windows. So the manually inserted kashida glyphs could fix the gap problem and the PDF output. But I guess we'll have to try and check the visual results.
@fme: In itradj.cxx the new code validates the kashida positions with the current font. Only after that we'll check the resulting kashida width against the minimal kashida width of the fonts. The new code also considers the possibility that all kashidas will be canceled. This situation is very likely with some fonts, both because of glyph ligatures and huge kashida glyphs. Regular blank justification should be done in that case. As the application of kashida justification still depends on the language attribute, I had to check this attribute in additional places. But this could be removed once issue 28203 is solved. As we also have to prepare for blank justification for Arabic, I once more suggest some changes to fntcache.cxx. The half spacing applied to Arabic inevitably causes kashidas to appear between the words.
@hennerdrewes: Thank you for your patch. I noticed that there are still these annoying _ artifacts in case a text line falls back to a common "blank justification" even with the bNoHalfSpace change in fntcache.cxx. I'll attach a document that shows the problem. Please have a look.
Created attachment 56947 [details] new bugdoc
@fme: Can you upload the bugdoc once more? The file seems to be corrupted. And can you also make a screenshot? I will check the file with my last build, before the changes from cws swarab02 were introduced. I haven't built the last version with the cws swarab02 changes yet. So until then, I can at least compare my version to the screenshot.
Created attachment 56950 [details] 2nd try
Created attachment 56951 [details] screenshot
@hennerdrewes: Sorry, new bugdoc and screenshot attached.
@fme: I also see the problem. So it is not connected to the swarab02 changes. I'll dig into that a little later.
@fme: Something seems to be wrong with the expanded blanks, if the blank is the last character of the portion. The bNoHalfSpace actually doesn't matter in this case, as > fntcache.cxx line 1871 > if ( i + 1 == nCnt ) > nSpaceSum += nSpaceAdd; Strangely enough, in UniscribeLayout::ApplyDxArray, the mpCharWidths array shows the character before the blank with the expanded width, and the blank with its regular width. My crazy UniscribeLayout::KashidaItemFix() ensures that this problem hasn't been left unnoticed, (: as it attempts to expand stretched glyphs to the minimal kashida width. Without this the visual result of the problem would occur less frequently, only when the added width exceeds the minimal kashida width.
Created attachment 57087 [details] Screenshot
Actually, if I look at this in Western text (see screenshot I've just attached), it seems to be regular behaviour. When the blank is the last character of the portion, the character before the blank seems to get expanded. The "blank-dot" moves to the right. While this is not problematic with Western text, it disrupts the blank justification for Arabic.
Created attachment 57136 [details] patch fntcache.cxx
@fme: uploaded a suggested patch for fntcache.cxx Actually the problem equally affects word underline. See following screen shot.
Created attachment 57137 [details] screenshot showing the problem for Western text with word underline
Comments to screen shot BlankAtPortionEndWordUnderline.png: The "t" of "Welt" is stretched instead of the following blank.
*** Issue 94896 has been marked as a duplicate of this issue. ***
@hennerdrewes: Patch works fine, thank you. @hdu/hennerdrewes: Anything left to do in vcl? Or are we done with this issue?
@fme: I would like to go over the vcl stuff once more. Other things that come to my mind: 1. When I validate the kashida positions in itradj.cxx, I set the layout mode as following: > ULONG nOldLayout = rInf.GetOut()->GetLayoutMode(); > rInf.GetOut()->SetLayoutMode ( nOldLayout | TEXT_LAYOUT_BIDI_RTL ); > nKashidasDropped = rInf.GetOut()->ValidateKashidas ( ...); > rInf.GetOut()->SetLayoutMode ( nOldLayout ); When wrote this, I wasn't really sure what is required. I found out that I need to set the TEXT_LAYOUT_BIDI_RTL flag to get correct results for LTR paragraphs. But since this has been done more by "trial and error" and not by knowing, I'd be glad, if you give it a thought. 2. We haven't dealt with your comments on the data structures: > I think using a sorted array (or a more 'modern' data structure > like std::set) is more suitable for the kashida array, since right > now IsKashidaInvalid/ClearKashidaInvalid always have to iterate the > whole array. @hdu: Can you comment on my remarks from 25/9?
> @hdu: Can you comment on my remarks from 25/9? Inserting the kashida-glyphs into the PDF-export was already on my TODO list. If this also helps for simplifing code in Kashida*Fix it is all the better. I'm looking into it and hope to get it done in the next few days.
@hdu: great.
@hennerdrewes: I guess we can replace this code by setting SwLayoutModeModifier aLayoutModeModifier( rInf.GetOut() ); aLayoutModeModifier.Modify( sal_True ); Since at this point in the code we are sure that the current text is Arabic (which will be written from right to left), the SwLayoutModeModifer can be used to pass this to the output device. The TEXT_LAYOUT_BIDI_STRONG flag will be set (i.e., "don't check the direction, I know what I do) and the TEXT_LAYOUT_BIDI_RTL flag will be set (i.e., "the next text is rtl text"). Please check, it the code works with the change.
*** Issue 87476 has been marked as a duplicate of this issue. ***
> Inserting the kashida-glyphs into the PDF-export was already on my TODO list. Done (also see issue 71804) > If this also helps for simplifing code in Kashida*Fix it is all the better. The above was solved differently, because that could possibly break other uses of the resulting SalLayout, e.g. with ScriptTextOut() etc. @fme: from my POV the CWS code is ready...
Fixed for Windows platforms.
@sba: please verify on XP and Vista, please test for regressions on other platforms
Verified in CWS kashidafix.
*** Issue 98194 has been marked as a duplicate of this issue. ***
OK in OOO310_m11. Closed.