SC_MOD_DELETECHECK / SCI_CHANGEDELETION anyone?

[i***@gmail.com]

I have implemented SC_MOD_DELETECHECK and SCI_CHANGEDELETION as
counterparts to SC_MOD_INSERTCHECK and SCI_CHANGEINSERTION in my own copy
of Scintilla. I would like to submit a patch if there is interest.

My motivation was to allow vetoing of changes to certain parts of the
document; SC_MOD_INSERTCHECK makes this possible for text insertion but I
also needed a similar method for text deletion. I originally tried to use
the SCI_STYLESETCHANGEABLE style attribute, but this has a number of
limitations that made it unsuitable for my use case – the most significant
being that it is not permitted to place the caret or make a selection in an
unchangeable region.

To veto an insertion it is just a matter of calling SCI_CHANGEINSERTION
with a length of zero and an empty string. I did have to modify the
implementation slightly so that such null-insertions don't cause an empty
undo action to be stored and the document potentially to leave its
save-point.

SCI_CHANGEDELETION allows the caller to modify the position and length of
the region being deleted. Setting the length to zero vetoes the deletion.

One of the original use-cases described by Neil for
SC_MOD_INSERTCHECK/SCI_CHANGEINSERTION was to replace certain Unicode
characters with equivalent character sequences, e.g. the n-dash character
with – in HTML. My changes extend this use case such that deleting a
single character in a token could be made to delete the entire token.

There were some platform-specific changes that I had to make to deal with
drag-and-drop issues. I have made these only for the Windows platform,
because I know nothing at all about the other supported platforms. One of
the changes is that SCN_MOD_INSERTCHECK is now called repeatedly during a
drag operation so that the editor can show the correct user feedback if the
dragged text could not be dropped at the current position. I would not
expect that this would cause any backwards compatibility problems, unless
the container is doing a very expensive operation in response.

Would you like me to submit a patch (with further explanation of the
changes)? What is the best way to do this?

[Neil Hodgson]

To veto an insertion it is just a matter of calling SCI_CHANGEINSERTION with a length of zero and an empty string. I did have to modify the implementation slightly so that such null-insertions don't cause an empty undo action to be stored and the document potentially to leave its save-point.

This should be changed in Scintilla.

SCI_CHANGEDELETION allows the caller to modify the position and length of the region being deleted. Setting the length to zero vetoes the deletion.

That looks a little dangerous. Scintilla may be performing a multi-step operation such as a rectangular deletion. If the application set the deletion range to overlap some of the other elements to be deleted then Scintilla would be confused. There would have to be additional checks about continued validity of the multi-step operation. A request to fail the deletion or reduce its range is relatively simple. Extending the range or moving it completely are more difficult to handle.

There were some platform-specific changes that I had to make to deal with drag-and-drop issues. I have made these only for the Windows platform, because I know nothing at all about the other supported platforms. One of the changes is that SCN_MOD_INSERTCHECK is now called repeatedly during a drag operation so that the editor can show the correct user feedback if the dragged text could not be dropped at the current position. I would not expect that this would cause any backwards compatibility problems, unless the container is doing a very expensive operation in response.

The container may be assuming in its SCN_MOD_INSERTCHECK handler that the insertion will definitely occur since it has been asked to check it. For mid-drag checks this is not the case and the drag may never be dropped.

Would you like me to submit a patch (with further explanation of the changes)? What is the best way to do this?

Either open an issue on the feature request tracker or post a patch (unified) to this list.
http://sourceforge.net/p/scintilla/feature-requests/

Neil

[i***@gmail.com]

Neil,

Thank you for your feedback. I've attached a patch with my changes as-is
(without addressing your comments yet).

I avoided extensively commenting the source because that seems to go
against the established coding style. So here is a quick commentary on the
changes:

*Document.cxx and Document.h*
1. Added 'deletion' class member to communicate changed deletions back to
Document::DeleteChars(). It's set to invalidPosition unless the deletion
range has been changed.

2. Document::DeleteChars() now calls DeleteCheck() to allow the container
to modify the deletion range. If the range length is made zero then bail
out immediately. DeleteCheck() returns true if the deletion range was
unchanged.

3. Factored the SC_MOD_INSERTCHECK code out of Document::InsertString()
into a new public method Document::InsertCheck() so that it can be called
by ScintillaWin::DragOver() (but see Neil's comment earlier in this thread
about a potential pitfall of this).

4. Document::InsertString() now bails out immediately if the inserted
string is changed to zero length to avoid a spurious undo action and the
document potentially leaving its save-point.

