Re: [patch] Re: RTL justification bug
Dov == Dov Feldstern [EMAIL PROTECTED] writes: Dov I still haven't gotten any other responses regarding this patch. Dov I think it's safe, I've been working with it since I first sent Dov it to the list and haven't run into any trouble. I don't think Dov that it's *crucial* that this go in before 1.5.0, but it would be Dov nice... I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say go ahead!, but I am not sure what are my credentials to do so :) JMarc
Re: [patch] Re: RTL justification bug
Jean-Marc Lasgouttes wrote: Dov == Dov Feldstern [EMAIL PROTECTED] writes: Dov I still haven't gotten any other responses regarding this patch. Dov I think it's safe, I've been working with it since I first sent Dov it to the list and haven't run into any trouble. I don't think Dov that it's *crucial* that this go in before 1.5.0, but it would be Dov nice... I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say go ahead!, but I am not sure what are my credentials to do so :) Same position for me. The patch looks not so complicate so I'd say go ahead too. As this is a display thing, we'll notice soon enough if something is wrong. Abdel.
Re: [patch] Re: RTL justification bug
On Monday 09 July 2007 09:50:27 Abdelrazak Younes wrote: I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say go ahead!, but I am not sure what are my credentials to do so :) Same position for me. The patch looks not so complicate so I'd say go ahead too. As this is a display thing, we'll notice soon enough if something is wrong. Abdel. I agree with Jean-Marc and Abdel so you have my OK. -- José Abílio
Re: [patch] Re: RTL justification bug
José Matos wrote: On Monday 09 July 2007 09:50:27 Abdelrazak Younes wrote: I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say go ahead!, but I am not sure what are my credentials to do so :) Same position for me. The patch looks not so complicate so I'd say go ahead too. As this is a display thing, we'll notice soon enough if something is wrong. Abdel. I agree with Jean-Marc and Abdel so you have my OK. Thanks, all! Committed as http://www.lyx.org/trac/changeset/19016. I closed bug 3889, and opened a new one, http://bugzilla.lyx.org/show_bug.cgi?id=3997, reporting a minor problem which is exposed by this fix (as explained when the patch was first submitted). Dov
Re: [patch] Re: RTL justification bug
> "Dov" == Dov Feldstern <[EMAIL PROTECTED]> writes: Dov> I still haven't gotten any other responses regarding this patch. Dov> I think it's safe, I've been working with it since I first sent Dov> it to the list and haven't run into any trouble. I don't think Dov> that it's *crucial* that this go in before 1.5.0, but it would be Dov> nice... I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say "go ahead!", but I am not sure what are my credentials to do so :) JMarc
Re: [patch] Re: RTL justification bug
Jean-Marc Lasgouttes wrote: "Dov" == Dov Feldstern <[EMAIL PROTECTED]> writes: Dov> I still haven't gotten any other responses regarding this patch. Dov> I think it's safe, I've been working with it since I first sent Dov> it to the list and haven't run into any trouble. I don't think Dov> that it's *crucial* that this go in before 1.5.0, but it would be Dov> nice... I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say "go ahead!", but I am not sure what are my credentials to do so :) Same position for me. The patch looks not so complicate so I'd say go ahead too. As this is a display thing, we'll notice soon enough if something is wrong. Abdel.
Re: [patch] Re: RTL justification bug
On Monday 09 July 2007 09:50:27 Abdelrazak Younes wrote: > > I guess the problem is that nobody understands this bidi stuff like > > you do. I would tend to say "go ahead!", but I am not sure what are my > > credentials to do so :) > > Same position for me. The patch looks not so complicate so I'd say go > ahead too. As this is a display thing, we'll notice soon enough if > something is wrong. > > Abdel. I agree with Jean-Marc and Abdel so you have my OK. -- José Abílio
Re: [patch] Re: RTL justification bug
José Matos wrote: On Monday 09 July 2007 09:50:27 Abdelrazak Younes wrote: I guess the problem is that nobody understands this bidi stuff like you do. I would tend to say "go ahead!", but I am not sure what are my credentials to do so :) Same position for me. The patch looks not so complicate so I'd say go ahead too. As this is a display thing, we'll notice soon enough if something is wrong. Abdel. I agree with Jean-Marc and Abdel so you have my OK. Thanks, all! Committed as http://www.lyx.org/trac/changeset/19016. I closed bug 3889, and opened a new one, http://bugzilla.lyx.org/show_bug.cgi?id=3997, reporting a minor problem which is exposed by this fix (as explained when the patch was first submitted). Dov
Re: [patch] Re: RTL justification bug
Dov Feldstern wrote: José Matos wrote: On Thursday 28 June 2007 21:00:28 Dov Feldstern wrote: I haven't gotten any further comments regarding this patch. Is it OK to go in? Are you sure it is safe? Could have the opinion of other RTL users? Would anyone like the patch attached at http://permalink.gmane.org/gmane.editors.lyx.devel/88617? It fixes http://bugzilla.lyx.org/show_bug.cgi?id=3889 (and some other problems, actually, see the related thread for details). It would be good if non-RTL users test this as well, just to make sure it doesn't break anything... But it's working for me, I think it's safe... Thanks! Dov Hi! I still haven't gotten any other responses regarding this patch. I think it's safe, I've been working with it since I first sent it to the list and haven't run into any trouble. I don't think that it's *crucial* that this go in before 1.5.0, but it would be nice... So --- does anyone object to my committing this, so that it gets as much testing as possible over the next few days? I know that the patch is doing the right thing in terms of what it's meant to do, the only thing is to make sure that it's not doing anything it's not meant to do, as well... ;) . So it should get testing both by RTL and non-RTL users (no active testing necessary, just apply and make sure nothing's wrong). Thanks! Dov
Re: [patch] Re: RTL justification bug
Dov Feldstern wrote: José Matos wrote: On Thursday 28 June 2007 21:00:28 Dov Feldstern wrote: I haven't gotten any further comments regarding this patch. Is it OK to go in? Are you sure it is safe? Could have the opinion of other RTL users? Would anyone like the patch attached at http://permalink.gmane.org/gmane.editors.lyx.devel/88617? It fixes http://bugzilla.lyx.org/show_bug.cgi?id=3889 (and some other problems, actually, see the related thread for details). It would be good if non-RTL users test this as well, just to make sure it doesn't break anything... But it's working for me, I think it's safe... Thanks! Dov Hi! I still haven't gotten any other responses regarding this patch. I think it's safe, I've been working with it since I first sent it to the list and haven't run into any trouble. I don't think that it's *crucial* that this go in before 1.5.0, but it would be nice... So --- does anyone object to my committing this, so that it gets as much testing as possible over the next few days? I know that the patch is doing the right thing in terms of what it's meant to do, the only thing is to make sure that it's not doing anything it's not meant to do, as well... ;) . So it should get testing both by RTL and non-RTL users (no active testing necessary, just apply and make sure nothing's wrong). Thanks! Dov
Re: [patch] Re: RTL justification bug
Dov Feldstern wrote: Stefan Schimanski wrote: Ok, after rereading your previous mail I got it. Would be good to put a better documentation there like: // Spaces at line breaks in bidi text can appear visually in the middle // of a row and must be skipped during painting: // * logically abc_[HEBREW_\nHEBREW] // * visually abc_[_WERBEH\nWERBEH] Haven't tested the patch, but it looks good. Stefan Thanks, Stefan. Attached the same patch, but better-commented... Any other opinions? Dov I haven't gotten any further comments regarding this patch. Is it OK to go in?
cusorsX speedup? (was: Re: [patch] Re: RTL justification bug)
Dov Feldstern wrote: Attached find a patch for this bug. OK? Twice almost the same code sequence. Any chance of factoring this out? I know... But if you'll take a look at the two functions (paintText and cursorX), the loop at the heart of them is very similar, and a lot of the same things are happening in both functions, except they're written in different forms. But it is actually very very similar. I think that to correctly factor this out would mean rewriting the loop for both these functions, which is not something for now... I've been thinking about this some more, and I think I have a direction for solving this: During painting, we should cache the cursorX positions. Then we won't have to repeat the same code again inside cursorX, that will solve the repeated code issue. It will also provide some speedup to the cursorX function, which currently goes through all of the characters on the line in order to calculate the position, on every single cursor blink! This would require some minor changes to the painting code --- for example, the painting code may not currently calculate X positions for every character, because we try to paint whole words at a time. But this should be easy to solve. And it would still be less expensive, because the painting code is called less frequently than the cursorX code; and anyhow, I'm pretty sure that every time painting occurs (at least in the cursor's line), cursorX will be called anyhow. We would also have to decide where the correct place for this cache is (TextMetrics?); should caching occur for the entire paragraph (or even document), or just for the line which the cursor is in? But basically, I think this would solve the repeated code, as well as provide an efficiency boost. Comments are welcome! Dov
Re: [patch] Re: RTL justification bug
On Thursday 28 June 2007 21:00:28 Dov Feldstern wrote: I haven't gotten any further comments regarding this patch. Is it OK to go in? Are you sure it is safe? Could have the opinion of other RTL users? -- José Abílio
Re: [patch] Re: RTL justification bug
José Matos wrote: On Thursday 28 June 2007 21:00:28 Dov Feldstern wrote: I haven't gotten any further comments regarding this patch. Is it OK to go in? Are you sure it is safe? Could have the opinion of other RTL users? Would anyone like the patch attached at http://permalink.gmane.org/gmane.editors.lyx.devel/88617? It fixes http://bugzilla.lyx.org/show_bug.cgi?id=3889 (and some other problems, actually, see the related thread for details). It would be good if non-RTL users test this as well, just to make sure it doesn't break anything... But it's working for me, I think it's safe... Thanks! Dov
Re: [patch] Re: RTL justification bug
Dov Feldstern wrote: Stefan Schimanski wrote: Ok, after rereading your previous mail I got it. Would be good to put a better documentation there like: // Spaces at line breaks in bidi text can appear visually in the middle // of a row and must be skipped during painting: // * logically "abc_[HEBREW_\nHEBREW]" // * visually "abc_[_WERBEH\nWERBEH]" Haven't tested the patch, but it looks good. Stefan Thanks, Stefan. Attached the same patch, but better-commented... Any other opinions? Dov I haven't gotten any further comments regarding this patch. Is it OK to go in?
cusorsX speedup? (was: Re: [patch] Re: RTL justification bug)
Dov Feldstern wrote: Attached find a patch for this bug. OK? Twice almost the same code sequence. Any chance of factoring this out? I know... But if you'll take a look at the two functions (paintText and cursorX), the loop at the heart of them is very similar, and a lot of the same things are happening in both functions, except they're written in different forms. But it is actually very very similar. I think that to correctly factor this out would mean rewriting the loop for both these functions, which is not something for now... I've been thinking about this some more, and I think I have a direction for solving this: During painting, we should cache the cursorX positions. Then we won't have to repeat the same code again inside cursorX, that will solve the repeated code issue. It will also provide some speedup to the cursorX function, which currently goes through all of the characters on the line in order to calculate the position, on every single cursor blink! This would require some minor changes to the painting code --- for example, the painting code may not currently calculate X positions for every character, because we try to paint whole words at a time. But this should be easy to solve. And it would still be less expensive, because the painting code is called less frequently than the cursorX code; and anyhow, I'm pretty sure that every time painting occurs (at least in the cursor's line), cursorX will be called anyhow. We would also have to decide where the correct place for this cache is (TextMetrics?); should caching occur for the entire paragraph (or even document), or just for the line which the cursor is in? But basically, I think this would solve the repeated code, as well as provide an efficiency boost. Comments are welcome! Dov
Re: [patch] Re: RTL justification bug
On Thursday 28 June 2007 21:00:28 Dov Feldstern wrote: > I haven't gotten any further comments regarding this patch. Is it OK to > go in? Are you sure it is safe? Could have the opinion of other RTL users? -- José Abílio
Re: [patch] Re: RTL justification bug
José Matos wrote: On Thursday 28 June 2007 21:00:28 Dov Feldstern wrote: I haven't gotten any further comments regarding this patch. Is it OK to go in? Are you sure it is safe? Could have the opinion of other RTL users? Would anyone like the patch attached at http://permalink.gmane.org/gmane.editors.lyx.devel/88617? It fixes http://bugzilla.lyx.org/show_bug.cgi?id=3889 (and some other problems, actually, see the related thread for details). It would be good if non-RTL users test this as well, just to make sure it doesn't break anything... But it's working for me, I think it's safe... Thanks! Dov
Re: [patch] Re: RTL justification bug
Stefan Schimanski wrote: +// If the last logical character is a separator, skip it, unless +// it's in the last row of a paragraph +if (end 0 end par.size() par.isSeparator(end - 1)) +skipped_sep_vpos = bidi.log2vis(end - 1); I thought it's about the visually last (or first for rtl paragraphs) character. Take HEBREW[ english] rendered as [ english]WERBEH No? I also thought so, but then I realized that it's more subtle. The real problem is with the last *logical* position, which is usually, but not always, at one of the ends. See the attached document for an example. (This is the other manifestation of the bug that I mentioned last night). Try it with and without the patch... +// If the last logical character is a separator, don't paint it, unless +// it's in the last row of a paragraph +if (end 0 end par_.size() par_.isSeparator(end - 1)) +skipped_sep_vpos = bidi_.log2vis(end - 1); + Same here. Same here. ;) Stefan Dov bug3889b.lyx Description: application/lyx
Re: [patch] Re: RTL justification bug
Martin Vermeer wrote: On Mon, Jun 25, 2007 at 11:17:15PM +0300, Dov Feldstern wrote: Dov Feldstern wrote: Hi! Here's another RTL regression --- I think this should be really easy to solve for anyone familiar with the painting code. I'd be happy to try and help out if necessary. http://bugzilla.lyx.org/show_bug.cgi?id=3889 Thanks! Dov Hi! Attached find a patch for this bug. OK? Twice almost the same code sequence. Any chance of factoring this out? I know... But if you'll take a look at the two functions (paintText and cursorX), the loop at the heart of them is very similar, and a lot of the same things are happening in both functions, except they're written in different forms. But it is actually very very similar. I think that to correctly factor this out would mean rewriting the loop for both these functions, which is not something for now... - Martin
Re: [patch] Re: RTL justification bug
Am 26.06.2007 um 21:25 schrieb Dov Feldstern: Stefan Schimanski wrote: +// If the last logical character is a separator, skip it, unless +// it's in the last row of a paragraph +if (end 0 end par.size() par.isSeparator(end - 1)) +skipped_sep_vpos = bidi.log2vis(end - 1); I thought it's about the visually last (or first for rtl paragraphs) character. Take HEBREW[ english] rendered as [ english]WERBEH No? I also thought so, but then I realized that it's more subtle. The real problem is with the last *logical* position, which is usually, but not always, at one of the ends. See the attached document for an example. (This is the other manifestation of the bug that I mentioned last night). Try it with and without the patch... Ok, after rereading your previous mail I got it. Would be good to put a better documentation there like: // Spaces at line breaks in bidi text can appear visually in the middle // of a row and must be skipped during painting: // * logically abc_[HEBREW_\nHEBREW] // * visually abc_[_WERBEH\nWERBEH] Haven't tested the patch, but it looks good. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] Re: RTL justification bug
Stefan Schimanski wrote:
Ok, after rereading your previous mail I got it. Would be good to put a
better documentation there like:
// Spaces at line breaks in bidi text can appear visually in the middle
// of a row and must be skipped during painting:
// * logically abc_[HEBREW_\nHEBREW]
// * visually abc_[_WERBEH\nWERBEH]
Haven't tested the patch, but it looks good.
Stefan
Thanks, Stefan. Attached the same patch, but better-commented...
Any other opinions?
Dov
Index: src/Text.cpp
===
--- src/Text.cpp(revision 18899)
+++ src/Text.cpp(working copy)
@@ -1718,6 +1718,12 @@
pos_type const row_pos = row.pos();
pos_type const end = row.endpos();
+ // Spaces at logical line breaks in bidi text must be skipped during
+ // cursor positioning. However, they may appear visually in the middle
+ // of a row; they must be skipped, wherever they are...
+ // * logically abc_[HEBREW_\nHEBREW]
+ // * visually abc_[_WERBEH\nWERBEH]
+ pos_type skipped_sep_vpos = -1;
if (end = row_pos)
cursor_vpos = row_pos;
@@ -1743,7 +1749,15 @@
FontMetrics const labelfm = theFontMetrics(
getLabelFont(buffer, par));
+ // If the last logical character is a separator, skip it, unless
+ // it's in the last row of a paragraph; see skipped_sep_vpos declaration
+ if (end 0 end par.size() par.isSeparator(end - 1))
+ skipped_sep_vpos = bidi.log2vis(end - 1);
+
for (pos_type vpos = row_pos; vpos cursor_vpos; ++vpos) {
+ // Skip the separator which is at the logical end of the row
+ if (vpos == skipped_sep_vpos)
+ continue;
pos_type pos = bidi.vis2log(vpos);
if (body_pos 0 pos == body_pos - 1) {
// FIXME UNICODE
Index: src/rowpainter.cpp
===
--- src/rowpainter.cpp (revision 18899)
+++ src/rowpainter.cpp (working copy)
@@ -734,6 +734,12 @@
void RowPainter::paintText()
{
pos_type const end = row_.endpos();
+ // Spaces at logical line breaks in bidi text must be skipped during
+ // painting. However, they may appear visually in the middle
+ // of a row; they must be skipped, wherever they are...
+ // * logically abc_[HEBREW_\nHEBREW]
+ // * visually abc_[_WERBEH\nWERBEH]
+ pos_type skipped_sep_vpos = -1;
pos_type body_pos = par_.beginOfBody();
if (body_pos 0
(body_pos end || !par_.isLineSeparator(body_pos - 1))) {
@@ -751,10 +757,21 @@
Font font;
Buffer const buffer = *bv_.buffer();
+ // If the last logical character is a separator, don't paint it, unless
+ // it's in the last row of a paragraph; see skipped_sep_vpos declaration
+ if (end 0 end par_.size() par_.isSeparator(end - 1))
+ skipped_sep_vpos = bidi_.log2vis(end - 1);
+
for (pos_type vpos = row_.pos(); vpos end; ) {
if (x_ bv_.workWidth())
break;
+ // Skip the separator at the logical end of the row
+ if (vpos == skipped_sep_vpos) {
+ ++vpos;
+ continue;
+ }
+
pos_type const pos = bidi_.vis2log(vpos);
if (pos = par_.size()) {
Re: [patch] Re: RTL justification bug
Stefan Schimanski wrote: +// If the last logical character is a separator, skip it, unless +// it's in the last row of a paragraph +if (end > 0 && end < par.size() && par.isSeparator(end - 1)) +skipped_sep_vpos = bidi.log2vis(end - 1); I thought it's about the visually last (or first for rtl paragraphs) character. Take HEBREW[ english] rendered as [ english]WERBEH No? I also thought so, but then I realized that it's more subtle. The real problem is with the last *logical* position, which is usually, but not always, at one of the ends. See the attached document for an example. (This is the other manifestation of the bug that I mentioned last night). Try it with and without the patch... +// If the last logical character is a separator, don't paint it, unless +// it's in the last row of a paragraph +if (end > 0 && end < par_.size() && par_.isSeparator(end - 1)) +skipped_sep_vpos = bidi_.log2vis(end - 1); + Same here. Same here. ;) Stefan Dov bug3889b.lyx Description: application/lyx
Re: [patch] Re: RTL justification bug
Martin Vermeer wrote: On Mon, Jun 25, 2007 at 11:17:15PM +0300, Dov Feldstern wrote: Dov Feldstern wrote: Hi! Here's another RTL regression --- I think this should be really easy to solve for anyone familiar with the painting code. I'd be happy to try and help out if necessary. http://bugzilla.lyx.org/show_bug.cgi?id=3889 Thanks! Dov Hi! Attached find a patch for this bug. OK? Twice almost the same code sequence. Any chance of factoring this out? I know... But if you'll take a look at the two functions (paintText and cursorX), the loop at the heart of them is very similar, and a lot of the same things are happening in both functions, except they're written in different forms. But it is actually very very similar. I think that to correctly factor this out would mean rewriting the loop for both these functions, which is not something for now... - Martin
Re: [patch] Re: RTL justification bug
Am 26.06.2007 um 21:25 schrieb Dov Feldstern: Stefan Schimanski wrote: +// If the last logical character is a separator, skip it, unless +// it's in the last row of a paragraph +if (end > 0 && end < par.size() && par.isSeparator(end - 1)) +skipped_sep_vpos = bidi.log2vis(end - 1); I thought it's about the visually last (or first for rtl paragraphs) character. Take HEBREW[ english] rendered as [ english]WERBEH No? I also thought so, but then I realized that it's more subtle. The real problem is with the last *logical* position, which is usually, but not always, at one of the ends. See the attached document for an example. (This is the other manifestation of the bug that I mentioned last night). Try it with and without the patch... Ok, after rereading your previous mail I got it. Would be good to put a better documentation there like: // Spaces at line breaks in bidi text can appear visually in the middle // of a row and must be skipped during painting: // * logically "abc_[HEBREW_\nHEBREW]" // * visually "abc_[_WERBEH\nWERBEH]" Haven't tested the patch, but it looks good. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] Re: RTL justification bug
Stefan Schimanski wrote:
Ok, after rereading your previous mail I got it. Would be good to put a
better documentation there like:
// Spaces at line breaks in bidi text can appear visually in the middle
// of a row and must be skipped during painting:
// * logically "abc_[HEBREW_\nHEBREW]"
// * visually "abc_[_WERBEH\nWERBEH]"
Haven't tested the patch, but it looks good.
Stefan
Thanks, Stefan. Attached the same patch, but better-commented...
Any other opinions?
Dov
Index: src/Text.cpp
===
--- src/Text.cpp(revision 18899)
+++ src/Text.cpp(working copy)
@@ -1718,6 +1718,12 @@
pos_type const row_pos = row.pos();
pos_type const end = row.endpos();
+ // Spaces at logical line breaks in bidi text must be skipped during
+ // cursor positioning. However, they may appear visually in the middle
+ // of a row; they must be skipped, wherever they are...
+ // * logically "abc_[HEBREW_\nHEBREW]"
+ // * visually "abc_[_WERBEH\nWERBEH]"
+ pos_type skipped_sep_vpos = -1;
if (end <= row_pos)
cursor_vpos = row_pos;
@@ -1743,7 +1749,15 @@
FontMetrics const & labelfm = theFontMetrics(
getLabelFont(buffer, par));
+ // If the last logical character is a separator, skip it, unless
+ // it's in the last row of a paragraph; see skipped_sep_vpos declaration
+ if (end > 0 && end < par.size() && par.isSeparator(end - 1))
+ skipped_sep_vpos = bidi.log2vis(end - 1);
+
for (pos_type vpos = row_pos; vpos < cursor_vpos; ++vpos) {
+ // Skip the separator which is at the logical end of the row
+ if (vpos == skipped_sep_vpos)
+ continue;
pos_type pos = bidi.vis2log(vpos);
if (body_pos > 0 && pos == body_pos - 1) {
// FIXME UNICODE
Index: src/rowpainter.cpp
===
--- src/rowpainter.cpp (revision 18899)
+++ src/rowpainter.cpp (working copy)
@@ -734,6 +734,12 @@
void RowPainter::paintText()
{
pos_type const end = row_.endpos();
+ // Spaces at logical line breaks in bidi text must be skipped during
+ // painting. However, they may appear visually in the middle
+ // of a row; they must be skipped, wherever they are...
+ // * logically "abc_[HEBREW_\nHEBREW]"
+ // * visually "abc_[_WERBEH\nWERBEH]"
+ pos_type skipped_sep_vpos = -1;
pos_type body_pos = par_.beginOfBody();
if (body_pos > 0 &&
(body_pos > end || !par_.isLineSeparator(body_pos - 1))) {
@@ -751,10 +757,21 @@
Font font;
Buffer const & buffer = *bv_.buffer();
+ // If the last logical character is a separator, don't paint it, unless
+ // it's in the last row of a paragraph; see skipped_sep_vpos declaration
+ if (end > 0 && end < par_.size() && par_.isSeparator(end - 1))
+ skipped_sep_vpos = bidi_.log2vis(end - 1);
+
for (pos_type vpos = row_.pos(); vpos < end; ) {
if (x_ > bv_.workWidth())
break;
+ // Skip the separator at the logical end of the row
+ if (vpos == skipped_sep_vpos) {
+ ++vpos;
+ continue;
+ }
+
pos_type const pos = bidi_.vis2log(vpos);
if (pos >= par_.size()) {
Re: [patch] Re: RTL justification bug
+ // If the last logical character is a separator, skip it, unless + // it's in the last row of a paragraph + if (end 0 end par.size() par.isSeparator(end - 1)) + skipped_sep_vpos = bidi.log2vis(end - 1); I thought it's about the visually last (or first for rtl paragraphs) character. Take HEBREW[ english] rendered as [ english]WERBEH No? + // If the last logical character is a separator, don't paint it, unless + // it's in the last row of a paragraph + if (end 0 end par_.size() par_.isSeparator(end - 1)) + skipped_sep_vpos = bidi_.log2vis(end - 1); + Same here. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] Re: RTL justification bug
On Mon, Jun 25, 2007 at 11:17:15PM +0300, Dov Feldstern wrote: Dov Feldstern wrote: Hi! Here's another RTL regression --- I think this should be really easy to solve for anyone familiar with the painting code. I'd be happy to try and help out if necessary. http://bugzilla.lyx.org/show_bug.cgi?id=3889 Thanks! Dov Hi! Attached find a patch for this bug. OK? Twice almost the same code sequence. Any chance of factoring this out? - Martin
Re: [patch] Re: RTL justification bug
+ // If the last logical character is a separator, skip it, unless + // it's in the last row of a paragraph + if (end > 0 && end < par.size() && par.isSeparator(end - 1)) + skipped_sep_vpos = bidi.log2vis(end - 1); I thought it's about the visually last (or first for rtl paragraphs) character. Take HEBREW[ english] rendered as [ english]WERBEH No? + // If the last logical character is a separator, don't paint it, unless + // it's in the last row of a paragraph + if (end > 0 && end < par_.size() && par_.isSeparator(end - 1)) + skipped_sep_vpos = bidi_.log2vis(end - 1); + Same here. Stefan PGP.sig Description: Signierter Teil der Nachricht
Re: [patch] Re: RTL justification bug
On Mon, Jun 25, 2007 at 11:17:15PM +0300, Dov Feldstern wrote: > Dov Feldstern wrote: > >Hi! > > > >Here's another RTL regression --- I think this should be really easy to > >solve for anyone familiar with the painting code. I'd be happy to try > >and help out if necessary. > > > >http://bugzilla.lyx.org/show_bug.cgi?id=3889 > > > >Thanks! > >Dov > > > > > > Hi! > > Attached find a patch for this bug. OK? Twice almost the same code sequence. Any chance of factoring this out? - Martin
