SC_MOD_DELETECHECK / SCI_CHANGEDELETION anyone?

[ian.g...@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]

ian.goldby:

> 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

[ian.g...@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]

ian.goldby:

> 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

[ian.g...@gmail.com]

Neil Hodgson wrote:

> 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 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.

> 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

[ian.g...@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]

Ian:

> 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

> 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.

> 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.

> 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.

> 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

[ian.g...@gmail.com]

Neil:

>> 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.

>> 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.

>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).

>> 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]

Ian:

> 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.

> 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.

> 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.

> 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]

Neil:

> 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]

Paul K:

> 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

[ian.g...@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]

Ian:

> 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

[ian.g...@gmail.com]

Neil:

>> 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.

> 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.

>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]

Ian:

> 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

[ian.g...@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]

Ian:

> 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

[ian.g...@gmail.com]

I'm submitting a new patch for vetoing of document modifications. I know that the release of 3.5.0 is imminent and I was going to wait until after that but a couple of people here have expressed interest, so consider this a preview. This patch is against version 3.4.4.

I've done what I consider to be fairly thorough manual testing, but the changes are much more extensive than I originally expected, so there is definitely the potential for bugs. That said the patch looks worse than it is; much of it is merely additional indentation in new if blocks.

There are three new messages and one new notification:

SCI_VETOMODIFICATION
This may be called only from a SCN_MODIFYCHECK notification handler and will veto the action, preventing the document from being modified.

SCI_SETVETOMODIFICATIONFLAGS(int flags)
SCI_GETVETOMODIFICATIONFLAGS
These messages set and return the veto modification flags. Currently only one flag is defined: SC_VETOMODIFICATION_PARTIAL. This flag affects the behaviour when a single action is composed of a series of individual document modifications, for example, deleting a multiple selection. If the flag is not set the a veto on any individual modification prevents the entire action. If it is set then only that individual modification is prevented, which means the action will be partially completed. The default is not set.

SCN_MODIFYCHECK
Sent to the container when Scintilla needs to check if a modification to a specific region in the document is allowed. The position field is set to the start of the region that will be modified and the length to its length. The handler may call SCI_VETOMODIFICATION to prevent the modification going ahead. SCN_MODIFYCHECK may be sent multiple times for a single action, for example, once per selection when deleting a multiple selection.


Here's a quick guided tour of the patch:

The biggest challenge was (unless SC_VETOMODIFICATION_PARTIAL is set) for each action to do all of the veto checking before making any modifications to the document so that the entire action can be aborted if any individual modification is vetoed. (This means not even changing the selection.) The new RAII class ModifyVetoChecker is for this purpose. Every action with the potential to discover a veto after it has already modified something declares an instance before it does anything that could modify the document.

There are three constructors, but they all check for vetoes on the range or ranges passed in. The checking is done in the constructor to force the programmer to do all of the checking up-front and in one go. Once the checks are done it sets pdoc->vetoModificationChecked = true. When the ModifyVetoChecker object goes out of scope this is reset back to false. This allows nesting of ModifyVetoChecker objects without repeating the checks. Only the ranges given to the outermost checker are checked, so the outermost check must be comprehensive.

Document::InsertString() and Document::DeleteChars() also check the vetoModificationChecked flag and skip the SCN_MODIFYCHECK notification if the flag indicates it has already been done.

This serves a dual purpose. If an action doesn't make use of ModifyVetoChecker (by error or design), the individual modification(s) will nevertheless be subjected to possible veto in InsertString()/DeleteChars(). This means that even if I've missed places that should use ModifyVetoChecker, the vetoed parts of the document are nevertheless safe from modification. Secondly, the SC_VETOMODIFICATION_PARTIAL implementation is trivial - if this flag is set then ModifyVetoChecker does nothing, leaving the check to InsertString() and DeleteChars().

The major shortcoming of this design is that the range(s) passed to the ModifyVetoChecker constructor might, due to programmer error, differ from the ranges actually modified later in the function, in which case part of the document in a vetoed range could be modified.

Lots of methods in Editor have had a ModifyVetoChecker added. PasteRectangular() needed extensive re-working to incorporate ModifyVetoChecker in a sane way. The external behaviour should be unaffected if there are no vetoes.

(I commented out Editor::DelChar() because it is dead code.)

Since Document::ConvertLineEnds() acts on the entire document rather than on the selection I decided it should simply skip converting line endings in vetoed regions. Reversing the direction of the loop made the housekeeping much simpler. Otherwise the algorithm is unchanged.

Known bugs:
Editor::CanPaste() should return false but returns true in the case where the selection is rectangular, the insertion point is not vetoed, but the bottom part of the rectangle will impinge upon a vetoed region.

Similarly, when dragging a rectangular selection the mouse cursor will incorrectly indicate that the text can be dropped if the insertion point is in a non-vetoed region but part of the rectangle would impinge upon a vetoed region.