*ScintillaWin.cxx*
1. Replaced bool hasOKText with std::vector<char> dragData because we need
to obtain (and cache) the dragged data as soon as the drag enters the
window so that we can test if it can be dropped at the current position in
order to provide proper user feedback.

2. ScintillaWin::StartDrag() now allows only copy (not move) if
DeleteCheck() indicates that the delete region would be changed in any way.
Without this the dropped text gets inserted in the wrong place because
Scintilla assumes in calculating the position that the source text was
fully deleted. Also, it's confusing to users if the source text is not
fully deleted after what they suppose will be a move operation.

(There is a minor UI issue here in that the drag operation still defaults
to move rather than copy and the user has to hold down the control key to
get a 'copy' cursor rather than the 'can't drop here' cursor. Really it
should default to copy if the source text cannot be deleted.)

3. ScintillaWin::DragEnter() now immediately reads the dragged text and
caches it (see 1). This code is moved here from ScintillaWin::Drop().

4. ScintillaWin::DragOver() calls InsertCheck() to see if the dragged text
can be dropped at the current position. It can be dropped as long as
InsertCheck() doesn't delete the text completely.


Regarding your comments:

I see your point regarding SCI_CHANGEDELETION and multi-step operations. I
don't have a solution to this and don't really have time to work on it. For
my purposes it would be sufficient to allow ChangeDeletion() the options
only of allowing or vetoing a deletion, which would be safe.

Is it really likely that a container will malfunction because it assumes
SCN_MOD_INSERTCHECK is always followed by the actual insertion? I do agree
that it is possible, but I'm doubtful that it is very likely.

One way to resolve both of these issues would be to abandon
SC_MOD_DELETECHECK/SCI_CHANGEDELETION and instead propose a new API that
simply allows vetoing of any change - e.g.
SC_MOD_CHANGECHECK/SCI_VETOCHANGE. I don't think this is as tidy though.

What do you think?

Ian

[Neil Hodgson]

3. ScintillaWin::DragEnter() now immediately reads the dragged text and caches it (see 1). This code is moved here from ScintillaWin::Drop().

The client's drag checking shouldn't be creating replacement text since this will be thrown away. It should just be saying OK/Not until the drop which may then be transformed.

I see your point regarding SCI_CHANGEDELETION and multi-step operations. I don't have a solution to this and don't really have time to work on it. For my purposes it would be sufficient to allow ChangeDeletion() the options only of allowing or vetoing a deletion, which would be safe.

A deletion-veto API seems safer.

Client code that requires more complex transformations should queue up a complex deletion command packet with all the information from the check and then process that in idle time, transforming the packet as needed for any other modifications that occur before it is acted on.

Is it really likely that a container will malfunction because it assumes SCN_MOD_INSERTCHECK is always followed by the actual insertion? I do agree that it is possible, but I'm doubtful that it is very likely.

To evaluate changes I look for things that can go wrong and if I see a potential problem quickly then it is very likely there are more that I haven't seen. Client code does many unexpected things inside notifications.

One way to resolve both of these issues would be to abandon SC_MOD_DELETECHECK/SCI_CHANGEDELETION and instead propose a new API that simply allows vetoing of any change - e.g. SC_MOD_CHANGECHECK/SCI_VETOCHANGE. I don't think this is as tidy though.

It has a much more limited scope which I see as less likely to cause problems.

Neil

[i***@gmail.com]

Post by i***@gmail.com

Post by i***@gmail.com
3. ScintillaWin::DragEnter() now immediately reads the dragged text and

caches it (see 1). This code is moved here from ScintillaWin::Drop().
The client’s drag checking shouldn’t be creating replacement text since
this will be thrown away. It should just be saying OK/Not until the drop
which may then be transformed.

This is the problem with trying to use an existing API in a new way.

A deletion-veto API seems safer.
Agreed - though if I make it a change-veto API then it resolves both
problems.

Client code that requires more complex transformations should queue up a

Post by i***@gmail.com
complex deletion command packet with all the information from the check and
then process that in idle time, transforming the packet as needed for any
other modifications that occur before it is acted on.

I don't have a need for this. It seemed simple to give clients the freedom
to do this sort of thing as a side-effect of using and extending the
existing API but you've now convinced me otherwise.

Post by i***@gmail.com
One way to resolve both of these issues would be to abandon
SC_MOD_DELETECHECK/SCI_CHANGEDELETION and instead propose a new API that
simply allows vetoing of any change - e.g.
SC_MOD_CHANGECHECK/SCI_VETOCHANGE. I don't think this is as tidy though.
It has a much more limited scope which I see as less likely to cause problems.

