recordingMacro should avoid TentativeActive()

[johnsonj]

remove TmpRecordingMacro.

[Neil Hodgson]

johnsonj:

> remove TmpRecordingMacro.

Shouldn’t this change be made for GTK+ and Qt as well?

Neil

[johnsonj]

I assume TentativeUndo should be used only for ime interaction.
If my assumption is false, this patch should not be allowed.

The value of 'tmpRecordingMacro' is always equivalent to !TentativeActive().
so !TentativeAcitive() removes the need of tmpRecordingMacro, which will be a redundant variable.
I think it may be also OK just to remove 'tmpRecordingMacro' on gtk an qt

For macro test, I used Notepad++.

[johnsonj]

but it may impact cocoa port.
I hope it may be a positive affect.

[Neil Hodgson]

johnsonj:

> I assume TentativeUndo should be used only for ime interaction.
> If my assumption is false, this patch should not be allowed.

Other uses of tentative undo, if any exist on non-core platforms or are added to the core platforms, may well want the temporary characters to not be recorded into macros since macros should be as simple as possible.

Another potential feature that could use tentative undo is inserting snippets with descriptive placeholders where you don’t want the placeholders to be recorded in macros. Like this where <condition> and <statement> should not appear in the macro:

if (<condition>) {
<statement>

}

> but it may impact cocoa port.

I do not know of any macro recording application on Cocoa. There is a good chance that the current IME implementation on Cocoa is insufficient for macro recording.

Neil

[johnsonj]

Thank your for your instruction.
It takes me to realize this patch is dangerous
since TentativeActive() for ime takes too much long time against other functions.

I will have to try to think about parameters to AddCharUTF() again.

[johnsonj]

I introduce the 4th parameter for imeStates.
I think the 3rd parameter should be set to !IsUnicodeMode().

[Neil Hodgson]

johnsonj:

> I introduce the 4th parameter for imeStates.
> I think the 3rd parameter should be set to !IsUnicodeMode().

I’d like to aim towards a more sensible API where AddCharUTF (or its replacement) is called only for individual characters (not for multiple characters or the bytes inside characters) and reports the character value as UTF-32 in the SCN_CHARADDED notification.

This leads to incompatibilities. Non-core platforms may be calling AddCharUTF(_, _, true) and will have to be changed to omit the treatAsDBCS parameter.

They should also avoid calling AddCharUTF for each byte of a DBCS character as is done in HandleCompositionWindowed. Applications may be written expecting individual bytes here.

Its hard to see how to move to the new API without potentially breaking client code. There could be wholesale code duplication and a mode flag to choose the new API but that looks quite unpleasant. It may be better to break compatibility.

I’d see a replacement call looking like:

enum { addRecord=0, addNoRecord=1 } addCharacterFlags;
AddCharacter(int utf32, std::string text, addCharacterFlags addf);

Neil

[Neil Hodgson]

A recent fix to support non-BMP characters in WM_CHAR and DBCS encodings for WM_UNICHAR reuses code from HandleCompositionWindowed which has the DBCS character slicing issue and which may need to be replaced.

http://sourceforge.net/p/scintilla/bugs/1779/
http://sourceforge.net/p/scintilla/code/ci/ce4680b7180549ac118893aff9034214c412f193/

Neil

[johnsonj]

Thank you for instructions.
I see your intentions.

The new API seems to need more time for well design.
It appears more complicated than I have expected.
I implement it with copy&paste, trial&error, and finally your instructions.
but this is about design.
Let me have long sleeps on it.

=============================================================================
About WM_UNICHAR:
I have suggested to have a chance for these APIs.

inline bool IsLeadSurrogate(wchar_t uch)
inline bool IsTrailSurrogate(wchar_t uch)
inline bool IsSurrogate(wchar_t uch)

Might it be better IS_HIGH_SURROGATE(x) and IS_LOW_SURROGATE(x) reside as functions in uniconversion.h?

Why addChar() is used instead of the existing addCharUTF()?

==========================================================================

[Neil Hodgson]

johnsonj:

> The new API seems to need more time for well design.

Yes. We don’t want to go through the effort of updating applications and platform layers twice.

> It appears more complicated than I have expected.
> I implement it with copy&paste, trial&error, and finally your instructions.

You should be trying to find the set of options that are needed to support all the potential IME actions in a way that can be supported well into the future.

> =============================================================================
> About WM_UNICHAR:
> I have suggested to have a chance for these APIs.
>
> inline bool IsLeadSurrogate(wchar_t uch)
> inline bool IsTrailSurrogate(wchar_t uch)
> inline bool IsSurrogate(wchar_t uch)
>
> Might it be better IS_HIGH_SURROGATE(x) and IS_LOW_SURROGATE(x) reside as functions in uniconversion.h?

The code in the change set comes from Sam Hocevar, not me, but I’ll try to channel his probable reasons.

IS_HIGH_SURROGATE and IS_LOW_SURROGATE are defined by the Win32 API and their uses are only currently in ScintillaWin.cxx which sees the Win32 headers. Almost all builds will use the versions of these macros from the Windows headers.
https://msdn.microsoft.com/en-us/library/windows/desktop/dd318683(v=vs.85).aspx
However, they were introduced for Windows Vista, so there is the possibility that Scintilla will be built with an old version of the Windows headers. Therefore there is also a definition.

I thought about changing these to functions in UniConversion.h but didn’t. There are some small benefits and costs either way. wchar_t is defined to be different sizes on Win32 and Unix and adding more wchar_t functions will spread the problems. If we could depend on C++11 then they could take char16_t and be the same on both platforms.

> Why addChar() is used instead of the existing addCharUTF()?

Because the code was copied from HandleCompositionWindowed. HandleCompositionWindowed and its predecessor HandleComposition have always called addChar.

Neil

[johnsonj]

I have not known IS_HIGH_SURROGATE macro.
Thank you for the tip.

"""wchar_t is defined to be different sizes on Win32 and Unix and adding more wchar_t functions will spread the problems. If we could depend on C++11 then they could take char16_t and be the same on both platforms.""""

Thank you for instructions.