Indent and dedent require that the entire line is free from vetoed regions, even though strictly only the leading whitespace should have to be free from vetoed regions.

Only the Windows platform-specific changes are implemented.

Scope for improvement:
The fact that the checking and making the modifications are independent is a potential source of bugs and complexity in the code. It would be better to replace the current internal interface for low-level document modifications with a transaction-based approach, which would allow arbitrarily complex modifications to the document to be carried out atomically. But this would be a major piece of work and is not something I can contemplate taking on at the moment (though I do have an idea of how it might be implemented).

I look forward to your comments.

Ian

[Neil Hodgson]

Ian:

I'm submitting a new patch for vetoing of document modifications. I know that the release of 3.5.0 is imminent and I was going to wait until after that but a couple of people here have expressed interest, so consider this a preview. This patch is against version 3.4.4.

   This is a large patch so will require quite a bit of review.

There are three new messages and one new notification:

SCI_VETOMODIFICATION
This may be called only from a SCN_MODIFYCHECK notification handler and will veto the action, preventing the document from being modified.

   The SCN_MODIFYCHECK / SCI_VETOMODIFICATION mechanism seems basically OK

SCI_SETVETOMODIFICATIONFLAGS(int flags)

   It took a while for me to understand the partial state - I thought it was response from the application when it receives a SCN_MODIFYCHECK saying that it wanted veto over parts of the current action but its a long term state about whether it always want partial veto.

Lots of methods in Editor have had a ModifyVetoChecker added. PasteRectangular() needed extensive re-working to incorporate ModifyVetoChecker in a sane way. The external behaviour should be unaffected if there are no vetoes.

   Not sure if a ModifyVetoChecker is needed for implementing APIs that will always be called by the container and are not the result of Scintilla handling user actions. For example AddStyledText isn’t called except to implement SCI_ADDSTYLEDTEXT. ReplaceTarget and SCI_SETTEXT and others are similar.

   Also unsure about ModifyVetoChecker being an object instead of a set of calls as it (currently) only yields a boolean result.

(I commented out Editor::DelChar() because it is dead code.)

   Its good to simplify major patches by breaking off pieces that can be applied independently. This looks like it can be done before the rest of the patch. Editor::DelChar looks like it became dead code with change set 2957 in 2009 which implemented discontiguous selection and virtual space.

Scope for improvement:
The fact that the checking and making the modifications are independent is a potential source of bugs and complexity in the code. It would be better to replace the current internal interface for low-level document modifications with a transaction-based approach, which would allow arbitrarily complex modifications to the document to be carried out atomically. But this would be a major piece of work and is not something I can contemplate taking on at the moment (though I do have an idea of how it might be implemented).

   Some sort of container of insertions/deletions may be a useful tool but it would be more work. It is complicated by actions changing the selection, particularly for multiple and rectangular selection.

   Neil

[ian.g...@gmail.com]

Neil:

   Not sure if a ModifyVetoChecker is needed for implementing APIs that will always be called by the container and are not the result of Scintilla handling user actions. For example AddStyledText isn’t called except to implement SCI_ADDSTYLEDTEXT. ReplaceTarget and SCI_SETTEXT and others are similar.

If APIs that can't be invoked directly by user actions didn't check for veto, then an application would need to distinguish between those that do and those that don't, and perform explicit veto checking for those that don't. I strongly prefer having a common pattern for all APIs, with all checking done via SCN_MODIFYCHECK; I think it is much simpler and more robust.
 

   Also unsure about ModifyVetoChecker being an object instead of a set of calls as it (currently) only yields a boolean result.

The only reason it is an object is so that it resets pdoc->vetoModificationChecked when it goes out of scope. This makes it impossible to leave the flag true between API calls. It's a programming convenience. The same could be achieved by careful placement of explicit statements to set pdoc->vetoModificationChecked = false on all relevant exit paths but it would prove more error prone, especially for off-normal code paths.

 

(I commented out Editor::DelChar() because it is dead code.)

   Its good to simplify major patches by breaking off pieces that can be applied independently. This looks like it can be done before the rest of the patch. Editor::DelChar looks like it became dead code with change set 2957 in 2009 which implemented discontiguous selection and virtual space.

I'll submit this part as a patch on the bug tracker shortly.

   Some sort of container of insertions/deletions may be a useful tool but it would be more work. It is complicated by actions changing the selection, particularly for multiple and rectangular selection.

Yes, and I really don't have time to work on this at the moment anyway.

It would be great if this patch could be bashed into shape for the next major release. For me it is working as I want it, but I'm not using all of the functionality of Scintilla and there are probably use-cases I haven't considered.

Ian

[Neil Hodgson]

Ian Goldby:

> If APIs that can't be invoked directly by user actions didn't check for veto, then an application would need to distinguish between those that do and those that don't, and perform explicit veto checking for those that don’t.

