[PATCH] emacs: add compatability functions for emacs 23

[PATCH] emacs: add compatability functions for emacs 23

[Mark Walters]

Some of the recent changes to the emacs code have used functions
introduced in emacs 24. The functions used are read-char-choice and
setq-local. This changeset adds compatability functions to
notmuch-lib so that it should work on emacs 23.

---

Hi

I tried compiling under emacs 23 recently and noticed that some recent
changes have used some features introuduced in emacs 24. I think we
still support emacs 23 so this changeset adds two compatability
functions.

They are setq-local, which is esentially trivial, and
read-char-choice. I have written a minimal version of read-char-choice
for the functionality we need -- an alternative would be to just copy
and paste the function from emacs 24 source.

I have tested (lightly) on emacs 23 and it seems to work (and didn't
before). It should not change anything on more recent emacs (i.e. in
cases where we weren't already broken).

Finally, it does leave a compiler warning when compiling under emacs
23: notmuch-company.el does not require notmuch-lib.el but I think
that is probably OK.

Best wishes

Mark



emacs/notmuch-lib.el | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 1f0d167..1459f83 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -966,6 +966,24 @@ status."
 (defvar notmuch-show-process-crypto nil)
 (make-variable-buffer-local 'notmuch-show-process-crypto)
 
+;; Compatability functions for emacs 23.
+
+(unless (fboundp 'setq-local)
+  (defmacro setq-local (var val)
+    `(set (make-local-variable ',var) ,val)))
+
+(unless (fboundp 'read-char-choice)
+  (defun read-char-choice (prompt chars)
+    (let (char done)
+      (while (not done)
+	(setq char (read-key prompt))
+	(cond
+	 ((memq char chars)
+	  (setq done t))
+	 ((memq char '(?\C-g ?\e))
+	  (keyboard-quit))))
+      char)))
+
 (provide 'notmuch-lib)
 
 ;; Local Variables:
-- 
2.1.4

Re: [PATCH] emacs: add compatability functions for emacs 23

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Some of the recent changes to the emacs code have used functions
> introduced in emacs 24. The functions used are read-char-choice and
> setq-local. This changeset adds compatability functions to
> notmuch-lib so that it should work on emacs 23.
>
> ---
>
> Hi
>
> I tried compiling under emacs 23 recently and noticed that some recent
> changes have used some features introuduced in emacs 24. I think we
> still support emacs 23 so this changeset adds two compatability
> functions.

Hi Mark;

Can you give me a recipe to reproduce the failures in emacs23? I only
get warnings when building. Of course I believe that things will go
wrong at some point calling non-existent functions.

d

Re: [PATCH] emacs: add compatability functions for emacs 23

[Mark Walters]

On Tue, 25 Oct 2016, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>> Some of the recent changes to the emacs code have used functions
>> introduced in emacs 24. The functions used are read-char-choice and
>> setq-local. This changeset adds compatability functions to
>> notmuch-lib so that it should work on emacs 23.
>>
>> ---
>>
>> Hi
>>
>> I tried compiling under emacs 23 recently and noticed that some recent
>> changes have used some features introuduced in emacs 24. I think we
>> still support emacs 23 so this changeset adds two compatability
>> functions.
>
> Hi Mark;
>
> Can you give me a recipe to reproduce the failures in emacs23? I only
> get warnings when building. Of course I believe that things will go
> wrong at some point calling non-existent functions.

Hi

You can make the read-char-choice fail by setting the fcc header to use
insert (the default), locking the database (eg doing notmuch tag
--batch), composing and sending a message. The Fcc will fail, which will
trigger the error.

For the setq-local start to compose a message, and then run
M-x notmuch-address-toggle-internal-completion
This will give the error.

Best wishes

Mark

Re: [PATCH] emacs: add compatability functions for emacs 23

[Matt Armstrong]

Mark Walters <markwalters1009@gmail.com> writes:

> +;; Compatability functions for emacs 23.
> +
> +(unless (fboundp 'setq-local)
> +  (defmacro setq-local (var val)
> +    `(set (make-local-variable ',var) ,val)))

A concern I have with this approach is that it modifies symbols outside
the notmuch namespace.  Could people on old Emacsen come to rely on the
newer Emacs interfaces inadvertently, and wonder how they came to be,
etc?

The only other package I have non-trivial experience working with is
Gnus, and the practice there was to place portability interfaces in the
gnus- namespace, and refrain from calling the "native" interfaces
directly.

Outside Gnus, I'm ignorant of what Emacs packages typically do for this
problem.  I have no strong feelings either way.

Re: [PATCH] emacs: add compatability functions for emacs 23

[Mark Walters]

On Thu, 27 Oct 2016, Matt Armstrong <marmstrong@google.com> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>> +;; Compatability functions for emacs 23.
>> +
>> +(unless (fboundp 'setq-local)
>> +  (defmacro setq-local (var val)
>> +    `(set (make-local-variable ',var) ,val)))
>
> A concern I have with this approach is that it modifies symbols outside
> the notmuch namespace.  Could people on old Emacsen come to rely on the
> newer Emacs interfaces inadvertently, and wonder how they came to be,
> etc?

This is a good point. I think I don't mind too much if they do -- they
should see it is provided by notmuch-lib if they do describe-function
etc. But maybe bremner would like to comment?

However, maybe other packages are doing the same. Thus I think we should
not put in a cut down version of read-char-choice but just include the
whole command from the emacs24 source. That way if any other package is
doing the same load order doesn't matter -- we don't stomp on them and
they don't stomp on us.

(As an aside if we do that do we need to include any copyright notice or
similar? -- maybe notmuch-lib.el could include notmuch-compat.el which
would contain all the compatability functions?)

> The only other package I have non-trivial experience working with is
> Gnus, and the practice there was to place portability interfaces in the
> gnus- namespace, and refrain from calling the "native" interfaces
> directly.

I think this would introduce a lot of clutter, so I would prefer not to
go that route.

Best wishes

Mark

Re: [PATCH] emacs: add compatability functions for emacs 23

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> This is a good point. I think I don't mind too much if they do -- they
> should see it is provided by notmuch-lib if they do describe-function
> etc. But maybe bremner would like to comment?
>
> However, maybe other packages are doing the same. Thus I think we should
> not put in a cut down version of read-char-choice but just include the
> whole command from the emacs24 source. That way if any other package is
> doing the same load order doesn't matter -- we don't stomp on them and
> they don't stomp on us.

There is a sort of precedent with the package cl-lib.el.  Of course it's
not exactly the same since it's mainly providing new names for
functionality that already existed. Quoting from the package description:

,----
| This is a forward compatibility package, which provides (a subset of) the
| features of the cl-lib package introduced in Emacs-24.3, for use on
| previous emacsen.
| 
| Make sure this is installed *late* in your `load-path`, i.e. after Emacs's
| built-in .../lisp/emacs-lisp directory, so that if/when you upgrade to
| Emacs-24.3, the built-in version of the file will take precedence, otherwise
| you could get into trouble (although we try to hack our way around the
| problem in case it happens).
`----

> (As an aside if we do that do we need to include any copyright notice or
> similar? -- maybe notmuch-lib.el could include notmuch-compat.el which
> would contain all the compatability functions?)

We should definitely preserve copyright information (the licensing is
the same, so no problem there)

>> The only other package I have non-trivial experience working with is
>> Gnus, and the practice there was to place portability interfaces in the
>> gnus- namespace, and refrain from calling the "native" interfaces
>> directly.
>
> I think this would introduce a lot of clutter, so I would prefer not to
> go that route.

I don't have strong opinions about that, but note that we're talking
about (currently) 5 lines of code.

d

Re: [PATCH] emacs: add compatability functions for emacs 23

[Matt Armstrong]

David Bremner <david@tethera.net> writes:

> Mark Walters <markwalters1009@gmail.com> writes:
>
>> This is a good point. I think I don't mind too much if they do -- they
>> should see it is provided by notmuch-lib if they do describe-function
>> etc. But maybe bremner would like to comment?
>>
>> However, maybe other packages are doing the same. Thus I think we should
>> not put in a cut down version of read-char-choice but just include the
>> whole command from the emacs24 source. That way if any other package is
>> doing the same load order doesn't matter -- we don't stomp on them and
>> they don't stomp on us.
>
> There is a sort of precedent with the package cl-lib.el.  Of course it's
> not exactly the same since it's mainly providing new names for
> functionality that already existed.

I'm obviously in no place to object strongly, but I like this approach
less the more I think about it.  I've had similar things come back to
bite me too many times in the past.

Here I find Steve Purcell raising the same concern:

https://github.com/melpa/melpa/pull/1748#issuecomment-45082451
https://github.com/swift-emacs/swift-mode/issues/10

I'm concerned entirely about surprises.  Folks don't expect packages to
provide future compatibility functions.  Imagine using Emacs 23 and,
maybe because you're copy/paste hacking your personal elisp, you come to
use setq-local.  Or perhaps you use some *other* add-on that uses
setq-local but makes no effort to support Emacs 23.  Then you decide to
move (require 'notmuch) a little lower in your .emacs and *boom*, your
config is broken.

So I'd rather see this kind of thing:

;; TODO: remove and use setq-local directly when Emacs 23 support is dropped.
(if (fboundp 'setq-local)
    (defalias 'notmuch-setq-local 'setq-local)
  (defmacro notmuch-setq-local (var val)
    "Set variable VAR to value VAL in current buffer."
    `(set (make-local-variable ',var) ,val)))

Then just call notmuch-setq-local from notmuch code.  That follows the
expected convention that packages add symbols under their own namespace.

But, as far as prior art, we have examples of other packages patching in
a setq-local:

https://github.com/flycheck/flycheck/commit/b19bd4fdf118c55a96d72b2b3c034a0393cfcae2
https://github.com/fxbois/web-mode/issues/438

...and I have to agree with Steve when he says "This isn't the most
egregious example, though. :-)"