#583 TCoolEdit renders multiline syntax elements incorrectly

[Vidar Hasfjord]

While the user is editing the text, the text may not be rendered with the proper syntax highlighting, if it contains syntax elements that span multiple lines, such as extended comments and lines broken by line continuation (i.e. ending with \).

The likely reasons are stale cookies and lack of refresh across all the text affected.

To reproduce, open a C++ test file, type in the following text, then delete the first slash. The syntax highlighting of the text is not properly updated until you move the cursor to the next line.

just /* do
this */ right

Empty lines is a particular problem. Using the example above, put a blank line between the two lines. Now delete the first slash. The syntax highlighting isn't updated, and in this case, moving the cursor over the lines does not help. However, if you save and reopen the file, it is rendered correctly (presumably because the syntax parser cookies are regenerated).

[Vidar Hasfjord]

I have made a small remedy for this issue, for the special case when lines are empty [r7189].

Note that the implementation of TCoolTextWnd::DrawLine skipped parsing for empty lines. The revised code parses empty lines as well. This makes sure that the parser state cookies for blank lines are updated. This is important for the correct parsing of subsequent lines, since parser state cascades.

If you perform the test in the ticket description, with a blank line inserted, the syntax highlighting will now update when you move the cursor over the lines.

However, note that this revision would not be needed, and parsing of blank lines could have been skipped, if TCoolEdit had properly cleared stale cookies when the text is edited. Then the cookies would have been regenerated within GetParserCookie as needed, even for empty lines.

We need a way to invalidate the cookies whenever the text buffer is updated. Is there a callback we can use for this purpose?