Scintilla wouldn’t ask the application whether it wants to veto non-user actions. Possibly add a noNeedToCheckVeto field to Document that is set and reset by each non-user action. In particular, whole document actions like SCI_SETTEXT and SCI_CONVERTEOLS can be easily avoided by the application if it has any reason for protecting the document.

> The only reason it is an object is so that it resets pdoc->vetoModificationChecked when it goes out of scope. This makes it impossible to leave the flag true between API calls. It's a programming convenience. The same could be achieved by careful placement of explicit statements to set pdoc->vetoModificationChecked = false on all relevant exit paths but it would prove more error prone, especially for off-normal code paths.

The interaction of the veto state is complex and it would be good to simplify it.

> It would be great if this patch could be bashed into shape for the next major release. For me it is working as I want it, but I'm not using all of the functionality of Scintilla and there are probably use-cases I haven't considered.

The patch is a little difficult to apply and it would be good to update it to 3.5.0 or current Hg. The API numbers should start at 2678 now or just start at 2900 and they can be fixed when its committed.

The size of the patch makes it difficult to sort out the significance of changes. Avoiding blocks for the !vc.Vetoed and using early returns/breaks makes it easier to check although it may not be the best form for later comprehension.

There are a couple of changes that are complex: Document::ConvertLineEnds (inverts direction) and Editor::PasteRectangular. If there are problems in these methods that can be fixed independently of the bulk changes then these should be done beforehand. My recollection of PasteRectangular is that it was complexified to deal with some unusual cases at file end and with either preserving or updating line end characters. There may be details in Hg.

This patch implements two techniques (partial and complete veto), but you will only use the complete veto mode. The complete veto mode not only adds complexity to the current code but it will need maintenance to remain viable: any new compound action should have a ModifyVetoChecker but its likely that contributors will forget or not realise this.

Without input from people that want partial veto, its difficult to justify its inclusion at this point.

Can the protection desired be completely described as a set of non-empty ranges where text can be inserted at the range ends along with a set of positions where text can not be inserted? A document

Name: xx\n
Number: 99\n

with fixed line start explanations could specify: protect(0:6,9:17), noInsertion(0,9).

Neil

[ian.g...@gmail.com]

Hi Neil,

Unfortunately I'm going on holiday tomorrow and won't be on email for 3 weeks, so I won't be able to deal with this until I get back.

A few comments and questions though:

> > If APIs that can't be invoked directly by user actions didn't check for veto, then an application would need to distinguish between those that do and those that don't, and perform explicit veto checking for those that don’t.

> Scintilla wouldn’t ask the application whether it wants to veto non-user actions. Possibly add a noNeedToCheckVeto field to Document that is set and reset by each non-user action. In particular, whole document actions like SCI_SETTEXT and SCI_CONVERTEOLS can be easily avoided by the application if it has any reason for protecting the document.

In the current patch it *does* ask. In my use case (at least) it doesn't matter *why* the action was initiated; there are certain parts of the document that must not be modified regardless of whether it was the user interacting directly with the editor or whether the application sends Scintilla a message such as SCI_SETTEXT.

I think we might be at cross-purposes here, because I can't understand why anyone would want their application to be able to make a change to the document via an explicit API message that the user shouldn't be able to make by interacting directly with the Scintilla window. If there is a use case where this is needed, it is a simple matter to make the handler for SCN_MODIFYCHECK allow or disallow changes depending on how they were initiated. After all, SCI_SETTEXT doesn't work on a document with SCI_SETREADONLY(True), so why should it work on a document where SCN_MODIFYCHECK denies the modification?

> > The only reason it is an object is so that it resets pdoc->vetoModificationChecked when it goes out of scope. This makes it impossible to leave the flag true between API calls. It's a programming convenience. The same could be achieved by careful placement of explicit statements to set pdoc->vetoModificationChecked = false on all relevant exit paths but it would prove more error prone, especially for off-normal code paths.

> The interaction of the veto state is complex and it would be good to simplify it.

Again, I'm not sure what you mean. To me, having to explicitly reset the vetoModificationChecked flag at the exit of every API is more complex than having a class to do it automatically. However, if you prefer the former I will (reluctantly) modify the patch accordingly.

> > It would be great if this patch could be bashed into shape for the next major release. For me it is working as I want it, but I'm not using all of the functionality of Scintilla and there are probably use-cases I haven't considered.

> The patch is a little difficult to apply and it would be good to update it to 3.5.0 or current Hg. The API numbers should start at 2678 now or just start at 2900 and they can be fixed when its committed.

I will do this. I want to merge in 3.5.0 anyway.

> The size of the patch makes it difficult to sort out the significance of changes. Avoiding blocks for the !vc.Vetoed and using early returns/breaks makes it easier to check although it may not be the best form for later comprehension.

