bug#5805: Returned mail: Cannot send message within 3 days

bug#5805: Returned mail: Cannot send message within 3 days

[Maguire, Andrew (GE Infra, Energy)]

Try again...

-----Original Message-----
From: Mail Delivery Subsystem [mailto:Mailer-Daemon@Smallworld.co.uk] 
Sent: 29 March 2010 13:07
To: andrewma@Smallworld.co.uk
Subject: Returned mail: Cannot send message within 3 days

The original message was received at Fri, 26 Mar 2010 12:06:32 +0100
(BST)
from shark [193.128.11.12]

   ----- The following addresses had permanent fatal errors -----
<bug-gnu-emacs@gnu.org>

   ----- Transcript of session follows -----
... while talking to eggs.gnu.org.:
>>> RCPT To:<bug-gnu-emacs@gnu.org>
<<< 451 Could not complete sender verify callout
<bug-gnu-emacs@gnu.org>... Deferred: 451 Could not complete sender
verify callout
Message could not be delivered for 3 days
Message will be deleted from queue

   ----- Original message follows -----

Return-Path: <andrewma@Smallworld.co.uk>
Received: from MAGUIRAN-CBG.smallworld.co.uk by
ultimate.Smallworld.co.uk (8.8.8+Sun/SWW-2.8-Gateway)
	id MAA03728; Fri, 26 Mar 2010 12:06:32 GMT
Date: Fri, 26 Mar 2010 12:25:49 +0000
Message-Id:
<x768w9fuv5e.fsf@MAGUIRAN-CBG.i-did-not-set--mail-host-address--so-tickl
e-me>
From: andrew.maguire@ge.com
To: bug-gnu-emacs@gnu.org
Subject: 23.1; abbrev-insert does not protext itself with save-excursion
Reply-to: andrew.maguire@ge.com

I believe there is a regression/change of behaviour with the lisp
implementation
of the abbrev package compared with earlier Emacsen.

In our code we use abbrev to dynamically expand certain
keywords. However, the new lisp implementation is more
eager in finding things to check, ie. it is able to look back past
non-word characters to see if an abbrev is to be found.
In this situation it is essential to do save-excursion.

Ideally, either insert-abbrev should do a save-excursion itself or all
calls to it should.
In our code, expand-abbrev is call which internally calls abbrev-insert.

Alternatively, is it the recommendation for user code to surround all
"abbrev" calls with save-excursion now?

Thanks for you advice,
Andrwe Maguire


In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-30 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: ENG
  value of $XMODIFIERS: nil
  locale-coding-system: cp1252
  default-enable-multibyte-characters: t

Major mode: Gis

Minor modes in effect:
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t
  abbrev-mode: t

Recent input:
! C-c C-c C-x C-f c b . e l <return> C-s M-p M-p M-p 
M-p M-p M-p M-p M-p M-p M-p C-g C-x 2 C-x b <up> <return> 
<prior> <prior> <prior> <prior> <prior> <prior> <prior> 
<prior> <down-mouse-1> <mouse-1> <down-mouse-1> <mouse-1> 
C-s C-w C-w C-w C-w C-w <help-echo> <down-mouse-1> 
<mouse-1> C-s C-s M-x <up> <return> SPC C-x v v SPC 
<down> <left> SPC C-k <S-insert> <down> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <S-insert> <left> 
<left> <backspace> 2 <up> <up> <up> C-a <C-right> <C-left> 
<left> C-x o <up> <C-left> <C-left> <left> M-x <up> 
<return> <help-echo> <down-mouse-1> <mouse-1> <double-down-mouse-1> 
<double-mouse-1> <triple-down-mouse-1> <triple-mouse-1> 
<help-echo> <down-mouse-2> <mouse-2> C-k C-k <tab> 
<help-echo> <down-mouse-1> <mouse-1> <wheel-down> <help-echo> 
<help-echo> <down-mouse-1> <mouse-1> <double-down-mouse-1> 
<double-mouse-1> <help-echo> <down-mouse-2> <mouse-2> 
<down-mouse-1> <mouse-1> <double-down-mouse-1> <double-mouse-1> 
C-w C-x C-s <wheel-up> <wheel-up> <wheel-up> <wheel-up> 
<double-wheel-up> <wheel-down> <double-wheel-down> 
<wheel-down> <wheel-down> C-x v v C-x b <return> C-M-b 
<prior> <prior> <up> <up> <up> <up> C-g C-x o <prior> 
<prior> <prior> <prior> M-x <up> <return> C-x v v M-p 
<up> <up> <C-right> <right> <right> C-k c b <M-prior> 
<M-prior> - m a g i k - e d i f f - m e t h o d s : 
SPC r e t a i n SPC w i n d o w SPC c o n f i g u r 
a t i o n SPC i f SPC m e t h o d SPC n o t SPC f o 
u n d C-c C-c <next> <down-mouse-1> <mouse-1> C-x k 
<return> <home> <C-home> <down> <down> <down> M-x e 
r a s e <tab> <return> SPC <f4> b C-x k <return> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<menu-bar> <help-menu> <send-emacs-bug-report>