I will produce an alternative patch along those lines and submit it here.

Thanks again for your valuable feedback.
Ian

[i***@gmail.com]

Neil,

I've started implementing SCN_CHECKMODIFY and SCI_MODIFYVETO but as hinted by something you wrote earlier, I'm running into problems with compound actions. My supposition is that if you have a rectangular selection (for example) you want the entire clear/cut/paste operation to fail if any of the selections has a veto on it. So we need checks not only in the gateway functions but also in some of the Editor functions that call these. There are two problems here: first that you can end up with multiple SCN_CHECKMODIFY notifications for the same basic operation, and secondly it starts to look very much like the implementation of protected regions.

You've written previously of your dissatisfaction with protected regions. Do you think it would be better if I concentrated on improving the implementation and functionality of protected regions, rather than implementing this new veto API? One essential modification that I would need would be the ability to select and copy text in a protected region. I'd also like to make the InsertString and DeleteChars gateway functions check if the text is protected as belt and braces in case checks are missed elsewhere. Can you see any obvious pitfalls? Which approach would you prefer?

Cheers,
Ian

[Neil Hodgson]

Post by i***@gmail.com
I've started implementing SCN_CHECKMODIFY and SCI_MODIFYVETO but as hinted by something you wrote earlier, I'm running into problems with compound actions. My supposition is that if you have a rectangular selection (for example) you want the entire clear/cut/paste operation to fail if any of the selections has a veto on it.

It appears to me that there are three main preferences here for rectangular delete which may be wanted by some users:
1) Fail if any line is vetoed.
2) Check each line and allow individual veto
3) Check each line and allow vetoing some portions

Post by i***@gmail.com
You've written previously of your dissatisfaction with protected regions.

They were included because I thought there may be a need but never implemented anything that used them for real. They haven't been widely used.

Post by i***@gmail.com
Do you think it would be better if I concentrated on improving the implementation and functionality of protected regions, rather than implementing this new veto API?

Your usage seems to be in that area so having a real use case could improve protected regions to where they are more popular.

Post by i***@gmail.com
One essential modification that I would need would be the ability to select and copy text in a protected region. I'd also like to make the InsertString and DeleteChars gateway functions check if the text is protected as belt and braces in case checks are missed elsewhere.

InsertString and DeleteChars can't see protected regions since that depends on the the meaning of styles which is view state. One view of the document may have protected regions and another view of that document may have no protected regions.

It may be better to have a new version of protected which isn't associated with styles.

Post by i***@gmail.com
Can you see any obvious pitfalls? Which approach would you prefer?

I'd prefer a more capable version of protected regions. More details on your application may help motivate choices.

Neil

[i***@gmail.com]

Post by Neil Hodgson

Post by i***@gmail.com
Do you think it would be better if I concentrated on improving the implementation and functionality of protected regions, rather than implementing this new veto API?

Your usage seems to be in that area so having a real use case could improve protected regions to where they are more popular.

Post by i***@gmail.com
One essential modification that I would need would be the ability to select and copy text in a protected region. I'd also like to make the InsertString and DeleteChars gateway functions check if the text is protected as belt and braces in case checks are missed elsewhere.

InsertString and DeleteChars can’t see protected regions since that depends on the the meaning of styles which is view state. One view of the document may have protected regions and another view of that document may have no protected regions.

We'll that puts the kibosh on that idea. I should have realised that was the case.

Post by Neil Hodgson
It may be better to have a new version of protected which isn’t associated with styles.