My personal preference is for early return anyway, so I'll change it to this style.

> There are a couple of changes that are complex: Document::ConvertLineEnds (inverts direction) and Editor::PasteRectangular. If there are problems in these methods that can be fixed independently of the bulk changes then these should be done beforehand.

There were no problems in these methods to fix. The changes were purely to make the implementation of vetoes manageable. The behaviour of these two methods should be unaffected, other than the addition of the veto capability.

Would you like me to submit these 'enabling' changes as separate patches? It might look slightly odd out of context, but will reduce the size of the veto patch.

> My recollection of PasteRectangular is that it was complexified to deal with some unusual cases at file end and with either preserving or updating line end characters. There may be details in Hg.

I will check the Hg history to see if there is anything I hadn't thought about that I might have broken. I did notice the line-end character thing though, so I think I've got that one covered.

> This patch implements two techniques (partial and complete veto), but you will only use the complete veto mode. The complete veto mode not only adds complexity to the current code but it will need maintenance to remain viable: any new compound action should have a ModifyVetoChecker but its likely that contributors will forget or not realise this.

I can't disagree. It would be much more satisfactory to implement transactions for document modifications so that any modification, no matter now complex, can be applied atomically. But this would be an even more disruptive change to the code base, and require time and effort I just can't offer at the moment.

Ultimately you need to tell me if you can accept a patch like this that creates the risk of contributors not realising they have to use a ModifyVetoChecker. If I were in your position, I'd have misgivings too.

> Without input from people that want partial veto, its difficult to justify its inclusion at this point.

I can remove this. I only put it in because it required almost no additional code to implement.

> Can the protection desired be completely described as a set of non-empty ranges where text can be inserted at the range ends along with a set of positions where text can not be inserted? A document

>  Name: xx\n
>  Number: 99\n

> with fixed line start explanations could specify: protect(0:6,9:17), noInsertion(0,9).

You could do that. I'm not sure where this is going though. Are you suggesting storing the protection information in Scintilla, rather than using a callback?

