Re: ielm automatic saving of history -- bug 67000

[Simen Heggestøyl]

Hi, I've had a look at the patch now.

Madhu <enometh@meer.net> writes:

>  (defcustom ielm-history-file-name
> -  (locate-user-emacs-file "ielm-history.eld")
> +  (if t nil
> +      (locate-user-emacs-file "ielm-history.eld"))
>    "If non-nil, name of the file to read/write IELM input history."
>    :type '(choice (const :tag "Disable input history" nil)
>                   file)

I don't think the default value should be changed. Having IELM keep
history by default aligns it with how other Emacs shells behave, such as
Eshell and other comint-based shells such as the Octave and IDLWAVE
shells.

> @@ -520,7 +521,9 @@ ielm--input-history-writer
>    "Return a function writing IELM input history to BUF."
>    (lambda ()
>      (with-current-buffer buf
> -      (comint-write-input-ring))))
> +      (let ((coding-system-for-write 'utf-8-emacs-unix))
> +        ;; cannot add a file-local section when using comint.
> +        (comint-write-input-ring)))))

Seems fine to be, but shouldn't a similar change also be needed
everywhere else where we call `comint-write-input-ring`? Should this
perhaps be handled by that function?

> @@ -623,17 +626,19 @@ inferior-emacs-lisp-mode
>    (add-hook 'comint-indirect-setup-hook
>              #'ielm-indirect-setup-hook 'append t)
>    (setq comint-indirect-setup-function #'emacs-lisp-mode)
> -
> -  ;; Input history
> -  (setq-local comint-input-ring-file-name ielm-history-file-name)
> -  (setq-local ielm--exit (ielm--input-history-writer (current-buffer)))
> -  (setq-local kill-buffer-hook
> +  (when ielm-history-file-name
> +    ;; Input history
> +    (setq-local comint-input-ring-file-name ielm-history-file-name)
> +    (setq-local ielm--exit (ielm--input-history-writer (current-buffer)))
> +    (add-hook 'kill-buffer-hook

The `when` check is unnecessary, since this is already handled in
`comint-read-input-ring` and `comint-write-input-ring`.

Another small bonus is that without the `when` check, a change to
`ielm-history-file-name` will take effect without having to reactivate
IELM mode.

The change from `(setq-local kill-buffer-hook ...` to `(add-hook
'kill-buffer-hook ...` looks good to me.

> +    (let ((coding-system-for-read 'utf-8-unix))
> +      (comint-read-input-ring t)))

Shouldn't this also be `utf-8-emacs-unix`?

-- Simen