bug#15599: [PATCH] Inferior Lisp Mode Menu

bug#15599: [PATCH] Inferior Lisp Mode Menu

[John Anthony]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

I have put together a menu for inferior-lisp mode, which is one of the
two listed in etc/TODO (line 66, I already supplied a patch for
text-mode). Please do let me know if there's anything I could have
done better as I'm planning to make sure all major modes have
appropriate menus.

-- 
John Anthony

[-- Attachment #2: inf-lisp-mode-menu.patch --]
[-- Type: text/x-diff, Size: 954 bytes --]

diff --git a/lisp/progmodes/inf-lisp.el b/lisp/progmodes/inf-lisp.el
index d73e948..4cda3fe 100644
--- a/lisp/progmodes/inf-lisp.el
+++ b/lisp/progmodes/inf-lisp.el
@@ -91,6 +91,19 @@ mode.  Default is whitespace followed by 0 or 1 single-letter colon-keyword
     (define-key map "\C-c\C-v" 'lisp-show-variable-documentation)
     map))
 
+(easy-menu-define
+  inferior-lisp-menu
+  inferior-lisp-mode-map
+  "Inferior Lisp Menu"
+  `("Inf-Lisp"
+    ["Eval Last Sexp" lisp-eval-last-sexp t]
+    ["Load File" lisp-load-file t]
+    ["Compile File" lisp-compile-file t]
+    ["Show Arglist" lisp-show-arglist t]
+    ["Describe Sym" lisp-describe-sym t]
+    ["Show Fucntion Doc" lisp-show-function-documentation t]
+    ["Show Variable Doc" lisp-show-variable-documentation t]))
+
 ;;; These commands augment Lisp mode, so you can process Lisp code in
 ;;; the source files.
 (define-key lisp-mode-map "\M-\C-x"  'lisp-eval-defun)     ; Gnu convention

bug#15599: [PATCH] Inferior Lisp Mode Menu

[John Anthony]

[-- Attachment #1: Type: text/plain, Size: 179 bytes --]

Oh, I'm terribly sorry. I used a quasiquote when a quote would have
done. This is probably the worst thing that has ever happened and I am
suitably embarrassed.

-- 
John Anthony

[-- Attachment #2: inf-lisp-mode-menu2.patch --]
[-- Type: text/x-diff, Size: 954 bytes --]

diff --git a/lisp/progmodes/inf-lisp.el b/lisp/progmodes/inf-lisp.el
index d73e948..eda7fda 100644
--- a/lisp/progmodes/inf-lisp.el
+++ b/lisp/progmodes/inf-lisp.el
@@ -91,6 +91,19 @@ mode.  Default is whitespace followed by 0 or 1 single-letter colon-keyword
     (define-key map "\C-c\C-v" 'lisp-show-variable-documentation)
     map))
 
+(easy-menu-define
+  inferior-lisp-menu
+  inferior-lisp-mode-map
+  "Inferior Lisp Menu"
+  '("Inf-Lisp"
+    ["Eval Last Sexp" lisp-eval-last-sexp t]
+    ["Load File" lisp-load-file t]
+    ["Compile File" lisp-compile-file t]
+    ["Show Arglist" lisp-show-arglist t]
+    ["Describe Sym" lisp-describe-sym t]
+    ["Show Fucntion Doc" lisp-show-function-documentation t]
+    ["Show Variable Doc" lisp-show-variable-documentation t]))
+
 ;;; These commands augment Lisp mode, so you can process Lisp code in
 ;;; the source files.
 (define-key lisp-mode-map "\M-\C-x"  'lisp-eval-defun)     ; Gnu convention

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Stefan Monnier]

> Oh, I'm terribly sorry.  I used a quasiquote when a quote would have
> done.   This is probably the worst thing that has ever happened and I am
> suitably embarrassed.

This is sickening!  I sentence you to a month of `vi'.
Oh, wait, you misnamed "backquote" to add insult to injury.  So that'll
be a two months of `ed' instead!


        Stefan "I hope you'll learn your lesson"

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Glenn Morris]

Thanks. Please send a reminder when you hear your copyright assignment
is complete.

> ["Eval Last Sexp" lisp-eval-last-sexp t]

Is it useful to have that appearing in the inferior lisp buffer's menu?
Surely it's normally used with stuff in a Lisp Mode buffer?

> ["Load File" lisp-load-file t]

Should be "Load File..."
Similarly for "Compile File...", and indeed for most of these menu items
I think.

> ["Show Fucntion Doc" lisp-show-function-documentation t]
         ^^^^^^^^
I think "Show Documentation for Function..." would be better.
But again, isn't this a command more likely to be useful in a Lisp Mode
buffer? IMO this applies to many of those menu items.
But I never use inferior-lisp-mode, so I don't know.

bug#15599: [PATCH] Inferior Lisp Mode Menu

[John Anthony]

On Fri, Oct 18, 2013 at 10:48:11PM -0400, Glenn Morris wrote:
> 
> Thanks. Please send a reminder when you hear your copyright assignment
> is complete.

I'll be sure to contact you as soon as I hear back.

> > ["Eval Last Sexp" lisp-eval-last-sexp t]
> 
> Is it useful to have that appearing in the inferior lisp buffer's menu?
> Surely it's normally used with stuff in a Lisp Mode buffer?

This was my initial though as well, although I have used it in an
inferior lisp session as well - useful for checking the last sexp you
wrote will give the correct result whilst working on a larger
expression.

> > ["Load File" lisp-load-file t]
> 
> Should be "Load File..."
> Similarly for "Compile File...", and indeed for most of these menu items
> I think.

I think you're quite correct. Before deciding to work on them I, like
I think most Emacs users, always hid and never thought about the
menus. It's taken me a while to really grok the style and filter out
the inconsistent stuff from the good.

> > ["Show Fucntion Doc" lisp-show-function-documentation t]
>          ^^^^^^^^
> I think "Show Documentation for Function..." would be better.
> But again, isn't this a command more likely to be useful in a Lisp Mode
> buffer? IMO this applies to many of those menu items.
> But I never use inferior-lisp-mode, so I don't know.

I've taken to spell-checking whilst I work on menu entries as well.

There are lots of times I've looked at things and tried to decide if
it should be in the menu entry. etc/TODO says inferior-lisp should
have a menu entry but all of the functions seem to be better off in a
Lisp Mode buffer. I'll also resist the urge for brevity and just use a
proper sentence in future - I guess it makes sense to provide menu
users with an informative menu item as they are less likely to have
much experience. In future I'll append "..." to menu entries that
require further input in accordance with style rules.

The two entries you pointed out seem to be the most useful entries,
actually, as I could see myself wanting to look up documentation
(especially for the last-typed function) in an inferior-lisp.

As always, style and implementation feedback is extremely welcome. I'm
intending to do a lot of this and therefore early feedback will
prevent me from doing a lot of extra work fixing things I've done
wrong.

-- 
John Anthony

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Stefan Monnier]

BTW, regarding inferior processes, it'd be really good to add
a library that provides the "standard" link between a programming mode
buffer and an inferior process, and then to adapt existing major modes
to use that library.

FWIW, I had started such a library (while working on sml-mode) but it's
just a first attempt, which has not yet been used for anything else than
sml-mode.

You can check sml-mode.el (in elpa) to see an example of its use (tho
sml-mode includes a copy of that prog-proc thingy, renamed to
sml-prog-proc).


        Stefan


;;; prog-proc.el --- Interacting from a source buffer with an inferior process  -*- lexical-binding: t; coding: utf-8 -*-

;; Copyright (C) 2012  Free Software Foundation, Inc.

;; Author: (Stefan Monnier) <monnier@iro.umontreal.ca>

;; This file is part of GNU Emacs.

;; GNU Emacs is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; GNU Emacs is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.

;;; Commentary:

;; Prog-Proc is a package designed to complement Comint: while Comint was
;; designed originally to handle the needs of inferior process buffers, such
;; as a buffer running a Scheme repl, Comint does not actually provide any
;; functionality that links this process buffer with some source code.
;;
;; That's where Prog-Proc comes into play: it provides the usual commands and
;; key-bindings that lets the user send his code to the underlying repl.

;;; Code:


(eval-when-compile (require 'cl))
(require 'comint)
(require 'compile)

(defvar prog-proc-mode-map
  (let ((map (make-sparse-keymap)))
    (define-key map [?\C-c ?\C-l] 'prog-proc-load-file)
    (define-key map [?\C-c ?\C-c] 'prog-proc-compile)
    (define-key map [?\C-c ?\C-z] 'prog-proc-switch-to)
    (define-key map [?\C-c ?\C-r] 'prog-proc-send-region)
    (define-key map [?\C-c ?\C-b] 'prog-proc-send-buffer)
    ;; FIXME: Add
    ;; (define-key map [?\M-C-x] 'prog-proc-send-defun)
    ;; (define-key map [?\C-x ?\C-e] 'prog-proc-send-last-sexp)
    ;; FIXME: Add menu.  Now, that's trickier because keymap inheritance
    ;; doesn't play nicely with menus!
    map)
  "Keymap for `prog-proc-mode'.")

(defvar prog-proc--buffer nil
  "The inferior-process buffer to which to send code.")
(make-variable-buffer-local 'prog-proc--buffer)

(defstruct (prog-proc-descriptor
            (:constructor prog-proc-make)
            (:predicate nil)
            (:copier nil))
  (name nil :read-only t)
  (run nil :read-only t)
  (load-cmd nil :read-only t)
  (chdir-cmd nil :read-only t)
  (command-eol "\n" :read-only t)
  (compile-commands-alist nil :read-only t))

(defvar prog-proc-descriptor nil
  "Struct containing the various functions to create a new process, ...")

(defmacro prog-proc--prop (prop)
  `(,(intern (format "prog-proc-descriptor-%s" prop))
    (or prog-proc-descriptor
        ;; FIXME: Look for available ones and pick one.
        (error "Not a `prog-proc' buffer"))))
(defmacro prog-proc--call (method &rest args)
  `(funcall (prog-proc--prop ,method) ,@args))

;; The inferior process and his buffer are basically interchangeable.
;; Currently the code takes prog-proc--buffer as the main reference,
;; but all users should either use prog-proc-proc or prog-proc-buffer
;; to find the info.

(defun prog-proc-proc ()
  "Return the inferior process for the code in current buffer."
  (or (and (buffer-live-p prog-proc--buffer)
           (get-buffer-process prog-proc--buffer))
      (when (derived-mode-p 'prog-proc-mode 'prog-proc-comint-mode)
        (setq prog-proc--buffer (current-buffer))
        (get-buffer-process prog-proc--buffer))
      (let ((ppd prog-proc-descriptor)
            (buf (prog-proc--call run)))
        (with-current-buffer buf
          (if (and ppd (null prog-proc-descriptor))
              (set (make-local-variable 'prog-proc-descriptor) ppd)))
        (setq prog-proc--buffer buf)
        (get-buffer-process prog-proc--buffer))))

(defun prog-proc-buffer ()
  "Return the buffer of the inferior process."
  (process-buffer (prog-proc-proc)))

(defun prog-proc-run-repl ()
  "Start the read-eval-print process, if it's not running yet."
  (interactive)
  (ignore (prog-proc-proc)))

(defun prog-proc-switch-to ()
  "Switch to the buffer running the read-eval-print process."
  (pop-to-buffer (prog-proc-buffer)))

(defun prog-proc-send-string (proc str)
  "Send command STR to PROC, with an EOL terminator appended."
  (with-current-buffer (process-buffer proc)
    ;; FIXME: comint-send-string does not pass the string through
    ;; comint-input-filter-function, so we have to do it by hand.
    ;; Maybe we should insert the command into the buffer and then call
    ;; comint-send-input?
    (prog-proc-comint-input-filter-function nil)
    (comint-send-string proc (concat str (prog-proc--prop command-eol)))))

(defun prog-proc-load-file (file &optional and-go)
  "Load FILE into the read-eval-print process.
FILE is the file visited by the current buffer.
If prefix argument AND-GO is used, then we additionally switch
to the buffer where the process is running."
  (interactive
   (list (or buffer-file-name
             (read-file-name "File to load: " nil nil t))
         current-prefix-arg))
  (comint-check-source file)
  (let ((proc (prog-proc-proc)))
    (prog-proc-send-string proc (prog-proc--call load-cmd file))
    (when and-go (pop-to-buffer (process-buffer proc)))))

(defvar prog-proc--tmp-file nil)

(defun prog-proc-send-region (start end &optional and-go)
  "Send the content of the region to the read-eval-print process.
START..END delimit the region; AND-GO if non-nil indicate to additionally
switch to the process's buffer."
  (interactive "r\nP")
  (if (> start end) (let ((tmp end)) (setq end start) (setq start tmp))
    (if (= start end) (error "Nothing to send: the region is empty")))
  (let ((proc (prog-proc-proc))
        (tmp (make-temp-file "emacs-region")))
    (write-region start end tmp nil 'silently)
    (when prog-proc--tmp-file
      (ignore-errors (delete-file (car prog-proc--tmp-file)))
      (set-marker (cdr prog-proc--tmp-file) nil))
    (setq prog-proc--tmp-file (cons tmp (copy-marker start)))
    (prog-proc-send-string proc (prog-proc--call load-cmd tmp))
    (when and-go (pop-to-buffer (process-buffer proc)))))

(defun prog-proc-send-buffer (&optional and-go)
  "Send the content of the current buffer to the read-eval-print process.
AND-GO if non-nil indicate to additionally switch to the process's buffer."
  (interactive "P")
  (prog-proc-send-region (point-min) (point-max) and-go))

(define-derived-mode prog-proc-mode prog-mode "Prog-Proc"
  "Major mode for editing source code and interact with an interactive loop."
  )

;;; Extended comint-mode for Prog-Proc.

(defun prog-proc-chdir (dir)
  "Change the working directory of the inferior process to DIR."
  (interactive "DChange to directory: ")
  (let ((dir (expand-file-name dir))
        (proc (prog-proc-proc)))
    (with-current-buffer (process-buffer proc)
      (prog-proc-send-string proc (prog-proc--call chdir-cmd dir))
      (setq default-directory (file-name-as-directory dir)))))

(defun prog-proc-comint-input-filter-function (str)
  ;; `compile.el' doesn't know that file location info from errors should be
  ;; recomputed afresh (without using stale info from earlier compilations).
  (compilation-forget-errors)       ;Has to run before compilation-fake-loc.
  (if (and prog-proc--tmp-file (marker-buffer (cdr prog-proc--tmp-file)))
      (compilation-fake-loc (cdr prog-proc--tmp-file)
                            (car prog-proc--tmp-file)))
  str)

(defvar prog-proc-comint-mode-map
  (let ((map (make-sparse-keymap)))
    (define-key map [?\C-c ?\C-r] 'prog-proc-run-repl)
    (define-key map [?\C-c ?\C-l] 'prog-proc-load-file)
    map))

(define-derived-mode prog-proc-comint-mode comint-mode "Prog-Proc-Comint"
  "Major mode for an inferior process used to run&compile source code."
  ;; Enable compilation-minor-mode, but only after the child mode is setup
  ;; since the child-mode might want to add rules to
  ;; compilation-error-regexp-alist.
  (add-hook 'after-change-major-mode-hook #'compilation-minor-mode nil t)
  ;; The keymap of compilation-minor-mode is too unbearable, so we
  ;; need to hide most of the bindings.
  (let ((map (make-sparse-keymap)))
    (dolist (keys '([menu-bar] [follow-link]))
      ;; Preserve some of the bindings.
      (define-key map keys (lookup-key compilation-minor-mode-map keys)))
    (add-to-list 'minor-mode-overriding-map-alist
                 (cons 'compilation-minor-mode map)))

  (add-hook 'comint-input-filter-functions
            #'prog-proc-comint-input-filter-function nil t))

(defvar prog-proc--compile-command nil
  "The command used by default by `prog-proc-compile'.")

(defun prog-proc-compile (command &optional and-go)
  "Pass COMMAND to the read-eval-loop process to compile the current file.

You can then use the command \\[next-error] to find the next error message
and move to the source code that caused it.

Interactively, prompts for the command if `compilation-read-command' is
non-nil.  With prefix arg, always prompts.

Prefix arg AND-GO also means to switch to the read-eval-loop buffer afterwards."
  (interactive
   (let* ((dir default-directory)
	  (cmd "cd \"."))
     ;; Look for files to determine the default command.
     (while (and (stringp dir)
                 (progn
                   (dolist (cf (prog-proc--prop compile-commands-alist))
                     (when (file-exists-p (expand-file-name (cdr cf) dir))
                       (setq cmd (concat cmd "\"; " (car cf)))
                       (return nil)))
                   (not cmd)))
       (let ((newdir (file-name-directory (directory-file-name dir))))
	 (setq dir (unless (equal newdir dir) newdir))
	 (setq cmd (concat cmd "/.."))))
     (setq cmd
	   (cond
	    ((local-variable-p 'prog-proc--compile-command)
             prog-proc--compile-command)
	    ((string-match "^\\s-*cd\\s-+\"\\.\"\\s-*;\\s-*" cmd)
	     (substring cmd (match-end 0)))
	    ((string-match "^\\s-*cd\\s-+\"\\(\\./\\)" cmd)
	     (replace-match "" t t cmd 1))
	    ((string-match ";" cmd) cmd)
	    (t prog-proc--compile-command)))
     ;; code taken from compile.el
     (list (if (or compilation-read-command current-prefix-arg)
               (read-from-minibuffer "Compile command: "
				     cmd nil nil '(compile-history . 1))
             cmd))))
     ;; ;; now look for command's file to determine the directory
     ;; (setq dir default-directory)
     ;; (while (and (stringp dir)
     ;; 	    (dolist (cf (prog-proc--prop compile-commands-alist) t)
     ;; 	      (when (and (equal cmd (car cf))
     ;; 			 (file-exists-p (expand-file-name (cdr cf) dir)))
     ;; 		(return nil))))
     ;;   (let ((newdir (file-name-directory (directory-file-name dir))))
     ;;     (setq dir (unless (equal newdir dir) newdir))))
     ;; (setq dir (or dir default-directory))
     ;; (list cmd dir)))
  (set (make-local-variable 'prog-proc--compile-command) command)
  (save-some-buffers (not compilation-ask-about-save) nil)
  (let ((dir default-directory))
    (when (string-match "^\\s-*cd\\s-+\"\\([^\"]+\\)\"\\s-*;" command)
      (setq dir (match-string 1 command))
      (setq command (replace-match "" t t command)))
    (setq dir (expand-file-name dir))
    (let ((proc (prog-proc-proc))
          (eol (prog-proc--prop command-eol)))
      (with-current-buffer (process-buffer proc)
        (setq default-directory dir)
        (prog-proc-send-string
         proc (concat (prog-proc--call chdir-cmd dir)
                      ;; Strip the newline, to avoid adding a prompt.
                      (if (string-match "\n\\'" eol)
                          (replace-match " " t t eol) eol)
                      command))
        (when and-go (pop-to-buffer (process-buffer proc)))))))


\f
(provide 'prog-proc)

;;; prog-proc.el ends here

bug#15599: [PATCH] Inferior Lisp Mode Menu

[John Anthony]

On Fri, Oct 18, 2013 at 10:48:11PM -0400, Glenn Morris wrote:
> 
> Thanks. Please send a reminder when you hear your copyright assignment
> is complete.

My copyright assignment is complete.

A quick question of style before I submit a patch with your
suggestions applied; should I check that a user is going to use a menu
option correctly and, if not, disable it? Disabling it seems to
improve tidiness but can also hide important feedback. To use a
working example, should I disable "Eval Last Sexp" in the menu if the
cursor is not positioned after a valid sexp?  It feels like it would
be better for a person to see the error (i.e. Scan error: "Unbalanced
Parenthesis").

-- 
John Anthony

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Glenn Morris]

John Anthony wrote:

> A quick question of style before I submit a patch with your
> suggestions applied; should I check that a user is going to use a menu
> option correctly and, if not, disable it?

Sometimes yes, sometimes no. Eg it makes sense to me that "Cut" and
"Copy" in the "Edit" menu are disabled when there is no active region.

> To use a working example, should I disable "Eval Last Sexp" in the
> menu if the cursor is not positioned after a valid sexp?

But that seems like too much. I'd leave such an item always enabled.

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Glenn Morris]

Stefan Monnier wrote:

> BTW, regarding inferior processes, it'd be really good to add
> a library that provides the "standard" link between a programming mode
> buffer and an inferior process, and then to adapt existing major modes
> to use that library.

Cloned as http://debbugs.gnu.org/15697

bug#15599: [PATCH] Inferior Lisp Mode Menu

[John Anthony]

Glenn Morris wrote:
> > To use a working example, should I disable "Eval Last Sexp" in the
> > menu if the cursor is not positioned after a valid sexp?
> 
> But that seems like too much. I'd leave such an item always enabled.

I'll be honest - I was always expecting the answer to be "it depends
on the context". Here's a patch with your style advice applied:

diff --git a/lisp/progmodes/inf-lisp.el b/lisp/progmodes/inf-lisp.el
index d73e948..a9953e2 100644
--- a/lisp/progmodes/inf-lisp.el
+++ b/lisp/progmodes/inf-lisp.el
@@ -91,6 +91,21 @@ mode.  Default is whitespace followed by 0 or 1 single-letter colon-keyword
     (define-key map "\C-c\C-v" 'lisp-show-variable-documentation)
     map))
 
+(easy-menu-define
+  inferior-lisp-menu
+  inferior-lisp-mode-map
+  "Inferior Lisp Menu"
+  '("Inf-Lisp"
+    ["Eval Last Sexp" lisp-eval-last-sexp t]
+    "--"
+    ["Load File..." lisp-load-file t]
+    ["Compile File..." lisp-compile-file t]
+    "--"
+    ["Show Arglist..." lisp-show-arglist t]
+    ["Describe Symbol..." lisp-describe-sym t]
+    ["Show Documentation for Function..." lisp-show-function-documentation t]
+    ["Show Documentation for Variable..." lisp-show-variable-documentation t]))
+
 ;;; These commands augment Lisp mode, so you can process Lisp code in
 ;;; the source files.
 (define-key lisp-mode-map "\M-\C-x"  'lisp-eval-defun)     ; Gnu convention

-- 
John Anthony
http://jo.hnanthony.com

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Stefan Monnier]

>> To use a working example, should I disable "Eval Last Sexp" in the
>> menu if the cursor is not positioned after a valid sexp?
> But that seems like too much. I'd leave such an item always enabled.

Agreed.  I can see two reasons for it:
- these "enabled" tests are run, pretty much all the time, so they need
  to be sufficiently "quick" not to disrupt normal work.  Testing if the
  last sexp is properly balanced may have to scan megabytes of text
  (even if it usually is not the case), so it can slow things down
  noticeably.
- it should be reasonably obvious to the user why a menu entry is
  disabled, and I think most users wouldn't find this one to be obvious.


        Stefan

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Drew Adams]

> > I'd leave such an item always enabled.
> 
> Agreed.  I can see two reasons for it:
> - these "enabled" tests are run, pretty much all the time, so they
>   need to be sufficiently "quick" not to disrupt normal work.  Testing
>   if the last sexp is properly balanced may have to scan megabytes of
>   text (even if it usually is not the case), so it can slow things down
>   noticeably.
> - it should be reasonably obvious to the user why a menu entry is
>   disabled, and I think most users wouldn't find this one to be
>   obvious.

Yes.  The first reason is pretty important, and needs to be taken into
consideration even when there are not megabytes to scan etc.  Pretty
much any :enable or :visible test needs to be simple & quick.

Dunno whether this is mentioned in the manual, but I think it should be,
as a useful guideline.

bug#15599: [PATCH] Inferior Lisp Mode Menu

[Glenn Morris]

Version: 24.4

Applied.