Recent messages:
Press C-c C-c when you are done editing.
Enter a change comment.  Type C-c C-c when done
Quit
Mark set [2 times]
Press C-c C-c when you are done editing.
Enter a change comment.  Type C-c C-c when done
Comment 1
Checking in
u:/tools/general/emacs/smallworld_lisp/smallworld/swlisp/cb.el...done
Mark set
Type y, n, ! or SPC (the space bar): 

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Chong Yidong]

> In our code we use abbrev to dynamically expand certain
> keywords. However, the new lisp implementation is more eager in
> finding things to check, ie. it is able to look back past non-word
> characters to see if an abbrev is to be found.  In this situation it
> is essential to do save-excursion.
>
> Ideally, either insert-abbrev should do a save-excursion itself or all
> calls to it should.  In our code, expand-abbrev is call which
> internally calls abbrev-insert.
>
> Alternatively, is it the recommendation for user code to surround all
> "abbrev" calls with save-excursion now?

Do you have a recipe for reproducing a bug?  By default, the abbrev code
is not supposed to change point, so there should be no advantage adding
a save-excursion.  I don't see why we should constrain the ability of
user-defined functions in `abbrev-expand-functions' to change point, if
they want to.

Or do you mean save-match-data?

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Maguire, Andrew (GE Infra, Energy)]

Create a global abbrev, abbrv => abbrev

Type the following, ^ indicating point location.

  abbrv ()
          ^
Then press C-x ' to expand the abbrev on the line.
  abbrev ()
        ^
Observe that point ^ is now before the ()s.
In the C version of abbrev expansion in all earlier Emacsen, point is
not affected by expansion,
i.e. it would be left after the ()s which is where the user pressed C-x
'

My current fix is 
;; Emacs 23 has a lisp implementation for abbrevs.
(if (fboundp 'abbrev-insert)
    (defadvice abbrev-insert (around
save-excursion-around-advice-insertion activate)
      "Lisp implementation of advice insertion does not save point
location afterwards.
When looking back over non-word characters to find a word that may be an
abbreviation
if it finds something to replace, it does not save its position."
      (save-excursion
	ad-do-it)))

Thanks,
Andrew


-----Original Message-----
From: Chong Yidong [mailto:cyd@stupidchicken.com] 
Sent: 10 April 2010 17:04
To: Maguire, Andrew (GE Infra, Energy)
Cc: 5805@debbugs.gnu.org
Subject: Re: 23.1; abbrev-insert does not protext itself with
save-excursion

> In our code we use abbrev to dynamically expand certain
> keywords. However, the new lisp implementation is more eager in
> finding things to check, ie. it is able to look back past non-word
> characters to see if an abbrev is to be found.  In this situation it
> is essential to do save-excursion.
>
> Ideally, either insert-abbrev should do a save-excursion itself or all
> calls to it should.  In our code, expand-abbrev is call which
> internally calls abbrev-insert.
>
> Alternatively, is it the recommendation for user code to surround all
> "abbrev" calls with save-excursion now?

Do you have a recipe for reproducing a bug?  By default, the abbrev code
is not supposed to change point, so there should be no advantage adding
a save-excursion.  I don't see why we should constrain the ability of
user-defined functions in `abbrev-expand-functions' to change point, if
they want to.

