Re: Problem stripLeadingSpaces()
Abdelrazak Younes schrieb: Right now, stripLeadingSpaces() returns the number of _physically_ deleted spaces. I can change this to return the number of physically and logically characters. Yes, I guess we should do that. Ok, the patch will be committed in a few minutes. Michael
Re: Problem stripLeadingSpaces()
Abdelrazak Younes schrieb: OK, IIUC, then you will have two deleted space then, right? Again, IIUC, the paragraph contents should still contain the two spaces but isDeleted(0) and (1) would return true, right? Yes & Yes. If yes, then as I said, there is a problem even before the screen update problem because while debugging I noticed that there was only _one_ leading space, not two. Hmmm. I see two (striked-out) spaces after moving around the cursor. Let's see how the situation changed with your latest patch. Michael
Re: Problem stripLeadingSpaces()
Michael Gerz wrote: Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Well, I still don't understand because the screen seems fine here, see attached. No, the screen is not fine: the second space should be marked as deleted! Why is that? Before I hit "Enter", the second space was not deleted; was should it be deleted after? The semantics of stripLeadingSpaces() is, well, to strip _all_ leading spaces. Since the second space is a leading space, too, it is marked as deleted in CT mode. stripLeadingSpaces() works as intended but the screen is not updated afterwards. Right now, stripLeadingSpaces() returns the number of _physically_ deleted spaces. I can change this to return the number of physically and logically characters. Yes, I guess we should do that. This may help but may also prevent some optimizations in DEPM. I also don't understand why we return false at the end of deleteEmptyParMech() even if stripLeadingSpaces returns !0. I've change that and now the crash is gone because going up with the arrow key will trigger the DEPM and the metrics will then be updated. Still, I guess my commit only cures the symptom because the second paragraph is still wrong. At least it's now wrong in the paragraph contents also. If you want the space to be deleted as soon as the new paragraph is created, we need to call stripLeadingSpaces() on the new paragraph also, and we don't do that in DEPM. Ah, and we should clarify/document the meaning of the return value of deleteEmptyParMech... Yes. Abdel. What do you think? I am a bit lost without your assistance... Michael
Re: Problem stripLeadingSpaces()
Michael Gerz wrote: Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Well, I still don't understand because the screen seems fine here, see attached. No, the screen is not fine: the second space should be marked as deleted! Why is that? Before I hit "Enter", the second space was not deleted; was should it be deleted after? The semantics of stripLeadingSpaces() is, well, to strip _all_ leading spaces. Since the second space is a leading space, too, it is marked as deleted in CT mode. stripLeadingSpaces() works as intended but the screen is not updated afterwards. Right now, stripLeadingSpaces() returns the number of _physically_ deleted spaces. OK, IIUC, then you will have two deleted space then, right? Again, IIUC, the paragraph contents should still contain the two spaces but isDeleted(0) and (1) would return true, right? If yes, then as I said, there is a problem even before the screen update problem because while debugging I noticed that there was only _one_ leading space, not two. I can change this to return the number of physically and logically characters. This may help but may also prevent some optimizations in DEPM. I think that DEPM is indeed the culprit here and we have to teach it to disregard deleted spaces (and deleted paragraphs for the matter). I also don't understand why we return false at the end of deleteEmptyParMech() even if stripLeadingSpaces returns !0. Ah, and we should clarify/document the meaning of the return value of deleteEmptyParMech... What do you think? I am a bit lost without your assistance... I have to study the code before answering you. Maybe what I said above will help you. Abdel.
Re: Problem stripLeadingSpaces()
Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Well, I still don't understand because the screen seems fine here, see attached. No, the screen is not fine: the second space should be marked as deleted! Why is that? Before I hit "Enter", the second space was not deleted; was should it be deleted after? The semantics of stripLeadingSpaces() is, well, to strip _all_ leading spaces. Since the second space is a leading space, too, it is marked as deleted in CT mode. stripLeadingSpaces() works as intended but the screen is not updated afterwards. Right now, stripLeadingSpaces() returns the number of _physically_ deleted spaces. I can change this to return the number of physically and logically characters. This may help but may also prevent some optimizations in DEPM. I also don't understand why we return false at the end of deleteEmptyParMech() even if stripLeadingSpaces returns !0. Ah, and we should clarify/document the meaning of the return value of deleteEmptyParMech... What do you think? I am a bit lost without your assistance... Michael
Re: Problem stripLeadingSpaces()
Michael Gerz wrote: Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Well, I still don't understand because the screen seems fine here, see attached. No, the screen is not fine: the second space should be marked as deleted! Why is that? Before I hit "Enter", the second space was not deleted; was should it be deleted after? I am missing something, obviously.
Re: Problem stripLeadingSpaces()
Abdelrazak Younes schrieb: Yes, this is exactly it. The problem is that the first (and unique) row of the first ParagraphMetrics has one more character than what is in the Paragraph. The Paragraph content does not seem to reflect the deleted characters. In the following loop, the position of the last char or the row does not correspond to the position of the last char in the paragraph. for (pos_type p = first; p < last; ++p) { if (par.isHfill(p)) ++n; } Your comment clearly indicates that there is a missing metrics/screen update somewhere. As said before, when adding the par break to my example file, both leading spaces should be marked as deleted. You can easily validate this by pressing return at the beginning of the first line. Magically, the two spaces are marked red in the (now) third line. Michael PS: I committed the innocent patch to stripLeadingSpaces(). The screen update has to be handled somewhere outside this method.
Re: Problem stripLeadingSpaces()
Abdelrazak Younes schrieb: Michael Gerz wrote: Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Well, I still don't understand because the screen seems fine here, see attached. No, the screen is not fine: the second space should be marked as deleted! Michael
Re: Problem stripLeadingSpaces()
Michael Gerz wrote: Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Well, I still don't understand because the screen seems fine here, see attached. Abdel.
Re: Problem stripLeadingSpaces()
Abdelrazak Younes wrote: Michael Gerz wrote: I think that you have to distinguish the document model from what is on screen. On screen, the row representation does not care if one char is a or a , seven chars are seven chars. I guess this is the problem and you have to skip deleted char in numberOfHfills() or something like that. Yes, this is exactly it. The problem is that the first (and unique) row of the first ParagraphMetrics has one more character than what is in the Paragraph. The Paragraph content does not seem to reflect the deleted characters. In the following loop, the position of the last char or the row does not correspond to the position of the last char in the paragraph. for (pos_type p = first; p < last; ++p) { if (par.isHfill(p)) ++n; } Abdel.
Re: Problem stripLeadingSpaces()
Abdelrazak Younes schrieb: I don't understand, why the space in the first line should be deleted? means a space that was marked as deleted. Use the attached lyx file. Press return after "hello". The leading spaces are marked as deleted but the screen is not updated. Michael strip.lyx Description: application/lyx
Re: Problem stripLeadingSpaces()
Michael Gerz wrote: Hi, I rethought the logic of stripLeadingSpaces() and came to the conclusion that it is not a good idea to remove the spaces in change tracking mode: imagine that your co-author inserts a par break that you don't like. If you revert his par break (revert = reject change, not undo!), you would like to see the spaces restored. I changed stripLeadingSpaces accordingly (see attached patch) but unfortunately I ran into two problems that look like painting/update problems. By switching CT on and off, create a one-line document that looks like this: helloworld Now, in CT mode, press return after "hello". The expected output is: hello world I don't understand, why the space in the first line should be deleted? Two does not sound right anyway. However, this is what I get on screen: hello world I don't understand exactly what I am supposed to do get this behaviour. When I hit return I have the same thing on the second line than what was in the first line after "hello". In other words, the deletion of the second space is not shown on screen. Maybe dEPM is triggered? You probably have to modify this method to take into account TrackChange. Even worse, if I move the cursor up (to first line) and down (back to second line), LyX crashes! I manage to reproduce that. Do you remember the Peter's fix in numberOfHfills() that got reverted at the end. This is exactly what happens here. I can't imagine that my fix is responsible for this crash. Abdel, do you have any idea of what goes wrong? I think that you have to distinguish the document model from what is on screen. On screen, the row representation does not care if one char is a or a , seven chars are seven chars. I guess this is the problem and you have to skip deleted char in numberOfHfills() or something like that. Abdel.
Problem stripLeadingSpaces()
Hi, I rethought the logic of stripLeadingSpaces() and came to the conclusion that it is not a good idea to remove the spaces in change tracking mode: imagine that your co-author inserts a par break that you don't like. If you revert his par break (revert = reject change, not undo!), you would like to see the spaces restored. I changed stripLeadingSpaces accordingly (see attached patch) but unfortunately I ran into two problems that look like painting/update problems. By switching CT on and off, create a one-line document that looks like this: helloworld Now, in CT mode, press return after "hello". The expected output is: hello world However, this is what I get on screen: hello world In other words, the deletion of the second space is not shown on screen. Even worse, if I move the cursor up (to first line) and down (back to second line), LyX crashes! I can't imagine that my fix is responsible for this crash. Abdel, do you have any idea of what goes wrong? Michael Index: src/paragraph.h === --- src/paragraph.h (Revision 16671) +++ src/paragraph.h (Arbeitskopie) @@ -346,7 +346,7 @@ int getPositionOfInset(InsetBase const * inset) const; /// Returns the number of line breaks and white-space stripped at the start - int stripLeadingSpaces(); + int stripLeadingSpaces(bool trackChanges); /// return true if we allow multiple spaces bool isFreeSpacing() const; Index: src/text2.C === --- src/text2.C (Revision 16671) +++ src/text2.C (Arbeitskopie) @@ -1231,7 +1231,7 @@ return true; } - if (oldpar.stripLeadingSpaces()) + if (oldpar.stripLeadingSpaces(cur.buffer().params().trackChanges)) need_anchor_change = true; return false; Index: src/CutAndPaste.C === --- src/CutAndPaste.C (Revision 16671) +++ src/CutAndPaste.C (Arbeitskopie) @@ -270,7 +270,7 @@ pars[last_paste].makeSameLayout(pars[last_paste + 1]); mergeParagraph(buffer.params(), pars, last_paste); } else { - pars[last_paste + 1].stripLeadingSpaces(); + pars[last_paste + 1].stripLeadingSpaces(buffer.params().trackChanges); ++last_paste; } } @@ -309,7 +309,7 @@ (pit + 1 != endpit || pars[pit].hasSameLayout(pars[pit + 1]))) { pos_type const thissize = pars[pit].size(); if (doclear) - pars[pit + 1].stripLeadingSpaces(); + pars[pit + 1].stripLeadingSpaces(params.trackChanges); mergeParagraph(params, pars, pit); --endpit; if (pit == endpit) @@ -539,7 +539,7 @@ // sometimes necessary if (doclear) - text->paragraphs()[begpit].stripLeadingSpaces(); + text->paragraphs()[begpit].stripLeadingSpaces(bp.trackChanges); // cutSelection can invalidate the cursor so we need to set // it anew. (Lgb) Index: src/paragraph.C === --- src/paragraph.C (Revision 16671) +++ src/paragraph.C (Arbeitskopie) @@ -560,19 +560,22 @@ } -int Paragraph::stripLeadingSpaces() +int Paragraph::stripLeadingSpaces(bool trackChanges) { if (isFreeSpacing()) return 0; - int i = 0; - while (!empty() && (isNewline(0) || isLineSeparator(0)) - && !isDeleted(0)) { - eraseChar(0, false); // no change tracking here - ++i; + int pos = 0; + int count = 0; + + while (pos < size() && (isNewline(pos) || isLineSeparator(pos))) { + if (eraseChar(pos, trackChanges)) + ++count; + else + ++pos; } - return i; + return count; }