johnsonj:
> """there must be exactly one character which can be extracted from the utf8Notify variable."""
> I am thinking about 'std::vector<std::string> s' like this:
>
> void Editor::AddCharacter(std::vector<std::string> docChar, std::vector<std::string> utf8Char, addCharacterFlags flags);
The logic in AddCharUTF is very strongly character at a time. You don’t want to add several characters then call NotifyChar several times since the application may be expecting each change to have an immediate notification. A convenience method could go around AddCharacter to insert strings of characters. There are also some benefits to handling a string together such as minimising calls to (expensive) WrapOneLine and EnsrureCaretVisible but I don’t think that outweighs confused apps.
> void AddCharacter(std::string docChar, std::string utf8Char="", bool allowMacro=true)
It shouldn’t be too hard for callers to always provide Unicode data so I don’t think utf8Char (or alternatively int utf32) should be optional. Using a bool for the last argument isn’t easy to extend and is part of why we are here. At least make it an enum for now:
enum AddCharacterFlags { addDefault, addRecord }; or {addDefault, addNoRecord} if recording is the common case.
> + struct addCharacterFlags {
Its a class / struct so capital first letter AddCharacterFlags. Methods also start with a cap, and local variables start with lower case so NoTentativeUndo->noTentativeUndo.
If fewer flags are needed then that would be great.
> Notify branch changed a lot.
Join conditions together when they reduce the changes from before:
if (inOverstrike && flags.allowOverstrike) {
instead of
if (inOverstrike) {
if (flags.allowOverstrike) {
Same for recording.
There is still a lot of code that should not be needed. Much simpler:
if (flags.allowNotify) {
NotifyChar(utf32);
}
This should eventually become NotifyCharacter(utf32, docChar) so that the text will also be passed up in SCNotification::text.
> *ClearTentaveStart fixed.
>
> + FilterSelections();
> + {
> + UndoGroup ug(pdoc, (sel.Count() > 1) || !sel.Empty() || inOverstrike);
>
> ClearBeforeTentativeStart in ScintillWin moved under if (lParam & GCS_COMPSTR) following document comments.
noTentativeUndo is just remembering the state of tentative undo early in code so should be something like
const bool tentativeUndoWasActive = doc->TentativeActive();
or (with inverse meaning)
const bool initialCompose = !doc->TentativeActive();
> I used utf8 std::string to get utf32 code point following your path.
Encapsulating the conversion to UTF-32 in a function would avoid extra variables in the complex IME handling and could be reused so you can say:
AddCharacter(docChar, UTF32CharacterFromUTF8(utf8Char));
Neil