bug#19915: 25.0.50; defadvice for `org-store-link' makes code fail because of `org-called-interactively-p'

bug#19915: 25.0.50; defadvice for `org-store-link' makes code fail because of `org-called-interactively-p'

[Drew Adams]

In an attempt to work around bug #19914, I tried advising
`org-store-link', as follows:

(defadvice org-store-link (before foo activate) "Reset `foo' to nil."
  (setq foo nil)) ; A defvar'd variable.

With that advice, this part of the `org-store-link' code fails:

 ;; Return the link
 (if (not (and (or (org-called-interactively-p 'any)
                   executing-kbd-macro) link))
     (or agenda-link (and link (org-make-link-string link desc)))
   (push (list link desc) org-stored-links)
   (message "Stored: %s" (or desc link))
   (when custom-id
     (setq link (concat "file:" (abbreviate-file-name
                                 (buffer-file-name)) "::#" custom-id))
     (push (list link desc) org-stored-links))
   (car org-stored-links))

The call to `org-called-interactively-p' returns nil, when it should
return non-nil (I invoked `org-store-link' interactively).  The result
is that `org-stored-links' is not updated.  If I unadvise the function
then there is no such problem.

The doc string of `called-interactively-p', which is used by
`org-called-interactively-p', says this, which seems relevant here:

  This function is very brittle, it may fail to return the intended
  result when the code is debugged, advised, or instrumented in some
                                    ^^^^^^^
  form.  Some macros and special forms (such as `condition-case') may
  also sometimes wrap their bodies in a `lambda', so any call to
  `called-interactively-p' from those bodies will indicate whether that
  lambda (rather than the surrounding function) was called
  interactively.

The Org code should presumably be changed following this part of that
doc string:

  Instead of using this function, it is cleaner and more reliable to
  give your function an extra optional argument whose `interactive' spec
  specifies non-nil unconditionally ("p" is a good way to do this), or
  via (not (or executing-kbd-macro noninteractive)).

The Org code uses `org-called-interactively-p' all over the place, it
seems.  Dunno whether other occurrences are as problematic as this one.

In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm@gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'

bug#19915: 25.0.50; defadvice for `org-store-link' makes code fail because of `org-called-interactively-p'

[Drew Adams]

And yes, `advice-add' does not have the same problem.

But it is a shame to limit users to recent Emacs versions
just because Org uses `called-interactively' instead of
adding an argument that indicates whether the command is
called interactively (as Emacs recommends).

bug#19915: 25.0.50; defadvice for `org-store-link' makes code fail because of `org-called-interactively-p'

[Nicolas Goaziou]

Hello,

Drew Adams <drew.adams@oracle.com> writes:

> And yes, `advice-add' does not have the same problem.
>
> But it is a shame to limit users to recent Emacs versions
> just because Org uses `called-interactively' instead of
> adding an argument that indicates whether the command is
> called interactively (as Emacs recommends).

I removed call to `called-interactively-p' in `org-store-link' (in
development version).

Thank you.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738