Invalidating everything on every key press would be inefficient. To ensure adequate performance, we may need to make sure we invalidate only when strictly needed (i.e. when the cookie of the edited line changes when regenerated), and that the invalidation cascades as far as needed only (e.g. assuming the edit didn't affect line count and ordering, then we can stop invalidating when a cookie of a line below does not change when regenerated).

[Vidar Hasfjord]

Brute-force cookie invalidation is now performed in TCoolEdit::InvalidateLines [r7192], if TInvalidationFlag::InvalidateCookies is passed. Call sites have been updated to use the flag, as appropriate.

Edit: This revision has been reverted, and a better solution has been implemented, as discussed later in this thread.

While this solution invalidates the cookies for all the lines of the file below the cursor for every character typed and modification made, it seems to run surprisingly well, even when typing at the top of files with thousands of lines. And most important of all, it seems to work correctly.

Please review and test, and let us know if you find corner-cases that do not work correctly.

[Erwin Lotter]

This bug has been fixed in branches/7-coolprj-dev with [r8013] by a more selective approach.

[Vidar Hasfjord]

@elotter wrote:

This bug has been fixed in branches/7-coolprj-dev with [r8013] by a more selective approach.

I think it would be good to find a better approach. My quick fix conflates invalidation of lines with clearing of cookies (which seems to have confused your approach as well). The clearing of invalid cookies is a separate issue from the invalidation of visible lines in the editor window.

However, in the current implementation, your selective approach does not work. Try this:

1. Open "basic.cpp".
2. Type /* at the start of the file (comments out the whole file).
3. Press Ctrl+End to jump to the end of the file.
4. Note that the last page is correctly highlighted in comment style.
5. Press Ctrl+Z to undo the change.
6. Press Ctrl+End to jump to the end of the file again.
7. Note that the syntax highlighting has not been updated correctly; the last page is still highlighted in comment style.

"InvalidateCookies" is a misleading function name, since it does not affect the cookies, but only invalidates visible lines on the screen. The cookie data structure is left unchanged (and incorrect) by the function. Hence the bug highlighted above.

A better function name would be "ClearInvalidCookies", assuming the function actually clears the part of the cookie data structure that has been invalidated by the change to the current line.

(Note: Recalculating all the cookies for every change is expensive and very often useless work, since cookies for lines outside the visible range may be overwritten by yet another edit and recalculation before being viewed, if they ever are. Hence a lazy approach seems appropriate here, i.e. simply clear the invalid cookies and have them recalculated on demand.)

PS. Here's some pseudocode by Copilot:

// Cascade check bounded to visible region
int finalLine = firstLine;
for (; finalLine <= lastVisibleLine; ++finalLine) {
  newCookie = SyntaxParser->ParseLine(incomingCookie, finalLine, 0, blocks);
  if (newCookie == ParseCookies[finalLine])
    break;
  incomingCookie = newCookie;
}

// Clear only the lines where cascade was detected
for (int i = firstLine; i < finalLine; ++i)
  ParseCookies[i] = COOKIE_UNINITIALIZED;

// If cascade reached the bottom of the visible window,
// consider all lines below as stale (but don’t parse yet)
if (finalLine > lastVisibleLine) {
  for (int i = lastVisibleLine + 1; i < ParseArraySize; ++i)
    ParseCookies[i] = COOKIE_UNINITIALIZED;
}

// Visual impact limited to visible range
InvalidateLines(firstLine, min(finalLine, lastVisibleLine));

[Erwin Lotter]

After adding the missing cookie invalidation your test case is displayed fine now.
There are two different cases handled:

1. ) After entering or deleting single characters or words the propagation of cookie changes is calculated and the affected lines are invalidated selectively. If also the last visible line is affected, cookies beyond it are cleared.
   I think this is the most critical case since typing characters can be quite frequent and optimizing this might get even noticeable on slow systems with extremely huge amounts of text.
2. ) When calling InvalidateLines() with -1 as the second line parameter, all cookies starting from the first invalid line are deleted. This could be improved by giving InvalidateLines() a third parameter as you did in your implementation.

[Vidar Hasfjord]

@elotter wrote:

1. ) [....] If also the last visible line is affected, cookies beyond it are cleared.

This part of the logic is still missing. See the pseudocode posted earlier.

The reason it now seems to work and my earlier posted repro test case now works fine, is that your new function ClearCookies is coincidentally called by InvalidateLines when navigating the file. This is quite brittle. In particular, you can still recreate the bug by circumventing screen refresh:

1. Open "basic.cpp", and make sure the MDI child window is not maximised.
2. Use Page Down/Up to scroll to the end of the file and back to the start. This ensures that cookies for all lines have been generated (you can set a breakpoint to ensure ClearCookies is not being called by this or following steps).
3. Insert /* at the start of the file (comments out the whole file).
4. Minimise the child window.
5. While the child window is minimized, but still focused, hold down Page Down until you reach the end of the file (see the line number on the status bar), then restore the window again.
6. Note that the syntax highlighting has not been updated correctly; the last page is still highlighted normally (should now be highlighted in comment style).

Moreover, clearing cookies in InvalidateLines is brute-force and causes unnecessary cookie recalculation. Cookies don't change unless there is a change to the text, so cookie invalidation is unnecessary for navigation and scrolling commands.

For these reasons, I think clearing cookies in InvalidateLines is a bad idea and a confusion of concerns. It should be the other way around, i.e. ClearInvalidCookies should call InvalidateLines as appropriate to redraw affected visible lines only. Again, see the pseudocode posted earlier.

I plan to change this on the trunk by introducing ClearInvalidCookies as sketched.

[Vidar Hasfjord]

@elotter committed [r8021]:

CHG: Modified approach to the clearing of invalid parser cookies. During Paint(), when the cookies of the current view are recalculated, a change of the last lines cookie is taken as an indication that the following cookies are probably also invalid.

While this is a creative attempt at (premature) optimisation, it unfortunatly doesn't pass the smell test, neither with regards to design concerns nor with regards to correctness. It is simply a very bad idea to conflate rendering state and the state of the object rendered. Object state should be correctly maintained whether or not it is rendered.

(By the way, I told Copilot on you, and it nearly had a fit! Ask it to explain, if you dare. :-)

For example, here's one way to expose the flaw with this approach:

1. Open "basic.cpp", and leave the cursor position at the start of the file.
2. Minimise the child window.
3. While the child window is minimised, but still focused, type /* (comments out the whole file).
4. With the window still minimised, press Ctrl+End to move to the end of the file.
5. Restore the window.
6. Note that the syntax highlighting has not been updated correctly; the last page is still highlighted normally (should now be highlighted in comment style).

PS. If you really want to proceed to optimisation at this stage, here is a clean approach to consider: Maintain a member variable that keeps track of the lastValidCookie. You can then check against that to generate cookies on demand, with no need to clear anything beyond.

[Erwin Lotter]

Hi Vidar,

this is my reply to your previous post. I missed to dispatch it yesterday.

This part of the logic is still missing.

Oops, thanks for the hint. Somehow it disappeared again.

The reason it now seems to work and my earlier posted repro test case now works fine, is that your new function ClearCookies is coincidentally called by InvalidateLines ..

Not coincidentally, because InvalidateByCookies() wasn't involded in the this test case. InvalidateByCookies() is not called for Undo/Redo because these invalidate up to line -1. I added ClearCookies() to InvalidateLines() for just these cases.

In particular, you can still recreate the bug by circumventing screen refresh:
Open "basic.cpp" ...

Hm, I cannot reproduce this. When I restore the window, I see the last page colored as comment (even before I updated InvalidateByCookies()).
The reason is that PgDn invalidates to -1, so ClearCookies is called from InvalidateLines.
From what you write I conclude that ClearCookies is not called when you page down the minimized window, right?

[Erwin Lotter]

For example, here's one way to expose the flaw with this approach...

Yes, I'm aware of the missing handling of not yet rendered input and I agree to

Object state should be correctly maintained whether or not it is rendered.

Otoh, I find this approach attractive, because it makes use of information, which is already existent, although not complete (under special conditions).

If you really want to proceed to optimisation at this stage, ...

That's a good question. I'm in doubt whether its worth it.

here is a clean approach to consider: Maintain a member variable that keeps track of the lastValidCookie.

Either this way or one could clear all cookies whenever the window is restored. But I fear there might be cases where this still is not sufficient (and will not find your blessing ;-).

By the way, I told Copilot on you, and it nearly had a fit!

What did you tell'm?

[Vidar Hasfjord]

@elotter wrote:

Hm, I cannot reproduce [the earlier bug]. [...] From what you write I conclude that ClearCookies is not called when you page down the minimized window, right?

Correct. I should have quoted the exact revision, but at some point ClearCookies was not called by Page Down, while it was called by Ctrl+End; hence the use of the former in my reproduction steps.

With the latest revision [r8023], these cookie issues seem to have been resolved. I see you have also fixed the latest bug I found with DeleteWord, for which I made a test case in CoolDemo. Good work!

(I encourage you to add test cases for your own bug fixes, for your new functionality, and for all the editor commands in general, as to achieve broad test coverage.)

What did you [tell Copilot]?

I described your solution (relying on cookie clearing in Paint), and it described many ways this is unreliable. In summary:

Are we guaranteed a repaint after every text change? Unfortunately, no—there are several real-world scenarios where a buffer mutation won’t immediately (or ever) trigger Paint(), leaving stale cookies in place: 1. Window Minimization or Being Hidden [...], 2. Programmatic Edits Without Invalidation [...] 3. Off-screen or Virtualized Views [...] 4. Coalesced or Deferred Paints [...] Recommendation: To guarantee correctness—and preserve separation of concerns—you still need an explicit invalidation step on every edit: 1. At edit time: call HandleLineChange/Insertion/Deletion(), which updates firstInvalidLine or directly clears the cookie range. 2. At paint time: consume only up-to-date cookies for drawing, without any side-effects. This hybrid ensures that cookies are correct immediately after a change, and Paint simply renders what the parser already knows—no surprises, no hidden dependencies on window state.

On reviewing my draft reply, Copilot did tell me off slightly for being harsh and suggested a softer tone, but then again, Copilot always likes to sugarcoat everything, so I stuck to my guns. Erwin is an old git (like me), I told it: He can handle the truth. :-)

[Erwin Lotter]

With the latest revision [r8023], these cookie issues seem to have been resolved.

Seems so. I hope it will withstand, because I returned to this. I left the additional code in Paint() as a starting point for future attempts (even if useless at the moment).

[Vidar Hasfjord]

@elotter wrote:

[Regarding cookie clearing in invalidate calls], Not coincidentally, because InvalidateByCookies() wasn't involded in the this test case. InvalidateByCookies() is not called for Undo/Redo because these invalidate up to line -1. I added ClearCookies() to InvalidateLines() for just these cases.

By "coincidentally", I didn't mean "by accident", but rather "at the same time", and somewhat unexpectedly and indirectly, since as we discussed earlier, invalidating lines on the screen for repainting isn't the same as updating the buffer and associated parser state (cookies). I still think separating these concerns would improve the design and make the code clearer, and as mentioned before, it is something I might explore on the trunk.

As you may have noticed in my exchange with Copilot about this, we had discussed implementing some callbacks in TSyntaxParser (HandleLineMutation, HandleLineInsertion, HandleLineDeletion) and make these handlers responsible for updating cookie state, now entirely encapsulated within TSyntaxParser (and not directly visible to the editor). The intention is that the handlers are explicitly called on every relevant change to the text buffer. This seems to be a design and implementation approach worth pursuing.

PS. Here is Copilot's summary of our design discussion:

---

🧩 Proposed TSyntaxParser Handlers Overview

These handler methods allow the parser to respond explicitly to buffer mutations, maintain internal cookie state (via the encapsulated TCookies class), and return a scoped invalidation result for visual refresh.

1. TInvalidation Enum

Defines how a buffer change impacts syntax state:

enum class TInvalidation {
  Contained, // Change affects only the modified line(s).
  Cascading  // Change impacts downstream lines due to parser cookie propagation.
};

2. Handler Methods in TSyntaxParser

Method	Parameters	Purpose
HandleLineMutation	int firstLine, int lastLine	Signals textual edits within a line range; calculates invalidation extent.
HandleLineInsertion	int insertAt, int count	Updates cookie structure to reflect inserted lines; may cause downstream cascade.
HandleLineDeletion	int deleteAt, int count	Removes cookie entries and shifts index alignment; cascade may follow.

Each method returns a TInvalidation value to guide the editor’s redraw logic.

🛠 Internal Support in TCookies

Encapsulates cookie mutation and cache tracking:

class TCookies {
public:
  void InsertLines(int atLine, int count);
  void RemoveLines(int atLine, int count);
  void Invalidate(int fromLine); // Updates FirstInvalidCookieIndex.
  uint32_t Get(int line); // May lazily recalculate.
  void Set(int line, uint32_t); // Internal use, likely private.
  int FirstInvalidLine() const;

private:
  std::vector<uint32_t> Cookies;
  int FirstInvalidCookieIndex;
};

🔄 Flow Summary

1. TCoolEdit mutates the buffer → calls appropriate TSyntaxParser::Handle…() method.
2. Parser adjusts cookie structure via TCookies.
3. Parser returns TInvalidation to the editor.
4. Editor calls InvalidateLines() on visible region depending on invalidation scope.

This modular flow ensures:

• Parser state is maintained separately from rendering.
• Editor redraws only what’s necessary.
• Lazy cookie regeneration supports performance.

[Erwin Lotter]

This sounds like a sound, albeit laborious, approach. The effort required to analyze and incorporate all possible actions in TCoolEdit (including undo/redo) is likely to be very high and error-prone.
I also don't think the performance gain compared to your brute-force approach would be noticeable, especially not on modern systems and if your suggestion of keeping track of the lastValidCookie is implemented.
I, who am not a software engineer, would rather choose the following design anyway, even if it breaks the formally important separation of data and rendering:

• Each buffer line contains its own cookie (of type TCookie, of course). This eliminates all potential sources of error in cookie creation and maintenance.
• All write accesses are consolidated into a few buffer functions. This should make it much easier to concentrate the cookie invalidation logic there in a clear and error-proof manner.
• Additional input from rendering and special treatment of the by far most common action, the character input, would optimize performance.

But since the last point, performance optimization, is so irrelevant, these ideas will likely remain purely fictitious.

[Vidar Hasfjord]

@elotter wrote:

Each buffer line contains its own cookie (of type TCookie, of course). This eliminates all potential sources of error in cookie creation and maintenance.

That's a clever way to make sure there is correspondence between cookies and lines. You may decouple it a bit by making the line data member used for the cookie a generic field, just like List Box and List View controls etc. in the Windows API provide the option of attaching client data to items.

That said, keeping cookies entirely encapsulated within TSyntaxParser seems the cleaner design, and it should be straightforward to manage the cookies correctly, provided the proposed handlers are called as intended, which admittedly is the tricky part to ensure.

All write accesses are consolidated into a few buffer functions. This should make it much easier to concentrate the cookie invalidation logic there in a clear and error-proof manner.

Exactly. And that should make the callbacks to the proposed handlers easy as well, since mutation is encapsulated and restricted to a few buffer functions.

But since the last point, performance optimization, is so irrelevant, these ideas will likely remain purely fictitious.

My focus is on code quality and ease of maintenance — make it easier to work with the code and ensure it is robust. If a cleaner design leads to better performance, that's just a bonus. If it costs a little, that's also fine, as long as it has the same time complexity (e.g. no linear-to-quadratic performance regressions).

PS. Interestingly, after your work on the code, the CoolDemo TCoolEdit benchmark in release mode now performs about the same as the version on the trunk (~7s on "cooledit.cpp" on my laptop). In debug mode, your version now performs significantly worse (56 seconds vs 12 seconds).

[Erwin Lotter]

In debug mode, your version now performs significantly worse (56 seconds vs 12 seconds).

Strange. I didn't add CHECKs or other tests.

[Erwin Lotter]

In debug mode, your version now performs significantly worse (56 seconds vs 12 seconds).

The reason was unnecessary cookie invalidation by TCoolScroller::ScrollTo(), see [r8056].
Thanks for the hint.

[Vidar Hasfjord]

The reason was unnecessary cookie invalidation by TCoolScroller::ScrollTo

To clarify (to the casual reader):

When the whole window needs update (i.e. when ScrollWindow is insufficient), TCoolScroller::ScrollTo calls TCoolTextWnd::InvalidateLines (0, -1) to mark the whole window for subsequent refresh — here performed immediately by UpdateWindow (causing an immediate WM_PAINT message for the invalidated region of the window).

The call to InvalidateLines is for rendering only and has nothing to do with text buffer or syntax parser state (hence nothing to do with syntax parser cookies).

Unfortunately, contrary to this fact, you had added unconditional cookie clearing within InvalidateLines, thereby causing a performance degradation.

(Admittedly, I committed a similar crime on the trunk by also adding cookie clearing inside InvalidateLines, albeit controlled by an optional extra parameter — I want to change this).

I see you've separated the concerns a little now by introducing InvalidateLinesAndCookies.

Resolved by invalidating cookies more selectively like the version on the trunk does.

Super, this clarifies the intent of the code greatly!

It seems you're starting to catch my drift. To recap:

Cookies don't change unless there is a change to the text, so cookie invalidation is unnecessary for navigation and scrolling commands. [...] It is simply a very bad idea to conflate rendering state and the state of the object rendered. Object state should be correctly maintained whether or not it is rendered.

I recommend you remove all your (commented out) code still conflating these separate concerns.

The next step is to delegate to the parser to update syntax highlighting state, strictly only when necessary (on text buffer changes). Then, as outlined in my proposed design, the parser will signal back to the editor whether and which part of the text is affected and may need window refresh for correct syntax highlighting. The main editor code will then be wholly uncluttered by the cookie complexities.

(Cookies are actually a parser implementation detail. For example, consider a parser that does not need cookies at all.)

[Vidar Hasfjord]

I wrote:

I committed a similar crime on the trunk by also adding cookie clearing inside InvalidateLines

This crime has now been reverted, and the proposed cleaner design with better separation of concerns has been implemented instead, encapsulating cookies within TSyntaxParser as private parser state [r8079].

While so far only fully implemented for text insertion, it seems to work well.

Notice how simple ReplaceText becomes. The new function TCoolTextWnd::DoInsertText encapsulates buffer mutation, parser callback and window update. Also see the TSyntaxParser callback implementation, in this case HandleLineMutation. It deals with the parser state (cookies) and determines whether the text change causes cascading changes in the syntax highlighting.

void TCoolEdit::ReplaceText(LPCTSTR text)
{
  PRECONDITION(GetBuffer());
  if (IsReadOnly()) return;

  auto& buffer = *GetBuffer();
  const auto isDirty = buffer.IsDirty();

  auto undoSel = Selection ? make_unique<TUndoDeleteSelection>(*this, isDirty) : nullptr;
  if (Selection)
    DeleteSelection();

  const auto r = DoInsertText(text, CursorPos);
  buffer.AddUndoNode(make_unique<TUndoReplaceText>(r.Start, r.End, std::move(undoSel), isDirty));

  SetCursorPos(r.End);
}

Now, it remains to implement similar encapsulations for text deletion, paste, etc., and do more fine-grained updates where now brute-forced.

Please review and test.

PS. Also, TCoolTextWnd::Paint now draws only invalidated lines [r8078], for more efficient updates of changes to a single line, the most common of operations.