Rectangle selections and RefreshStyleData bug

[John Ehresman]

Hi,

When deleting a rectangular selection with the backspace key, I'm seeing
too many characters on the 2nd+ lines deleted. The cause seems to be
RefreshStyleData being called after the characters on the 1st line are
deleted and calling SetRectangularRange which resets the selection based
on rangeRectangular.

The following patch to Selection::MovePositions seems to fix it, though
I don't know if it's the right thing to do:

--- a/src/Selection.cxx Mon Jun 23 16:50:46 2014 -0600
+++ b/src/Selection.cxx Thu Jun 26 19:14:41 2014 -0400
@@ -278,6 +278,10 @@
ranges[i].caret.MoveForInsertDelete(insertion, startChange, length);
ranges[i].anchor.MoveForInsertDelete(insertion, startChange, length);
}
+ if (selType == selRectangle) {
+ rangeRectangular.caret.MoveForInsertDelete(insertion,
startChange, length);
+ rangeRectangular.anchor.MoveForInsertDelete(insertion,
startChange, length);
+ }
}

void Selection::TrimSelection(SelectionRange range) {

Thanks,

John

[Neil Hodgson]

Hi John,

When deleting a rectangular selection with the backspace key, I'm seeing too many characters on the 2nd+ lines deleted.

I haven't seen this in SciTE.

The cause seems to be RefreshStyleData being called after the characters on the 1st line are deleted and calling SetRectangularRange which resets the selection based on rangeRectangular.

RefreshStyleData should only perform changes when stylesValid is false. For most applications the style settings should remain relatively constant. Perhaps your code is changing a style property when it doesn't have to. This could be because of resetting the styles - most style setting code in Scintilla does not bother to see whether the new setting is different so will set stylesValid false without need.

It may be worthwhile calling RefreshStyleData at the top of DelCharBack to ensure that style settings are consistent through the loop.

--- a/src/Selection.cxx Mon Jun 23 16:50:46 2014 -0600
+++ b/src/Selection.cxx Thu Jun 26 19:14:41 2014 -0400
@@ -278,6 +278,10 @@
ranges[i].caret.MoveForInsertDelete(insertion, startChange, length);
ranges[i].anchor.MoveForInsertDelete(insertion, startChange, length);
}
+ if (selType == selRectangle) {
+ rangeRectangular.caret.MoveForInsertDelete(insertion, startChange, length);
+ rangeRectangular.anchor.MoveForInsertDelete(insertion, startChange, length);
+ }
}

That appears to be a reasonable change.

Neil

[John Ehresman]

Hi Neil,

Post by Neil Hodgson

The cause seems to be RefreshStyleData being called after the characters on the 1st line are deleted and calling SetRectangularRange which resets the selection based on rangeRectangular.

RefreshStyleData should only perform changes when stylesValid is false. For most applications the style settings should remain relatively constant. Perhaps your code is changing a style property when it doesn't have to. This could be because of resetting the styles - most style setting code in Scintilla does not bother to see whether the new setting is different so will set stylesValid false without need.

It turns out that we have code that invalidates the styles via calls to
set a margin up with the same values (type, mask, etc) that they were
already set to. We can eliminate these calls and avoid this issue.
That said, the change to Selection::MovePositions is probably
worthwhile. I agree it can wait until after 3.4.4.

Thanks,

John

[Neil Hodgson]

It turns out that we have code that invalidates the styles via calls to set a margin up with the same values (type, mask, etc) that they were already set to. We can eliminate these calls and avoid this issue.

That may also benefit performance. Scintilla should really do a better job here by checking whether a property is really changed before invalidating styles. That means making a large number of small changes with a potential for a mistake.

Neil

[Neil Hodgson]

Committed as https://sourceforge.net/p/scintilla/code/ci/50cf5adf96261c38b4269ebeb3c39981a4493858/

Neil