Or do you mean save-match-data?

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Stefan Monnier]

> Create a global abbrev, abbrv => abbrev
> Type the following, ^ indicating point location.

>   abbrv ()
>           ^
> Then press C-x ' to expand the abbrev on the line.
>   abbrev ()
>         ^
> Observe that point ^ is now before the ()s.

Right.  The question now is: why is that a problem?

I ask because for some abbrevs, moving point isa feature, e.g. an abbrev
that uses skeletons to expand

    begi
        ^
into

    \begin{itemize}  \end{itemize}
                    ^

so maybe the problem is that your code makes unwarranted assumptions
about what abbrevs can do, or maybe your code knows that it won't
encounter such abbreviations or that it wouldn't care about their
point-placement feature, so it could/should use save-excursion.

> ;; Emacs 23 has a lisp implementation for abbrevs.

BTW, the problem is not that the implementation is in Lisp, but that it
behaves differently.


        Stefan

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Maguire, Andrew (GE Infra, Energy)]

I do appreciate the need for some abbrevs to move point and that I am
reporting a perceived change in behaviour.
However, the question is whether the change in behaviour is deliberate
or not.

If a user wishes to create an abbrev that requires point to move
presumably they have to create the abbrev in a certain way.
The example you give below would still require user code to move point
to between the LaTeX statements.
i.e If I created a simple global abbrev to expand "begi" point would be
left after the \end{itemize}^

So how should I fix my code that uses expand-abbrev to work in Emacs 23?
It currently works as is in Emacs 20, 21 and 22.

Thanks,
Andrew
-----Original Message-----
From: Stefan Monnier [mailto:monnier@iro.umontreal.ca] 
Sent: 10 April 2010 20:10
To: Maguire, Andrew (GE Infra, Energy)
Cc: Chong Yidong; 5805@debbugs.gnu.org
Subject: Re: bug#5805: 23.1; abbrev-insert does not protext itself with
save-excursion

> Create a global abbrev, abbrv => abbrev
> Type the following, ^ indicating point location.

>   abbrv ()
>           ^
> Then press C-x ' to expand the abbrev on the line.
>   abbrev ()
>         ^
> Observe that point ^ is now before the ()s.

Right.  The question now is: why is that a problem?

I ask because for some abbrevs, moving point isa feature, e.g. an abbrev
that uses skeletons to expand

    begi
        ^
into

    \begin{itemize}  \end{itemize}
                    ^

so maybe the problem is that your code makes unwarranted assumptions
about what abbrevs can do, or maybe your code knows that it won't
encounter such abbreviations or that it wouldn't care about their
point-placement feature, so it could/should use save-excursion.

> ;; Emacs 23 has a lisp implementation for abbrevs.

BTW, the problem is not that the implementation is in Lisp, but that it
behaves differently.


        Stefan

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Stefan Monnier]

> I do appreciate the need for some abbrevs to move point and that I am
> reporting a perceived change in behaviour.
> However, the question is whether the change in behaviour is deliberate
> or not.

The change was not deliberate, no.

> If a user wishes to create an abbrev that requires point to move
> presumably they have to create the abbrev in a certain way.
> The example you give below would still require user code to move point
> to between the LaTeX statements.
> i.e If I created a simple global abbrev to expand "begi" point would be
> left after the \end{itemize}^

Yes, the abbrev would need to be defined differently, but the
point-movement would be done by the abbrev itself, i.e. the caller would
still just call expand-abbrev.

> So how should I fix my code that uses expand-abbrev to work in Emacs 23?
> It currently works as is in Emacs 20, 21 and 22.

