Discussion:
Rectangle selections and RefreshStyleData bug
(too old to reply)
John Ehresman
2014-06-26 23:20:27 UTC
Permalink
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
--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-interest+***@googlegroups.com.
To post to this group, send email to scintilla-***@googlegroups.com.
Visit this group at http://groups.google.com/group/scintilla-interest.
For more options, visit https://groups.google.com/d/optout.
Neil Hodgson
2014-06-28 03:55:29 UTC
Permalink
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
--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-interest+***@googlegroups.com.
To post to this group, send email to scintilla-***@googlegroups.com.
Visit this group at http://groups.google.com/group/scintilla-interest.
For more options, visit https://groups.google.com/d/optout.
John Ehresman
2014-06-30 14:59:35 UTC
Permalink
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
--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-interest+***@googlegroups.com.
To post to this group, send email to scintilla-***@googlegroups.com.
Visit this group at http://groups.google.com/group/scintilla-interest.
For more options, visit https://groups.google.com/d/optout.
Neil Hodgson
2014-07-01 02:16:03 UTC
Permalink
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
--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-interest+***@googlegroups.com.
To post to this group, send email to scintilla-***@googlegroups.com.
Visit this group at http://groups.google.com/group/scintilla-interest.
For more options, visit https://groups.google.com/d/optout.
Neil Hodgson
2014-07-07 13:55:29 UTC
Permalink
Committed as https://sourceforge.net/p/scintilla/code/ci/50cf5adf96261c38b4269ebeb3c39981a4493858/

Neil
--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-interest+***@googlegroups.com.
To post to this group, send email to scintilla-***@googlegroups.com.
Visit this group at http://groups.google.com/group/scintilla-interest.
For more options, visit https://groups.google.com/d/optout.
Continue reading on narkive:
Loading...