SCN_CHARADDED and undo

[Teodor Petrov]

Hello,

I'm experimenting with the brace completion implementation in Code::Blocks.
It works sort of fine, but there is an undo problem. The problem is that
when the
user enters the open brace we handle the char added notification and add the
closing brace if needed. And then if the user wants to undo the brace
addition
he/she has to execute two undo actions to remove the open brace. The first
undo action removes the close brace and then the second undo action removes
the open brace. This is not intuitive and it is quite annoying.

This happens because in Editor::AddCharUTF there are:
1. Explicit scope which ends before the notification is sent. This means
that undo
    grouping ends before the notifications is sent. So our handler can
only start a
    new undo action group.
2. The UndoGroup with grouping disabled most of the time.

If I remove both of these the undo of such brace completions start to be
more
user friendly. I'm sure this is not a proper fix, because 1 and 2 have
been done on
purpose. The question is what can we do to fix this problem? I guess all
handlers
of char added might be affected.

Best regards,
Teodor

[Neil Hodgson]

Teodor Petrov:

> I'm experimenting with the brace completion implementation in Code::Blocks.
> It works sort of fine, but there is an undo problem. The problem is that when the
> user enters the open brace we handle the char added notification and add the
> closing brace if needed. And then if the user wants to undo the brace addition
> he/she has to execute two undo actions to remove the open brace.

For simple cases (one caret) you could, in the char added notification:
send SCI_UNDO to remove the just added bracket
start a compound action
add both the opening and closing bracket
close the compound action

There may be more issues but that appears to allow undoing both in one go.

Neil

[Teodor Petrov]

I've just tested the undo trick and yes it works, but it is a bit
unintuitive and strange.

Also I want it to work for multiple cursors. I've tested a bit the
modification in
Editor::AddCharUTF and it works for this case, but I've not tested it
for anything else.
Can you try to predict what problems I might expect with this change to
the undo
grouping behaviour? Do you remember why it has been implemented in this
manner?

/Teodor

[Neil Hodgson]

Teodor Petrov:

> Also I want it to work for multiple cursors. I've tested a bit the modification in
> Editor::AddCharUTF and it works for this case, but I've not tested it for anything else.

I’d avoid the potential problems by not adding the closing brace when there are multiple selections. Adding a matched brace may help when simply writing new code but when there are multiple selections, the user is probably doing a more complex edit and typing an extra brace is a small cost.

You could look into implementing some more complex undo stack manipulation but that area can be tricky. Say a SCI_CONTINUEUNDOACTION that re-opens the previous undo group (if any) and appends modifications until SCI_ENDUNDOACTION.

> Can you try to predict what problems I might expect with this change to the undo
> grouping behaviour?

AddCharUTF is dealing with multiple issues, including virtual space, protected areas, overstrike (replacement) mode, and ensuring that IME input works well. If your application enables these then check that it works when they are active.

> Do you remember why it has been implemented in this manner?

There is an undo group around multiple selection character adding as that allows them all to be undone in one go.

Neil

[Teodor Petrov]

On Monday, January 22, 2018 at 6:44:50 AM UTC+2, Neil Hodgson wrote:

Teodor Petrov:

   I’d avoid the potential problems by not adding the closing brace when there are multiple selections. Adding a matched brace may help when simply writing new code but when there are multiple selections, the user is probably doing a more complex edit and typing an extra brace is a small cost.

Yes, disabling brace completion when there are multiple-cursors is the easiest option,

but it will be inconsistent and lame. I want to implement it well. As far as I can see it

could be implemented. For example you can go to https://godbolt.org/ and play with

the editor. It does support brace completion for multiple cursors and it works really

well. And also it supports proper undo - both braces are removed with a single undo

command and the additional cursors aren't removed. Note use ctrl-shift-down to

create multiple cursors. As far as I know this is the same editor as the one used in

Visual Studio Code.

 

> Do you remember why it has been implemented in this manner?

   There is an undo group around multiple selection character adding as that allows them all to be undone in one go.

The undo group is active only under certain conditions and it ends before the notification

is sent. Do you know why this is done this way? I'll test the options you've mentioned

to see if they are broken or not.

/Teodor

[Neil Hodgson]

Teodor:

> The undo group is active only under certain conditions and it ends before the notification
> is sent. Do you know why this is done this way?

Typing normally coalesces so you don’t want undo groups as they break the coalescing. When there are multiple carets then the changes aren’t contiguous and you want to undo the whole set at once so there are groups.

Having the group extend over the notification and any application response would likely be more complex.

Neil