I think you need to add a save-excursion around the call to
expand-abbrev to make it clear that you don't want this call to
move point.
This will also save you in the case where the user has setup an abbrev
like "begi", which is a case that could also happen in previous Emacsen.

But that really depends on what behavior you expect in the case where
your code encounters a "begi"-like abbrev.


        Stefan


> Thanks,
> Andrew
> -----Original Message-----
> From: Stefan Monnier [mailto:monnier@iro.umontreal.ca] 
> Sent: 10 April 2010 20:10
> To: Maguire, Andrew (GE Infra, Energy)
> Cc: Chong Yidong; 5805@debbugs.gnu.org
> Subject: Re: bug#5805: 23.1; abbrev-insert does not protext itself with
> save-excursion

>> Create a global abbrev, abbrv => abbrev
>> Type the following, ^ indicating point location.

>> abbrv ()
>> ^
>> Then press C-x ' to expand the abbrev on the line.
>> abbrev ()
>> ^
>> Observe that point ^ is now before the ()s.

> Right.  The question now is: why is that a problem?

> I ask because for some abbrevs, moving point isa feature, e.g. an abbrev
> that uses skeletons to expand

>     begi
>         ^
> into

>     \begin{itemize}  \end{itemize}
>                     ^

> so maybe the problem is that your code makes unwarranted assumptions
> about what abbrevs can do, or maybe your code knows that it won't
> encounter such abbreviations or that it wouldn't care about their
> point-placement feature, so it could/should use save-excursion.

>> ;; Emacs 23 has a lisp implementation for abbrevs.

> BTW, the problem is not that the implementation is in Lisp, but that it
> behaves differently.


>         Stefan

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Maguire, Andrew (GE Infra, Energy)]

The workaround I have locally is using advice to place a save-excursion
around the non-interactive function abbrev-insert.
Please check to see whether abbrev-insert should be allowed to not
preserve point.

;; Emacs 23 has a lisp implementation for abbrevs.
(if (fboundp 'abbrev-insert)
    (defadvice abbrev-insert (around
save-excursion-around-advice-insertion activate)
      "Lisp implementation of advice insertion does not save point
location afterwards."
      (save-excursion
	ad-do-it)))

Thanks for your prompt replies :-)
Andrew

-----Original Message-----
From: Stefan Monnier [mailto:monnier@iro.umontreal.ca] 
Sent: 12 April 2010 14:31
To: Maguire, Andrew (GE Infra, Energy)
Cc: Chong Yidong; 5805@debbugs.gnu.org
Subject: Re: bug#5805: 23.1; abbrev-insert does not protext itself with
save-excursion

> I do appreciate the need for some abbrevs to move point and that I am
> reporting a perceived change in behaviour.
> However, the question is whether the change in behaviour is deliberate
> or not.

The change was not deliberate, no.

> If a user wishes to create an abbrev that requires point to move
> presumably they have to create the abbrev in a certain way.
> The example you give below would still require user code to move point
> to between the LaTeX statements.
> i.e If I created a simple global abbrev to expand "begi" point would
be
> left after the \end{itemize}^

Yes, the abbrev would need to be defined differently, but the
point-movement would be done by the abbrev itself, i.e. the caller would
still just call expand-abbrev.

> So how should I fix my code that uses expand-abbrev to work in Emacs
23?
> It currently works as is in Emacs 20, 21 and 22.

I think you need to add a save-excursion around the call to
expand-abbrev to make it clear that you don't want this call to
move point.
This will also save you in the case where the user has setup an abbrev
like "begi", which is a case that could also happen in previous Emacsen.

But that really depends on what behavior you expect in the case where
your code encounters a "begi"-like abbrev.


        Stefan


> Thanks,
> Andrew
> -----Original Message-----
> From: Stefan Monnier [mailto:monnier@iro.umontreal.ca] 
> Sent: 10 April 2010 20:10
> To: Maguire, Andrew (GE Infra, Energy)
> Cc: Chong Yidong; 5805@debbugs.gnu.org
> Subject: Re: bug#5805: 23.1; abbrev-insert does not protext itself
with
> save-excursion