(Sorry if I'm sounding argumentative or negative in this message. I think it's because I don't quite understand some of your suggestions/comments.)

Ian

[Neil Hodgson]

Ian Goldby:

> I think we might be at cross-purposes here, because I can't understand why anyone would want their application to be able to make a change to the document via an explicit API message that the user shouldn't be able to make by interacting directly with the Scintilla window.

Your application, IIRC, shows the contents of referenced files inline. If one of those files is changed on disk by another process or user then this may be detected (through polling or similar) by the application which would then replace that files content in Scintilla. This is an action that is fine for the application to perform but not the user.

> If there is a use case where this is needed, it is a simple matter to make the handler for SCN_MODIFYCHECK allow or disallow changes depending on how they were initiated. After all, SCI_SETTEXT doesn't work on a document with SCI_SETREADONLY(True), so why should it work on a document where SCN_MODIFYCHECK denies the modification?

I’m just not seeing that it is necessary to check inside SCI_SETTEXT since this can’t be performed by the user. If there is no need to check then that piece of checking code can be removed, simplifying this change. This also reduces the need for adding modification checking to new features unless they can be directly performed by the user.

> Again, I'm not sure what you mean. To me, having to explicitly reset the vetoModificationChecked flag at the exit of every API is more complex than having a class to do it automatically. However, if you prefer the former I will (reluctantly) modify the patch accordingly.

A problem is using additional state flags on Document to control this. Adding state can lead to problems with various unexpected execution paths such as re-entrance. Another approach could be to add a default true ‘performCheck’ parameter to DeleteChars/InsertString that can have false passed in complex actions where ModifyVetoChecker has been used.

> There were no problems in these methods to fix. The changes were purely to make the implementation of vetoes manageable. The behaviour of these two methods should be unaffected, other than the addition of the veto capability.

The implementation of PasteRectangular looks very different so it is difficult to determine if it is actually performing the same actions.

> Would you like me to submit these 'enabling' changes as separate patches? It might look slightly odd out of context, but will reduce the size of the veto patch.

I expect that ConvertLineEnds should be checking the result of DeleteChars in a couple more places with no need for explicit ModificationVetoed calls since there is a check in DeleteChars. For PasteRectangular, equivalences is not obvious: for example, it looks like the new code may use a different style for determining how many spaces to insert and may round differently.

> I can't disagree. It would be much more satisfactory to implement transactions for document modifications so that any modification, no matter now complex, can be applied atomically. But this would be an even more disruptive change to the code base, and require time and effort I just can't offer at the moment.

There is already a transaction system for content: the undo stack. For the recent changes for inline entry of Asian text, the Tentative* calls allow text modifications that are later completely cancelled leaving no trace in the undo history.

One technique could be to start a tentative modification at the start of complex modifications and then to call TentativeUndo/TentativeCommit depending on whether a veto occurred. This could be implemented in a wrapper around complex modifications with an exception used to propagate the veto from InsertString/DeleteChars.

However, this would need to coordinate with the Asian entry code, possibly by implementing nested transactions, with the Asian entry actions being the outer transactions as they last between key presses.

> > Can the protection desired be completely described as a set of non-empty ranges where text can be inserted at the range ends along with a set of positions where text can not be inserted?

There is a case not covered here where protected blocks can not be modified but can be removed completely.

> You could do that. I'm not sure where this is going though. Are you suggesting storing the protection information in Scintilla, rather than using a callback?

Yes, that may be possible although there may still need to be a callback to allow changing the protection ranges if the document has been modified since the last check. There is the existing protected style, which can’t be removed except with a lengthy deprecation period, and using common code for the two similar features would decrease implementation costs and lead to more consistent behaviour.

> (Sorry if I'm sounding argumentative or negative in this message. I think it's because I don't quite understand some of your suggestions/comments.)

I’m trying to explore the implementation space to see what the alternatives are and which benefits each provides.

Neil

[ian.g...@gmail.com]

Hi Neil,

I'm back from holiday now.

I'd still like to get modification veto into the main Scintilla codebase but I have very limited time available to spend on this now. Still, I'll see what can be done.

It seems to me that there are 3 significant questions to explore:

Should Scintilla apply vetos to APIs that can't be invoked directly by the user (e.g. SCI_SETTEXT)?

Arguments for:
1. That's how SCI_SETREADONLY already works. SCI_SETTEXT won't change a read-only document.
2. All of the application's veto logic is in one place.
3. The application developer doesn't need to remember which APIs do their own checking and which leave it to the calling application. Some APIs are invoked both directly by the user and indirectly through application code.

Arguments against:
1. More veto checking in the Scintilla codebase.
2. Scintilla developers might forget to add veto checking to new APIs.

You already know which option I favour. I'm hoping to convince you that the added complexity in the Scintilla codebase is a worthwhile trade-off. I will try further to convince you by bringing the patch up to date with 3.5.0 and simplifying it where possible.

Could tentative undo or transactions be a better way to implement vetos?

I've already proposed a transaction-based approach where modifications are queued up and then committed, but it would be a highly disruptive change to the codebase. Essentially, the document 'gateway' functions would all go and be replaced with a transaction class that queues up modification steps and then commits them to the document at the end of the transaction.

Would a system based on TentativeUndo/TentativeCommit preserve all aspects of the document? It looks to me as if it is an extension of standard Undo, which is text only; it doesn't preserve markers, indicators, annotations, fold state, SCI_GETENDSTYLED, etc.

I think implementing a transactional system that preserves everything would be quite hard to do with undo, but relatively easier if the actual document were not touched until the transaction were committed.

Extend the existing protected text feature (SCI_STYLESETCHANGEABLE)?

The major issue I found with this was that I need it to be possible to place the caret and make selections inside a protected region (just as you can if the entire document is read-only). I suppose this could be made into an option, but I suspect that quite a bit of the implementation relies on the current behaviour. Additionally, we'd still need something like the VetoModificationChecker to prevent partial actions, and this is where most of the complexity comes from in the first place.

Right, my next task is to prepare a new patch.

Ian

[ian.g...@gmail.com]

This patch refactors the implementation of Document::ConvertLineEnds() in preparation for the change veto functionality.

I have tested all six combinations of from and to formats and can confirm that it works exactly the same as the former implementation.

The reason for reversing the direction of the loop is so that when the number of characters in the line ending sequence changes, it does not affect the position of changes yet to be made as the loop progresses. Applying this patch will have no effect on the operation of Scintilla, but it will greatly simplify the implementation of change veto in this function.

[ian.g...@gmail.com]

This is a second preparatory patch for the change veto functionality.

It refactors Editor::PasteRectangular(). The essential changes are
1. that the rectangular selection on the clipboard is parsed into separate lines before any of it is inserted into the document. Formerly the parsing and insertion were done in parallel.
2. that the selection isn't modified until the paste has succeeded.

This has the happy side-effect of slightly simplifying two edge cases: removing blank lines at the end of the data and handling line endings when the end of pasted rectangle extends beyond the bottom of the document.

This refactoring will make it possible to check that all data can be pasted before actually modifying the document when change veto is implemented.

It's a bit harder to test this function than ConvertLineEnds() in the previous patch but I'm happy that it is working correctly.

[ian.g...@gmail.com]

Finally, here's the main patch that implements modification veto. The other two patches must be applied first.

The implementation is essentially the same as my earlier submission, but I have tidied it up to reduce the size and complexity of the patch. The main change is to use early return, rather than nested blocks, to avoid too many changes that are purely whitespace.

I have also removed SCI_SETVETOMODIFICATIONFLAGS and the corresponding partial modification flag. These could be put back in again very easily if there is demand.

Finally, there are changes in win32, but no corresponding changes in qt or gtk. Someone with some knowledge of these platforms will have to implement these.

[Neil Hodgson]

Ian Goldby:

> The reason for reversing the direction of the loop is so that when the number of characters in the line ending sequence changes, it does not affect the position of changes yet to be made as the loop progresses.

The changed code does have an effect on some line associated data. When the reverse version is converting to CR and it encounters a bare LF, it first deletes the LF which removes the line from associated data such as bookmarks which may move the bookmark. Then the CR is inserted but the bookmark doesn’t move back. This could be improved by inverting the order so that the line is always there, changing that hunk to

@@ -1392,8 +1391,8 @@
if (eolModeSet == SC_EOL_CRLF) {
- pos += InsertString(pos + 1, "\n", 1); // Insert LF
- } else if (eolModeSet == SC_EOL_LF) {
- pos += InsertString(pos, "\n", 1); // Insert LF
- DeleteChars(pos, 1); // Delete CR
- pos--;
+ InsertString(pos, "\r", 1); // Insert CR
+ } else if (eolModeSet == SC_EOL_CR) {
+ if (InsertString(pos, "\r", 1)) { // Insert CR
+ DeleteChars(pos+1, 1); // Delete LF
+ }
}
}

The inverse operation CR→LF should have a similar change.

The current code isn’t perfect with line associated data either, because it treats each current line end in isolation so it doesn’t handle some cases where lines are temporarily merged.

Neil

[ian.g...@gmail.com]

Neil,

If you are happy with those changes then I'd certainly be happy to see this patch committed. It would be wise to wait though until you've decided whether to accept the modification veto patch itself, as the former is of no real use without the latter.

Ian

[Neil Hodgson]

Ian Goldby:

> If you are happy with those changes then I'd certainly be happy to see this patch committed.

My change to line end conversion didn’t handle the possibility of InsertString(“\r”) returning a value other than 0 or 1. I can’t think of a reasonable case for doing this (padding out line ends?) but it should probably be more like const int width=Insert(“\r”); DeleteChars(pos+width, 1);

Part of my reason for the reply on this patch was to make sure you were aware of the line-associated state and the need to maintain this as far as possible since the post contained “Applying this patch will have no effect on the operation of Scintilla” when it would be more accurate to say that it has no effect on the document content.

Neil

[Neil Hodgson]

Ian Goldby:

> It refactors Editor::PasteRectangular().

The line ‘*ptr++’ does nothing with the dereferenced value. The “lines.rbegin()->size()’ expression can fail if the data is empty (len == 0).

64-bit builds produce several size_t to int conversion warnings.

Backwards loops to 0 with an unsigned counter are easy to get wrong. If not now, then when changed by maintenance. Placing the mutation in the test clause adds to the fragility.

The results are not always exactly the same as before with proportional fonts although the new results are mostly better.

The anchor fields in the ranges are never used so I expect it would be clearer to use a vector of SelectionPosition.


I’m not seeing how the ‘veto the complete action’ concept works with pastes: the deletions occur in ScintillaWin::Paste which can be vetoed but if they succeed and then the insertion is vetoed, the deletions aren’t reversed.

At a more general level, the veto mechanism seems to have an implicit model where allowing a deletion at a position implies allowing insertion at that position. However, this isn’t really reflected in the API so applications may try to implement features counter to this model (delete-only or insert-only regions for example) but which produce different results depending on which action the user performs.

Neil

[ian.g...@gmail.com]

On Saturday, 27 September 2014 00:58:23 UTC+1, Neil Hodgson wrote:

   Part of my reason for the reply on this patch was to make sure you were aware of the line-associated state and the need to maintain this as far as possible since the post contained “Applying this patch will have no effect on the operation of Scintilla” when it would be more accurate to say that it has no effect on the document content.

I had indeed completely missed that. Thank you for the explanation.

[ian.g...@gmail.com]

On Saturday, 27 September 2014 01:22:11 UTC+1, Neil Hodgson wrote:

   The line ‘*ptr++’ does nothing with the dereferenced value. The “lines.rbegin()->size()’ expression can fail if the data is empty (len == 0).

My mistake. Obviously the deference operator is spurious.
Apparently before lines.rbegin()->size() == 0 it needs a pre-check that lines.rbegin() != lines.rend(). Another one I'd missed.
 

   64-bit builds produce several size_t to int conversion warnings.

Noted.
 

   Backwards loops to 0 with an unsigned counter are easy to get wrong. If not now, then when changed by maintenance. Placing the mutation in the test clause adds to the fragility.

I'm so used to doing it this way that I can't right now think how you would avoid the mutation in the test clause. (I refuse to use a signed integer for an unsigned quantity.)
 

   The results are not always exactly the same as before with proportional fonts although the new results are mostly better.

   The anchor fields in the ranges are never used so I expect it would be clearer to use a vector of SelectionPosition.

That shouldn't be a problem. I'm not overly familiar with Scintilla's internal data types.
 

   I’m not seeing how the ‘veto the complete action’ concept works with pastes: the deletions occur in ScintillaWin::Paste which can be vetoed but if they succeed and then the insertion is vetoed, the deletions aren’t reversed.

You are right – there is a bug if you paste a rectangular selection over a shorter rectangular selection and the bottom of the pasted rectangle would be vetoed. In this case as you say the original selection is deleted, but the paste gets vetoed. I'm afraid I can't see a way to resolve this that isn't insanely complicated.

As far as I can see, none of the other paths through ScintillaWin::Paste() suffer from this problem, because it's only the rectangular paste that can try to modify the document outside of the original selection.

(This is probably the area that would benefit most from switching to a transactional model for document modifications.)
 

   At a more general level, the veto mechanism seems to have an implicit model where allowing a deletion at a position implies allowing insertion at that position.

Correct. That was the intention.
 

However, this isn’t really reflected in the API so applications may try to implement features counter to this model (delete-only or insert-only regions for example) but which produce different results depending on which action the user performs.

Can we avoid this by careful documentation? I know developers will ultimately do what they want to do, but we can at least warn that the callback must provide consistent results during a particular operation (much as a search comparator must do). In particular, the callback must not try to second-guess whether the modification is an insertion or deletion by looking at the length parameter, because Scintilla assumes that if a particular range can be deleted then an insertion can also be made anywhere within that range.

Allowing separate rules for deletions and insertions opens a whole new can of worms and isn't a route I'd like to go down. I'd be open to changing to the API to make it impossible to distinguish deletions from insertions in the callback, but I can't see how this could be achieved.

[Neil Hodgson]

Ian Goldby:

   At a more general level, the veto mechanism seems to have an implicit model where allowing a deletion at a position implies allowing insertion at that position.

Correct. That was the intention.

In particular, the callback must not try to second-guess whether the modification is an insertion or deletion by looking at the length parameter, because Scintilla assumes that if a particular range can be deleted then an insertion can also be made anywhere within that range.

   Does this also imply that inserting immediately before a protected range is not permitted? This could be used, for example, with a calculation document where queries are entered on the left and the results inserted as read-only blocks to the right.

   Neil

[ian.g...@gmail.com]

   Does this also imply that inserting immediately before a protected range is not permitted?

In my application, non-modifiable ranges are always complete lines. If the start or end of the range passed to the SCN_MODIFYCHECK handler falls on a non-modifiable line then the modification is vetoed. The natural consequence is that you can insert a new line at the end of the last modifiable line, but you can't add a new one at the start of the first non-modifiable line. You cannot forward delete from either position, which is intuitively consistent with the above.

In the general case, it depends on how you write the handler. Imagine the following document, where m is modifiable, and V is not:

  content    m m V V V m m
  position  0 1 2 3 4 5 6 7
 
The following handler would allow insertions at positions 2 and 5 whilst preventing deletion of any of the text between 2 and 5:

  end = start + length
  if (start > 2 and start < 5) or (end > 2 and end < 5)
    SCI_VETOMODIFICATION
   
This handler implementation does allow the entire non-modifiable range to be deleted as a single operation but you could easily amend the handler to prevent this. Of course that would make SCI_CLEARALL always fail.
   
So I think the answer to your question is 'not necessarily'.

(Of course, you would never code fixed ranges like this because the non-modifiable ranges need to move with the document content. Two easy ways to do this are with line markers as I do, or with indicators. But it could also be tracked by the application. You could even make specific character sequences non-modifiable.)
 

[Neil Hodgson]

[ I haven’t been able to achieve much clarity on this subject ]

Ian Goldby (and >Me):

Can we avoid this by careful documentation? I know developers will ultimately do what they want to do, but we can at least warn that the callback must provide consistent results during a particular operation (much as a search comparator must do).

   What is the model then? If contiguous ranges a..b and b..c can be individually deleted, must the union of the ranges a..c be deletable? Does the converse (range division) hold?

Allowing separate rules for deletions and insertions opens a whole new can of worms and isn't a route I'd like to go down. I'd be open to changing to the API to make it impossible to distinguish deletions from insertions in the callback, but I can't see how this could be achieved.

   If the model is simple enough (such as dividing the document into protected and deletable ranges) then it can be implemented as state without callbacks or with an update-protection callback. Its going to be easier to explain what can be done in terms of a data structure that specifies a particular shape that is protected.

   I’m not seeing how the ‘veto the complete action’ concept works with pastes: the deletions occur in ScintillaWin::Paste which can be vetoed but if they succeed and then the insertion is vetoed, the deletions aren’t reversed.

You are right – there is a bug if you paste a rectangular selection over a shorter rectangular selection and the bottom of the pasted rectangle would be vetoed. In this case as you say the original selection is deleted, but the paste gets vetoed. I'm afraid I can't see a way to resolve this that isn't insanely complicated.

   Its this sort of case, and doubtless more complex future operations, that make me want a generic transaction system. There may be bugs in the veto system and they will be hard to explain. 

   However, then there is the deletion of associated data (such as markers) that may occur when document content is deleted and there would have to be a new mechanism to save/restore this since it isn't handled by the existing undo system as associated data may or may not be something that should be undoable (think: bookmarking several lines then undoing some distance and checking each of the bookmarked lines in the earlier context). Space-efficient storage of the pre-transaction state would be quite fiddly so would probably be done as an expensive clone.

   Neil

[ian.g...@gmail.com]

Hi Neil,

I think the lack of clarity is because we seem to have different aims for this patch. I'll try my best to answer your questions though. (My biggest problem now though is that I really have no more time to work on this patch. I would be able to make one or two very minor tweaks but that's about the limit.)