Yes. My protected regions are associated with the document (but each view uses the same styling so it isn't immediately obvious).

Post by Neil Hodgson

Post by i***@gmail.com
Can you see any obvious pitfalls? Which approach would you prefer?

I’d prefer a more capable version of protected regions. More details on your application may help motivate choices.

It's an editor for a specialised control and instrumentation language that has 'include file' functionality. Unlike C they are used for reuse of common code snippets, not just headers. The engineers don't like using include files because they make the code hard to read. I am implementing in-line display of include files so they can see the included text in context. But clearly these regions must be read-only. I insert the include files when the main file is loaded and use line state to keep track of them.

I'm currently using a background marker to visualise which lines are read-only, but I also have an earlier version that defined a second set of styles with the changeable flag set false. Visually it's fine (though it doubles the number of styles required).

I see a few choices:
1. Document vs view feature. Although my application strictly speaking concerns the document, it could be implemented at the view level because all views have the same presentation.
2. Client vetoes changes via a message vs Scintilla knows which bits are protected. The former is more flexible and avoids overhead for clients that don't need this. But it is less efficient and perhaps more complicated.

At the moment (and my mind keeps changing) I'm inclined to 'do it properly' with a document-level veto system, but I need to come up with an efficient way to handle compound actions. To me it seems wrong that the client should be asked about any one selection region more than once in a particular compound action, but i am quite keen that the entire compound action should be prevented if any component action is vetoed.

I'll be back on this in ernest on Wednesday I hope. (Home DIY day today!)
Ian

[Neil Hodgson]

Post by i***@gmail.com
It's an editor for a specialised control and instrumentation language that has 'include file' functionality. Unlike C they are used for reuse of common code snippets, not just headers. The engineers don't like using include files because they make the code hard to read. I am implementing in-line display of include files so they can see the included text in context.

OK, that is a different concept to those previously examined.

Post by i***@gmail.com
I'm currently using a background marker to visualise which lines are read-only, but I also have an earlier version that defined a second set of styles with the changeable flag set false. Visually it's fine (though it doubles the number of styles required).

A second set of styles doesn't seem a bad idea as it allows modes that emphasise either the main file or the includes or leaves them visually identical.

Post by i***@gmail.com
At the moment (and my mind keeps changing) I'm inclined to 'do it properly' with a document-level veto system, but I need to come up with an efficient way to handle compound actions.

Compound actions could be quite complex as the whole scope of the proposed change needs to be presented to the application. Additional compound actions could be implemented after the application has learnt how to handle the current ones. Maybe there should be some switches that complex actions (rectangular insertion/deletion, multiple selection changes) need to be turned on to occur.

Checking for veto could be done piece by piece but that requires running through the logic twice, first to check for permission and then to make the changes if allowed.

Post by i***@gmail.com
To me it seems wrong that the client should be asked about any one selection region more than once in a particular compound action, but i am quite keen that the entire compound action should be prevented if any component action is vetoed.

I suppose a good example here would be deleting a rectangular selection that includes portions of the main and include files in your scenario. The user's action doesn't really make sense and performing just part may leave them confused with some of the context gone. Ideally there'd be an indication of a failure such as showing a message while styling the read-only section distinctively for a brief period.

When selecting read-only text, a different colour (or other visual style) could be used to indicate it can't be deleted.

Neil

[Paul K]

I’d prefer a more capable version of protected regions. More details on

your application may help motivate choices.

I'm interested in this functionality as well and can describe my use cases.
I'm developing a Lua IDE (http://studio.zerobrane.com) and all the use
cases are related to that IDE.

1. Console with a prompt that shows results of earlier commands. Right now
I use a workaround that keeps the editor read-only and I only enable writes
to it when I detect that the change is requested in the right area (this is
done on key presses and dragging fragments over the editor). It works fine
for my purpose, but would be less code if I could simply protect a
particular region.

2. For educational purposes, I'd like to be able to limit the editable
region to a small area where I expect users to make changes. I can then
check the results to see if the script compiles and produces correct
results. This may be a body of a function, a call for a test, or something
similar.

3. I'd like to provide hex editor as a plugin. The hex editor has regions
that are edited differently and the text between the regions shouldn't
change at all. It's possible to implement a workaround similar to what I
did for #1, but it would be much less work if the protected regions were
available. Thank you.

Paul.

[Neil Hodgson]

1. Console with a prompt that shows results of earlier commands. Right now I use a workaround that keeps the editor read-only and I only enable writes to it when I detect that the change is requested in the right area (this is done on key presses and dragging fragments over the editor). It works fine for my purpose, but would be less code if I could simply protect a particular region.

Currently, protected styles are implemented but the implementation may not be complete. If you are using your own lexer you can duplicate all the styles with the second set being protected.
http://www.scintilla.org/ScintillaDoc.html#SCI_STYLESETCHANGEABLE

Neil

[i***@gmail.com]

Paul:
Thanks for your interest. I seem to have settled on the following API:

SCN_MODIFYCHECK – notification of position and length when a modification
is about to take place.
The notification handler can call SCI_VETOMODIFICATION (no parameters) to
veto the modification.

The handler is called every time the text of the document is about to be
modified in any way, and can decide whether to veto the action according to
any arbitrary criteria. Scintilla doesn't store any information about which
portions of the document are read-only. For some modifications the handler
is called multiple times with different values of position and length –
e.g. deletion of a multiple or rectangular selection. If the length
parameter is zero then the change is an insertion, otherwise it is a
deletion.

There are two additional functions: SCI_GETVETOMODIFICATIONFLAGS and
SCI_SETVETOMODIFICATIONFLAGS. The only flag currently defined is
SC_VETOMODIFICATION_PARTIAL. If this is set then for single actions that
result in multiple calls to the SCN_MODIFYCHECK handler the vetoes will be
applied individually and any ranges not specifically vetoed will be
modified. Otherwise just a single veto from the handler (on any of the
ranges) will veto the entire action.

Of course all of this is highly provisional.

I'm hoping to publish a patch to scintilla-interest in the early part of
next week. I'd be very grateful if you would try it and let me have your
feedback.

Ian

[Neil Hodgson]

If the length parameter is zero then the change is an insertion, otherwise it is a deletion.

Are you providing the potentially inserted text? That would require length to be the length of the insertion instead of 0.

At one point in the design of SC_MOD_INSERTCHECK I considered including a parameter indicating the operation being performed like paste/drop/typing but couldn't see an example where it would be needed so didn't include it.

Some changes can be seen as modifications that are implemented as a sequence of deletions and insertions. One such command you should consider is case conversion such as upper-casing all of a rectangular selection that covers both read-only and read-write areas.

Neil

[i***@gmail.com]

Post by Neil Hodgson

If the length parameter is zero then the change is an insertion, otherwise it is a deletion.

Are you providing the potentially inserted text? That would require length to be the length of the insertion instead of 0.

No. Perhaps it would be less confusing to describe the parameters as indicating which part of the original document is about to be modified, rather than to talk explicitly about insertions and deletion. The decision to veto a modification is taken purely on the document. If someone wants to veto based on the text being inserted they can use SC_MOD_INSERTCHECK.

Post by Neil Hodgson
At one point in the design of SC_MOD_INSERTCHECK I considered including a parameter indicating the operation being performed like paste/drop/typing but couldn’t see an example where it would be needed so didn’t include it.

I too considered this briefly, but couldn't see an application.

Post by Neil Hodgson
Some changes can be seen as modifications that are implemented as a sequence of deletions and insertions. One such command you should consider is case conversion such as upper-casing all of a rectangular selection that covers both read-only and read-write areas.

This is what I meant by 'compound actions' in what I wrote a few days ago. I've since noticed that the documentation uses this term to refer to user actions in an undo group, so perhaps I should have coined a different term.

It turns out that making these kinds of actions behave properly is a lot more work than I anticipated, and there are a number of options (virtual space, multiple carets, etc) that interact and change the behaviour of some of the actions in complex ways.

The default behaviour is that the entire user action should do nothing (not even modify the selection) if any part of it is vetoed (though there is a flag to modify this behaviour as I wrote previously). I've written a tiny RAII class that is responsible for checking for vetoes in the top level function of a user action, and also making sure that once this check has been done it isn't repeated in the same user action (so the client is never asked twice about the same range). This is working well in most cases, but there are a few awkward cases. Pasting into a rectangular selection seems to be the hardest.

I expect to spend a few more days before this is ready. Do you have any kind of formal testing procedures for Scintilla? At the moment I'm testing it manually as I go along.

Ian

[Neil Hodgson]

Post by i***@gmail.com
I expect to spend a few more days before this is ready. Do you have any kind of formal testing procedures for Scintilla? At the moment I'm testing it manually as I go along.

There are two sets of automatic tests. scintilla/test contains API tests written in Python that run on Win32 or Qt (simpleTests.py is the main script). Unit tests for internal classes are in scintilla/test/unit written in C++ using the Catch testing framework.

None of these test events which would be needed for your application veto.

Neil

[i***@gmail.com]

I wrote earlier that I'd submit a patch in the next couple of days. I'm
holding off a bit longer while I get the code integrated into my project so
that I can iron out any remaining bugs and other issues.

It's a rather large patch (all the checking to ensure public API calls and
user edits either complete in their entirety or, if any part is vetoed not
at all) so I very much doubt Neil would want to put it in the next release
anyway, which I understand is approaching feature freeze.

[Neil Hodgson]

It's a rather large patch (all the checking to ensure public API calls and user edits either complete in their entirety or, if any part is vetoed not at all) so I very much doubt Neil would want to put it in the next release anyway, which I understand is approaching feature freeze.

Large changes like this should land early in a release cycle so they receive more testing before distribution.

Neil