>> Create a global abbrev, abbrv => abbrev
>> Type the following, ^ indicating point location.

>> abbrv ()
>> ^
>> Then press C-x ' to expand the abbrev on the line.
>> abbrev ()
>> ^
>> Observe that point ^ is now before the ()s.

> Right.  The question now is: why is that a problem?

> I ask because for some abbrevs, moving point isa feature, e.g. an
abbrev
> that uses skeletons to expand

>     begi
>         ^
> into

>     \begin{itemize}  \end{itemize}
>                     ^

> so maybe the problem is that your code makes unwarranted assumptions
> about what abbrevs can do, or maybe your code knows that it won't
> encounter such abbreviations or that it wouldn't care about their
> point-placement feature, so it could/should use save-excursion.

>> ;; Emacs 23 has a lisp implementation for abbrevs.

> BTW, the problem is not that the implementation is in Lisp, but that
it
> behaves differently.


>         Stefan

bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Stefan Monnier]

> The workaround I have locally is using advice to place a save-excursion
> around the non-interactive function abbrev-insert.

As explained already, some abbrevs will want to move point and that is
done within abbrev-insert.

Please give us more information about your particular use.
I.e. I can't help you much further until you answer the question I asked
at the very beginning:

>> Observe that point ^ is now before the ()s.
> Right.  The question now is: why is that a problem?


        Stefan

bug#5805: 23.3 abbrev-insert needs a limited save-excursion

[Bob Nnamtrop]

I agree with the original poster that a save-excursion is needed in
abbrev-expand and by making its scope limited (see below) it will not
interfere with abbrevs that want to change point. The reason I find it
annoying the way it is now is that some modes (e.g., idlwave) use
abbrev's extensively to change keywords (e.g. capitalize them). These
are defined without the leading "\". So if I am any number of lines
with only whitespace below a keyword defined in this way (e.g. endfor)
and run expand-abbrev then the point moves even if no visible change
takes place. This is a VERY common occurrence for me since in viper
mode changing from insert to vi mode runs expand-abbrev! Placing the
save-excursion just around the part of abbrev-insert that actually
expands the abbrev fixes this problem and does not limit abbrev's from
moving the point (idlwave does this extensively as well and I've
tested that it is not impacted). Here is the diff against the
abbrev.el in emacs 23.3 (note that whitespace not adjusted).

Thanks much,
Bob

