#286 CoolPrj: Port independent bug fixes from branches/elotter-cooledit and the trunk to branch 7

[Ognyan Chernokozhev]

The branches/elotter-cooledit branch (formerly 7-coolprj-dev, forked from branches/7 at [r7871]) redesigns CoolPrj's TCoolEdit: multiple cursors, encodings and a new undo system. Along the way it fixed several bugs that don't depend on the redesign. It also picked up some trunk CoolPrj fixes that never reached branch 7.

Branch 7 users still have all of these bugs. Several are memory errors: reads past the end of a line, and wrong buffer sizes in Unicode builds. The trunk doesn't need them, because its CoolPrj is a full rewrite.

I propose porting the fixes below to branch 7, without the redesign. I have a working-copy patch ready. CoolPrj builds with no warnings at /W3 for x86 and x64, ANSI and Unicode, and the cooldemo example builds in all four configurations.

Trunk fixes not yet in branch 7

• TEnumFormatEtc::Clone leaks the new enumerator when allocation fails ([r5734], [bugs:#521]). Ported as a merge, with svn:mergeinfo.
• regexp.cpp add() sizes its buffer in bytes instead of characters ([r5740], [bugs:#523]). The trunk version scales the copy destination twice: *s + *len*sizeof(_TCHAR) is a byte offset applied to a _TCHAR*. The port uses *s + *len instead.
• Whitespace in the syntax parsers is coloured as an operator ([r7148], [bugs:#581]).

Syntax parsers

• Several ParseLine functions read the character at, or after, the end of the line ([r7999], [r8052]). The port applies the guards to all 20 parsers that have this loop, not just the 11 the branch fixed.
• python.cpp's number test was missing a ! before xisalpha(pszChars[nPos + 1]).
• xisalpha/xisalnum call _istalpha/_istalnum before xisspecial. For non-ASCII characters in ANSI builds, this can trip the CRT's debug assertion ([r7999]).
• The Basic parser compares keywords case-sensitively, and "Select Case" can never match as one keyword ([r8498]).

TCoolEdit / TCoolTextWnd

• The clipboard selection IDs are compared with memcmp(..., 7), which is a byte count. In Unicode builds, every ID therefore matches the stream ID. The column test also checks the line ID a second time. Now _tcsncmp with the column ID is used, in all four places.
• TTextSelection::TextToClipboard and SetDragData treat character counts as byte counts. In Unicode builds they copy half the text, without a terminator. SetDragData also checks hHandle1 where it means hHandle2.
• TCoolEdit::Paste deletes the selection before checking that the clipboard holds text ([r8093]).
• TCoolEditFile::FileOpen(0) ignores CanClear() ([r8192]). Also, a successful open through the dialog returns false.
• ClearRandBookmarks doesn't repaint the lines it clears ([r8553]).
• Auto-indent adds a full tab width per tab instead of moving to the next tab stop ([r7910], [bugs:#616]).
• Text2Client reads past the end of the line when the cursor is beyond it ([r8112]).
• NextBreak calls _tcscspn on line text, which is not null-terminated ([r7985]).
• TEditRange::operator != returns the wrong result ([r7975]).
• The whole-word checks in Find pass possibly negative char values to _istalnum. The port casts them to _TUCHAR. [r8023] used c >= 0 instead, which treats non-ASCII letters as word breaks.

One behaviour change: column paste

With the column ID now recognised, PasteFromMemoryImp would reach its column-insertion loop. That loop never advances: it always rescans from the start of the buffer, so it can hang. TUndoPaste can only undo a stream range anyway. The port therefore pastes column data as plain stream text. That is what branch 7 has always done in practice, because the column ID was never recognised.

Proper column paste, with matching undo, is part of the redesign (TTextColumn and a column-aware TUndoPaste in elotter-cooledit), so it isn't included.

Not included, possible follow-ups

These are in branch 7 today. They're out of scope here, but worth separate tickets:

• TColumnSelection::Copy sets text[range.ecol] = 0 but restores text[endP], and endP is computed from range.erow. Copying a column selection can corrupt the line text.
• TColumnSelection::Delete uses range.srow as the end column and skips column 0.
• PastFromDataObject queries the other character-set format (afe), but then calls GetData(&tfe). It also converts dropped text with A2W/W2A, which allocate on the stack.
• RxReplace frees its malloc'd buffer with delete [] on the error path.

Comments welcome, in particular from @elotter, since most of these fixes come from his branch.

[Ognyan Chernokozhev]

Status update for the trunk:

I've checked all the BUG: entries in branches/elotter-cooledit (about 120) against the trunk CoolPrj. The trunk CoolPrj is a rewrite: it has stream selections only, undo through TSource commands, and a new syntax tokenizer. So most of the fixes have no counterpart there. The ones that do apply are fixed in [r9121]:

• Busy clipboard (as [bugs:#624], [r8209] in the branch): Copy and Cut let the TXClipboard exception escape, because only runtime_error was caught. CopySelectedTextToClipboard now returns bool, and Cut deletes the text only if the copy succeeded. The hex editor's copy had the same problem.
• Caret ([r8102], [r8529], [r9109]):
• A font change now recreates the caret at the new size.
• The caret is no longer drawn in the margin when scrolling horizontally with the scroll bar.
• A click past the end of a line that is scrolled out of view now scrolls the cursor into view.
• Toggling line numbers now updates the scroll range.
• Non-ASCII characters in ANSI builds ([r7999], [r8023]): xisspecial compared a signed tchar with 0x7f, so non-ASCII characters reached _istalpha/_istalnum as negative values. That is a debug assertion, and in release builds non-ASCII letters were treated as operators. The other character classification calls in CoolPrj had the same problem, for example the font enumeration with non-ASCII font names.

Two more trunk bugs turned up while checking these:

• With "Match whole word only", Find skipped the rest of a line after a match inside a longer word. For example, in "cat category cat" it missed the last "cat".
• The "Cannot find" message was formatted into an 80-character buffer. A search term of about 65 characters or more with no match terminated the program.

Not applicable to the trunk:

• Selection, column, multi-cursor and undo fixes: different design.
• Clipboard data sizes and selection IDs, and Paste deleting the selection before checking the clipboard: the trunk code is already correct.
• Parser reads beyond the end of the line, and whitespace colouring: the trunk tokenizer checks bounds and has a whitespace token.
• Text2Client, NextBreak, and auto-indent tab stops: the trunk equivalents are correct.
• File open, bookmarks, the "Drag&Drop" label, the undo limit, and drop conversion: these features don't exist in the trunk TCoolEdit.
• [r5734] is already in the trunk, and the trunk has no "regexp.cpp".
• The rendering fixes ([bugs:#525], [bugs:#625], line gap brush) concern the old renderer and were not ported.

The trunk CoolPrj builds without warnings for Visual C++ (x86 and x64, ANSI and Unicode) and C++Builder (Win64x), and cooldemo builds in all four configurations. A test of the search and character classification fixes fails without the changes and passes with them. The caret changes have not been tested interactively yet, so feedback is welcome.

[Ognyan Chernokozhev]

Implemented in branch 7 in [r9120].

[Erwin Lotter]

Wow, that’s a huge task. I’m surprised it’s even possible to extract relevant fixes from the many changes I’ve made. I also recall that some fixes turned out to be insufficient, and I later improved them as part of more extensive overhauls—without the connection to branch 7 being made clear.
I wonder if it might be more effective to try to make elotter-cooledit API/ABI compatible with cooledit-7, and thereby replace cooledit-7. However, I have no experience with this, so it’s hard for me to say whether that’s feasible.
In any case, I will build branch 7 with the new releases and check what Cooldemo does.

[Vidar Hasfjord]

@elotter wrote:

Wow, that’s a huge task

Indeed! I'm curious to know whether this ticket was proposed on Claude's initiative or instructed by Ognyan.

Anyway, great to see version 7 users getting the bug fixes.

I wonder if it might be more effective to try to make elotter-cooledit API/ABI compatible with cooledit-7, and thereby replace cooledit-7.

As far as versioning goes, if you could make it a non-breaking change, it would be 7.1. If not, it would require a bump to 8, with the trunk then bumping to 9.

However, you would have to consider incompatibilities with the rewrite on the trunk — either accept that functionality is lost going to the trunk, or bring your functionality to the trunk, by reimplementing it within the new design and cleaned up implementation. AI can probably do that in a jiffy, though!

[Ognyan Chernokozhev]

Indeed! I'm curious to know whether this ticket was proposed on Claude's initiative or instructed by Ognyan.

I initiated that request. I am still in control, the machines have not taken over ;-)

I wonder if it might be more effective to try to make elotter-cooledit API/ABI compatible with cooledit-7, and thereby replace cooledit-7.

As far as versioning goes, if you could make it a non-breaking change, it would be 7.1. If not, it would require a bump to 8, with the trunk then bumping to 9.

Maybe the changes can go to the current trunk.

My general plans are: this weekend to release the updates for 6.44 and 7.0, then to continue working on some more of the feasible featrure requests and fixes for the trunk, and in the near future, hopefully before the end of the year, make an official 8 release - there is a lot of work done there already, and Claude can likely offer detailed guidance for porting from 7 to 8.

[Erwin Lotter]

... or bring your functionality to the trunk, by reimplementing it within the new design and cleaned up implementation. AI can probably do that in a jiffy, though!

If AI can manage that, then in my view, it would be the ideal solution.
And I have a few more ideas for minor improvements regarding what AI could do next :-).

[Ognyan Chernokozhev]

Here is the analysis that Claude did on the differences between the branches:

I compared the two checkouts under Subversion\branches. The shell kept failing its safety check, so I couldn't run svn log or diff; this comes from comparing the files directly. If either working copy has local edits, they're included.

Summary

The branch forked from branch 7 a while ago and is a large rewrite of CoolPrj's editor (TCoolEdit and friends). Outside CoolPrj, branch 7 is the one that has moved on.

1. OWL core: branch 7 is newer

Only a handful of OWL headers differ, and in each case branch 7 has the later change. For example, owllink.h has VS 2026/v145 support only on branch 7. theme.h on the branch still has the old tmschema.h fallback, which branch 7 has removed. Small differences in string.h, shellitm.h, module.h, printer.h and the wsk*.h headers look like the same kind of drift. The branch makes no changes of its own outside CoolPrj and the cooldemo example. Merging it back would mean merging only include/coolprj, source/coolprj and examples/cooldemo.

2. CoolPrj: what the branch adds

Area	branch 7	elotter-cooledit
Size	cooledit.cpp 8,413 lines, cooledit.h 1,460	12,475 and 2,320; cooledit.inl folded into the header
File encodings	CR/LF style only (TCrLfStyle)	TEncoding (ANSI, UTF-8 with or without BOM, UTF-16 and UTF-32 in both byte orders) plus TNewlineEncoding; Load/Save code taken from trunk r7864, saves through a temp file
Cursors	One caret and one selection	Multiple cursors (TSelCursor, TMultiCursors, iterators), split column selection, duplicate selection or line, move lines up/down, position history (GoBack)
Undo	Separate TUndoNode and TRedoNode lists, working on the buffer	One TUndoNode with Undo/Redo(TCoolEdit&), cascades, and about 20 concrete undo classes exposed in the header
Colours	int index into a C array; TxColor/BkColor	TColorIndex enum in an unordered_map, adding Function and three diff colours; fields renamed TextColor/BackgroundColor; COLORINDEX_* macros kept for compatibility
Search	Brute force, Boyer-Moore, TRegexp, phonetic and fuzzy engines; separate TCoolReplaceDlg	Only brute force and std::regex; one find/replace dialog (with find-in-selection and forwarding of undo/open/save)
Parsers	Hand-written C/C++ parser; TPlainTextParser	Trunk's 2024 state-machine C++ parser behind an adapter (TCppParserBase); new Arduino, JSON and XML parsers; plain text has no parser
Settings	TConfigFile&	TConfigFile*, plus a local TAppConfig copy of OWL 8's registry-settings code
Language standard	C++17	C++20 (stdcpp20 in the vcxproj and vc.mak)
cooldemo	512-line main.cpp	1,179 lines: adds a "CoolEdit Test" self-test command and an info window listing shortcuts

3. Breaking changes for code built against branch 7

• Renamed or removed API:
• GetCRLFMode/SetCRLFMode, TCrLfStyle, TRedoNode
• TCoolReplaceDlg, the Boyer-Moore, phonetic and fuzzy engines, TPlainTextParser
• Clear(bool)
• GetText into a raw buffer (now fills a tstring)
• TParserInfo::Pattern, now pattern
• resource IDs IDS_COOLREDOSTRINGS, IDC_FINDENGINE and IDC_FINDGLOBAL
• Parser lookup changed:
• Pattern format: entries must now look like "Description (*.ext;...". GetFileTypePattern() does _tcschr(pattern, '(') + 1 with no null check (lang.cpp:485). A branch-7-style AddParser(_T("*.foo"), ...) therefore dereferences a near-null pointer and crashes.
• Search order: lookup now runs front to back. On branch 7 it ran back to front, so parsers added with AddParser overrode the built-in ones; now the built-ins win unless you use AddParserAt(0, ...).
• Dropped file types: the Assembler (*.asm;*.inc;*.s) and Rexx entries are commented out, so those files lose highlighting.

4. Problems I spotted on the branch

1. The C++Builder project is incomplete. CB\coolprj.cbproj doesn't include arduino.cpp, json.cpp or xml.cpp, but lang.cpp references their creator functions. That should fail to link. The VS project and vc.mak do include them.
2. Debug leftover: #pragma optimize("g", off) at cooledit.cpp:20 turns off global optimisation for the whole file in release builds.
3. Settings object built too early: a file-scope TAppConfig appObj(owl::GetApplicationObject(0)) (cooledit.cpp:115) runs during static initialisation, before the application object normally exists. Later, GetSettingsRoot() dereferences the stored pointer, so the registry path may end up null or wrong.
4. Wrong export macro: ConvertToWide is defined in CoolPrj with _OWLFUNC, which is OWL's macro. In builds that use the OWL DLL, that probably expands to dllimport on a definition, which is a compile error.
5. Duplicated code: the old stream-facet encoding code (USE_DEPRECATED_CPP_STREAM_FACETS, set to 0) is still in the file next to its replacement. That's about 1,000 lines, with the BOM tables defined twice.
6. New dependency: PlaySound needs winmm.lib, which apps linking CoolPrj statically may not already link.

Two bugs already exist on branch 7 and the branch didn't fix them:

• GetTxColor() returns the background colour (header line 2153).
• delete should be delete[] for patterns allocated with nstrnewdup in ~TParserSelector.

5. Not verified

• I didn't build either branch.
• I didn't check whether C++Builder's bcc64x handles the C++20 code (char8_t codecvt, std::source_location in the demo).
• I didn't check the branch's revision history.

To get the exact diff once the shell works again:

svn diff --old=C:/Work/OWLNext/Subversion/branches/7/source/coolprj --new=C:/Work/OWLNext/Subversion/branches/elotter-cooledit/source/coolprj

[Erwin Lotter]

I ran a few quick tests on some functions that I worked on at a late stage. As expected, they still contain bugs.:

• Copying in line selection mode copies one extra line.
• Column selection mode behaves very strange. Moving the cursor around seems to insert characters. And other things.
• Find will not proceed to the second finding in a line.
• Regular expression search for "cool" crashes.
• CoolDemo: Menu entry 'Stream Selection' is disabled.
• Dropping text to the right of the source point inserts at destination + X, where X is the lenght of the moved/dropped text. The corresponding in Y happens when dropping below the source point.

I suspect that Unicode handling is also still buggy.

[Vidar Hasfjord]

@elotter and @jogybl,

Are we now at a point where branch 7 introduces new functionality?

Vital test: If I write a program fully dependent on all the features on the branch, can I still replace the libraries (.lib/.dll) with the current release (7.0.21), without recompiling and without breaking my program?

If not, functionality has been added that breaks the forward compatibility of 7.0.21. According to SemVer, a standard we now try to follow, this requires a bump in the minor revision number, i.e. to 7.1. It means that if you go to 7.1 and come to depend on all its features, you cannot go back to 7.0 without losing some features.

As far as I know, bumping to 7.1 has no ramifications of consequence, so there is no reason to be reluctant to move to 7.1 if new functionality has been added. Let's try to stick to SemVer, if we can.

If going to 7.1, this also means you can feel free to add substantial new functionality (e.g. your neat multi-cursor features in CoolEdit), as long as you don't break existing functionality, of course. Upgrading from 7.0 to 7.1 must still only require a rebuild, with no source code changes, and with no build errors.