> What is the model then? If contiguous ranges a..b and b..c can be individually deleted, must the union of the ranges a..c be deletable? Does the converse (range division) hold?

I defined the callback API first, and the model emerges from that. That's *why* I prefer a callback rather than having Scintilla maintain the state of what is modifiable and what is vetoed. So the model is whatever the application writer implements in the callback. Not all models will make sense though:

> If contiguous ranges a..b and b..c can be individually deleted, must the union of the ranges a..c be deletable?

No, The API doesn't enforce this. If you were feeling perverse, you could write the following callback where the union is not deletable (but anything else is):

  end = start + length

  if (start = a and end = c)
    SCI_VETOMODIFICATION

However, this would almost certainly cause odd results and some compound actions could behave strangely.

I don't have a problem with this - one could implement a callback that will make Scintilla behave oddly, but such a model would be outside the scope of what I'm trying to achieve with this patch. It's up to the application writer to implement something that will make sense to the user.

> Does the converse (range division) hold?

No, for the same reason.

> If the model is simple enough (such as dividing the document into protected and deletable ranges) then it can be implemented as state without callbacks or with an update-protection callback. Its going to be easier to explain what can be done in terms of a data structure that specifies a particular shape that is protected.

In other words, SCI_STYLESETCHANGEABLE with enhancements to allow selections within in a protected region, and with pre-modification checking to ensure that compound actions are not carried out only partially. I would go for that solution; SCI_STYLESETCHANGEABLE is what I tried initially before I resorted to writing this patch. The only real disadvantage I can see is that in cases like my own it doubles the number of styles needed because every visual style that can occur in a writeable region can also occur in a protected region, but I could live with that. I'm really sorry though - I can't offer the time to implement this now. I wish I could, but I'm working in a commercial environment.