--- 23.3/abbrev.el      2011-07-07 08:16:16.000000000 -0600
+++ 23.3.fix/abbrev.el  2011-07-07 08:43:24.000000000 -0600
@@ -713,6 +713,7 @@
     ;; If this abbrev has an expansion, delete the abbrev
     ;; and insert the expansion.
     (when (stringp (symbol-value abbrev))
+      (save-excursion
       (goto-char wordstart)
       ;; Insert at beginning so that markers at the end (e.g. point)
       ;; are preserved.
@@ -741,7 +742,7 @@
               (skip-syntax-forward "^w" (1- end))
               ;; Change just that.
               (upcase-initials-region (point) (1+ (point)))
-              (goto-char end))))))
+              (goto-char end)))))))
     ;; Now point is at the end of the expansion and the beginning is
     ;; in last-abbrev-location.
     (when (symbol-function abbrev)

bug#5805: 23.3 abbrev-insert needs a limited save-excursion

[Stefan Monnier]

tags 5805 patch
thanks

> I agree with the original poster that a save-excursion is needed in
> abbrev-expand and by making its scope limited (see below) it will not
> interfere with abbrevs that want to change point. The reason I find it
> annoying the way it is now is that some modes (e.g., idlwave) use
> abbrev's extensively to change keywords (e.g. capitalize them). These
> are defined without the leading "\". So if I am any number of lines
> with only whitespace below a keyword defined in this way (e.g. endfor)
> and run expand-abbrev then the point moves even if no visible change
> takes place. This is a VERY common occurrence for me since in viper
> mode changing from insert to vi mode runs expand-abbrev! Placing the
> save-excursion just around the part of abbrev-insert that actually
> expands the abbrev fixes this problem and does not limit abbrev's from
> moving the point (idlwave does this extensively as well and I've
> tested that it is not impacted).

Hmm... sounds like an interesting solution.  I'll take a closer look and
get back to you.  Thank you.

> Here is the diff against the
> abbrev.el in emacs 23.3 (note that whitespace not adjusted).

Highly appreciated.


        Stefan

bug#5805: 23.3 abbrev-insert needs a limited save-excursion

[Bob Nnamtrop]

On Thu, Jul 7, 2011 at 2:59 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> tags 5805 patch
> thanks
>
>> I agree with the original poster that a save-excursion is needed in
>> abbrev-expand and by making its scope limited (see below) it will not
>> interfere with abbrevs that want to change point. The reason I find it
>> annoying the way it is now is that some modes (e.g., idlwave) use
>> abbrev's extensively to change keywords (e.g. capitalize them). These
>> are defined without the leading "\". So if I am any number of lines
>> with only whitespace below a keyword defined in this way (e.g. endfor)
>> and run expand-abbrev then the point moves even if no visible change
>> takes place. This is a VERY common occurrence for me since in viper
>> mode changing from insert to vi mode runs expand-abbrev! Placing the
>> save-excursion just around the part of abbrev-insert that actually
>> expands the abbrev fixes this problem and does not limit abbrev's from
>> moving the point (idlwave does this extensively as well and I've
>> tested that it is not impacted).
>
> Hmm... sounds like an interesting solution.  I'll take a closer look and
> get back to you.  Thank you.
>
>> Here is the diff against the
>> abbrev.el in emacs 23.3 (note that whitespace not adjusted).
>
> Highly appreciated.
>
>
>        Stefan
>

I've been thinking about this some more and while I think the
save-excursion solution mentioned above is not a bad idea, perhaps
fixing this at the heart of the problem would be better. The basic
problem is that changing state from viper insert state to viper vi
state causes abbrev's to be expanded if there is any amount of white
space between the point and an abbrev. This differs from the rules of
e.g. "space" causing an abbrev to be expanded. A space (or comma, ret,
etc) will only cause an abbrev to expand if it is immediately after an
abbrev. If changing state from viper insert to vi state followed the
same rules then everything would work nicer.

For example, what commonly happens to me is I type an abbrev (say \ef)
then return. In idlwave mode this causes \ef to be expanded to endfor
and the cursor blinks to the corresponding begin and back. Now if I
type a return and a tab to indent and then exit viper insert state
abbrev-expand is run again (since endfor is also an abbrev, for
itself) and the cursor again blinks to the begin and the cursor is
left on the endfor. The save-excursion fix only fixes where the cursor
is left. The blinking still occurs. Thus it is only a partial fix.

The rule should be "if a space (or any other non-word character) would
not cause an abbrev to expand then exiting from viper insert state
should not cause an abbrev to expand". And vise-versa. This would fix
all the issues I have with abbrev.

I've been trying to implement this today but without success. I know
why changing viper state causes abbrev to run. There is a line

(if abbrev-mode (expand-abbrev))

in viper-change-state-to-vi in viper-cmd.el which causes it. What I
cannot figure out is what causes a normal space (or comma, ret, etc)
to make an abbrev expand when one is directly after an abbrev (and
when the point is one space past the abbrev another space does not
cause an expansion, unlike the code above). Maybe you could give me a
hint. Is there a "abbrev-pending" or something like that.

Thanks,
Bob

bug#5805: 23.3 abbrev-insert needs a limited save-excursion

[Bob Nnamtrop]

On Thu, Jul 7, 2011 at 3:46 PM, Bob Nnamtrop <bobnnamtrop@gmail.com> wrote:
> On Thu, Jul 7, 2011 at 2:59 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> tags 5805 patch
>> thanks
>>
>>> I agree with the original poster that a save-excursion is needed in
>>> abbrev-expand and by making its scope limited (see below) it will not
>>> interfere with abbrevs that want to change point. The reason I find it
>>> annoying the way it is now is that some modes (e.g., idlwave) use
>>> abbrev's extensively to change keywords (e.g. capitalize them). These
>>> are defined without the leading "\". So if I am any number of lines
>>> with only whitespace below a keyword defined in this way (e.g. endfor)
>>> and run expand-abbrev then the point moves even if no visible change
>>> takes place. This is a VERY common occurrence for me since in viper
>>> mode changing from insert to vi mode runs expand-abbrev! Placing the
>>> save-excursion just around the part of abbrev-insert that actually
>>> expands the abbrev fixes this problem and does not limit abbrev's from
>>> moving the point (idlwave does this extensively as well and I've
>>> tested that it is not impacted).
>>
>> Hmm... sounds like an interesting solution.  I'll take a closer look and
>> get back to you.  Thank you.
>>
>>> Here is the diff against the
>>> abbrev.el in emacs 23.3 (note that whitespace not adjusted).
>>
>> Highly appreciated.
>>
>>
>>        Stefan
>>
>
> (SNIP)
> The rule should be "if a space (or any other non-word character) would
> not cause an abbrev to expand then exiting from viper insert state
> should not cause an abbrev to expand". And vise-versa. This would fix
> all the issues I have with abbrev.
> (SNIP)

Here are two patches that do what I explained above. I am new to elisp
so be kind :-) Note that these apply against the trunk (sorry to be
switching back of forth). I left in the save-excursion fix but it is
not absolutely necessary at this point but I still think it is the
right thing to do. Let me know what you think.

