"StartInsertAbbreviation" cursor positioning bug

[Neo]

I have a question about "StartInsertAbbreviation" function of the
SciTE, I think there is a bug.
I can not understand the meaning of the code below:

================================================
SciTEBase.cxx (2149):
// set the caret to the desired position
if (double_pipe) {
sel_length = 0;
} else if (!at_start && sel_length == 0) {
sel_start += static_cast<int>(expbuflen);
}
wEditor.Call(SCI_SETSEL, sel_start, sel_start + sel_length);
================================================

Look at this fragment:
... if (!at_start && sel_length == 0) {
sel_start += static_cast<int>(expbuflen);
}
imho, meaning of this fragment is to move the cursor position if there
is only one "|" in the currently inserting text ("!double_pipe" AND "!
at_start") and no text is
selected ("sel_length == 0"), but why?

maybe it should be like this:
... if (at_start && sel_length == 0) {
sel_start -= static_cast<int>(expbuflen);
}
this code returns back the cursor position if there is no "|" and no
selections

FOR TESTING:
1. Put this line below to the SciTEUser.properties file of the SciTE:
abbreviations.*.txt=$(SciteDefaultHome)\txt.abbrev

2. Create "txt.abbrev" file in the SciTE home directory with content
like this:
bb=test(|);

3. Then create a new file in the editor named "sample.txt" and write
down this:
aa
bb|
aa

4. Place cursor at position marked as "|"

5. After this press "Ctrl+Shift+R" and select "bb" from the list and
you can see how cursor jumped out of the brackets

--
Neo

[mozers]

Sunday, March 13, 2011, 1:16:07 PM, Neo wrote:
> I have a question about "StartInsertAbbreviation" function of the
> SciTE, I think there is a bug.

InsertAbbreviation does not work properly, unlike the function ExpandAbbreviation.
InsertAbbreviation ignores the selection and incorrect positioning the cursor.
InsertAbbreviation invented not Neil but temporary author.
This author is gone. Nobody wants to be responsible for others' mistakes.
I was waiting for that fix will be included in the 2.25 ...
But probably this bug SciTE will last forever :(

--
mozers
<http://scite.net.ru>

[Neil Hodgson]

mozers:

> InsertAbbreviation invented not Neil but temporary author.

It was in change set 1359 with changes in 1892 (and some more
smaller) from Jakub Vrána with this explanation:

3. Sometimes, it is useful to have a chance to insert abbreviation in
a different way than from editor. E.g. if I have some text and I want
to insert abbreviation just after this text. Here is an example:

abbreviation: b=<B>|</B>
I want to write some part<B>ially</B> bolded text.

This is quite stupid example, but I use abbreviations also for e.g.
escaping from string ("'+|+'" in JS) and I needed it couple of times.

So I have added dialog by which you can insert abbreviation. This
dialog also works with selections - if something is selected,
abbreviation containing caret mark '|' is surrounded around the
selection. If there are two caret marks in the abbreviation, the
second one is used for selection and the caret is placed at the first
one. This is extremely useful for HTML tags (e.g. <a href="|">|</a>).

I bind this dialog to Ctrl+Shift+R. A little reasonable should be
Ctrl+Shift+B, but users already use it for Box comment.

Neil

[mozers]

Thursday, March 17, 2011, 11:24:37 AM, Neil wrote:

> It was in change set 1359 with changes in 1892 (and some more

> smaller) from Jakub Vr�na

This author has implemented a useful idea.
I like it.
But Neo was talking about a particular bug with cursor positioning.
Neil, you accept the fix?

--
mozers
<http://scite.net.ru>

[Neil Hodgson]

StartInsertAbbreviation is complex and its hard to work out all the
consequences of a change. The proposed patch moves the selection back
to the beginning when there is no '|' in the abbreviation.

The expected behaviour is for the selection to be at the end of the
inserted abbreviation when there is no '|'. This matches Expand
Abbreviation.

Neil

[Neo]

Sorry, my subscription doesn't work.

I did not propose, but have just assumed. In order to keep cursor at
the end, that part of the code fragment need to be removed since it
makes no sense.

Here is the patch:
http://dl.dropbox.com/u/23899768/sia_patch.diff

[Neil Hodgson]

Neo:

> I did not propose, but have just assumed.

Yes, then mozers proposed: "Neil, you accept the fix?".

> In order to keep cursor at
> the end, that part of the code fragment need to be removed since it
> makes no sense.

I can't make sense of it either.

Now that 2.25 is released I have removed that code. Anyone
interested in testing this feature can download from Hg so any
regressions can be found before 2.26.

Neil