Either way though, I think we both agree that to do the job properly we need a generic transaction system.

> there is the deletion of associated data (such as markers) that may occur when document content is deleted and there would have to be a new mechanism to save/restore this since it isn't handled by the existing undo system

I think the cleanest design would queue up textual modifications in a separate buffer and then apply them all at once - rather than making piecemeal modifications to the document and then trying to undo them. That way associated data would not be affected. There would have to be an interface that provided a view of the document as it would look with all the modifications so far (should they be accepted) for the use of existing functions that read the document in parallel with modifying it (such as line-ending conversion).

Shall we lay this patch to one side for now? It sounds like this is the inevitable conclusion. I will continue to use it in my application (I have no choice). Perhaps at a later date I or someone else will be able to design and implement a generic transaction system and then beef up SCI_STYLESETCHANGEABLE?

[Neil Hodgson]

Ian Goldby:

> I defined the callback API first, and the model emerges from that. That's *why* I prefer a callback rather than having Scintilla maintain the state of what is modifiable and what is vetoed. So the model is whatever the application writer implements in the callback.

By model, I didn’t mean what the callback can express but what it can achieve given the patterns in which it is called. As mentioned earlier the callback can express that only deletion and not insertion is possible at some position but that doesn’t matter because it won’t be asked in all cases. As another example, can an application allow any deletion of whole elements (and no partial elements) such as whole statements, whole identifiers, or whole combining characters? Its difficult to tell although I can see the potential for problems with features like overtype and rectangular selections.

> SCI_STYLESETCHANGEABLE is what I tried initially before I resorted to writing this patch. The only real disadvantage I can see is that in cases like my own it doubles the number of styles needed because every visual style that can occur in a writeable region can also occur in a protected region, but I could live with that.

It would be better to use an indicator for protected areas since they are relatively space efficient and avoid doubling the number of styles.

> Shall we lay this patch to one side for now?

I think that is best. The patches are published so others can experiment with them to further develop the concepts.

Neil