Bob

--- trunk/lisp/emulation/viper-cmd.el   2011-07-07 13:00:22.000000000 -0600
+++ local-fixes/lisp/emulation/viper-cmd.el     2011-07-07
17:45:14.000000000 -0600
@@ -617,7 +617,7 @@
     (or (viper-overlay-p viper-replace-overlay)
       (viper-set-replace-overlay (point-min) (point-min)))
     (viper-hide-replace-overlay)
-    (if abbrev-mode (expand-abbrev))
+    (if abbrev-mode (expand-abbrev t))
     (if (and auto-fill-function (> (current-column) fill-column))
        (funcall auto-fill-function))
     ;; don't leave whitespace lines around

--- trunk/lisp/abbrev.el        2011-07-07 13:00:22.000000000 -0600
+++ local-fixes/lisp/abbrev.el  2011-07-07 17:49:26.000000000 -0600
@@ -752,6 +752,7 @@
     ;; If this abbrev has an expansion, delete the abbrev
     ;; and insert the expansion.
     (when (stringp (symbol-value abbrev))
+      (save-excursion
       (goto-char wordstart)
       ;; Insert at beginning so that markers at the end (e.g. point)
       ;; are preserved.
@@ -780,7 +781,7 @@
               (skip-syntax-forward "^w" (1- end))
               ;; Change just that.
               (upcase-initials-region (point) (1+ (point)))
-              (goto-char end))))))
+              (goto-char end)))))))
     ;; Now point is at the end of the expansion and the beginning is
     ;; in last-abbrev-location.
     (when (symbol-function abbrev)
@@ -804,16 +805,18 @@
 a function that performs the abbrev expansion.  It should return
 the abbrev symbol if expansion took place.")

-(defun expand-abbrev ()
+(defun expand-abbrev (&optional strict)
   "Expand the abbrev before point, if there is an abbrev there.
 Effective when explicitly called even when `abbrev-mode' is nil.
+If strict is t then point must be directly after then end of the
+abbrev for the expansion to take place.
   (interactive)
   (run-hooks 'pre-abbrev-expand-hook)
   (with-wrapper-hook abbrev-expand-functions ()
     (destructuring-bind (&optional sym name wordstart wordend)
         (abbrev--before-point)
-      (when sym
+      (when (and sym (or (not strict) (= wordend (point))))
         (unless (or ;; executing-kbd-macro
                  noninteractive
                  (window-minibuffer-p (selected-window)))

bug#5805: 23.3 abbrev-insert needs a limited save-excursion

[Stefan Monnier]

> Here are two patches that do what I explained above. I am new to elisp
> so be kind :-) Note that these apply against the trunk (sorry to be
> switching back of forth).

I still haven't thought about the save-excursion issue, but for the
other, I think the fix should be complete in viper without changing
expand-abbrev.
The way it work in "normal Emacs" is the self-insert-command only calls
expand-abbrev if (as its docstring explains) the char inserted doesn't
have word syntax and the previous one does.  The corresponding code
(it's in C):

  if (!NILP (BVAR (current_buffer, abbrev_mode))
      && synt != Sword
      && NILP (BVAR (current_buffer, read_only))
      && PT > BEGV
      && (SYNTAX (!NILP (BVAR (current_buffer, enable_multibyte_characters))
		  ? XFASTINT (Fprevious_char ())
		  : UNIBYTE_TO_CHAR (XFASTINT (Fprevious_char ())))
	  == Sword))
    {


-- Stefan

bug#5805: 23.3 abbrev-insert needs a limited save-excursion

[Stefan Monnier]

> --- 23.3/abbrev.el      2011-07-07 08:16:16.000000000 -0600
> +++ 23.3.fix/abbrev.el  2011-07-07 08:43:24.000000000 -0600
> @@ -713,6 +713,7 @@
>      ;; If this abbrev has an expansion, delete the abbrev
>      ;; and insert the expansion.
>      (when (stringp (symbol-value abbrev))
> +      (save-excursion
>        (goto-char wordstart)
>        ;; Insert at beginning so that markers at the end (e.g. point)
>        ;; are preserved.
> @@ -741,7 +742,7 @@
>                (skip-syntax-forward "^w" (1- end))
>                ;; Change just that.
>                (upcase-initials-region (point) (1+ (point)))
> -              (goto-char end))))))
> +              (goto-char end)))))))
>      ;; Now point is at the end of the expansion and the beginning is
>      ;; in last-abbrev-location.

This can't be right, as can be seen in the comment right here: after
this (goto-char end), the rest of the code expects point to be "at the
end of the expansion".

I installed the ugly patch below instead.


        Stefan


=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2011-07-08 14:25:25 +0000
+++ lisp/ChangeLog	2011-07-08 14:41:48 +0000
@@ -5,6 +5,8 @@
 
 2011-07-08  Stefan Monnier  <monnier@iro.umontreal.ca>
 
+	* abbrev.el (expand-abbrev): Try to preserve point (bug#5805).
+
 	* vc/vc-bzr.el (vc-bzr-revision-keywords): Remove svn, it's only
 	provided by a particular plugin.
 

=== modified file 'lisp/abbrev.el'
--- lisp/abbrev.el	2011-07-05 15:31:22 +0000
+++ lisp/abbrev.el	2011-07-08 14:41:16 +0000
@@ -814,6 +814,8 @@
     (destructuring-bind (&optional sym name wordstart wordend)
         (abbrev--before-point)
       (when sym
+        (let ((startpos (copy-marker (point) t))
+              (endmark (copy-marker wordend t)))
         (unless (or ;; executing-kbd-macro
                  noninteractive
                  (window-minibuffer-p (selected-window)))
@@ -826,7 +828,14 @@
         (setq last-abbrev-location wordstart)
         ;; If this abbrev has an expansion, delete the abbrev
         ;; and insert the expansion.
-        (abbrev-insert sym name wordstart wordend)))))
+          (prog1
+              (abbrev-insert sym name wordstart wordend)
+            ;; Yuck!!  If expand-abbrev is called with point slightly
+            ;; further than the end of the abbrev, move point back to
+            ;; where it started.
+            (if (and (> startpos endmark)
+                     (= (point) endmark)) ;Obey skeletons that move point.
+                (goto-char startpos))))))))
 
 (defun unexpand-abbrev ()
   "Undo the expansion of the last abbrev that expanded.