unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [patch]  minor patch for register.el
@ 2013-02-19 23:18 Drew Adams
  2013-02-20  2:18 ` Masatake YAMATO
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Drew Adams @ 2013-02-19 23:18 UTC (permalink / raw)
  To: emacs-devel

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

If the attached patch is OK I will send a change log entry.

The patch does this:

1. Adds functions `register-insertable-p', `register-jumpable-p', and
`register-printable-p'.

2. In `jump-to-register':
   . Use `register-jumpable-p'.
   . Raise error if no such register or cannot jump to it.

3. In `view-register': Raise error if no such register.

4. In `list-registers': Raise error if `register-alist' is empty.

5. In `describe-register-1':
   . Use `register-printable-p'.
   . Wrap names with `...'.

6. In `insert-register':
   . Use `register-insertable-p'.
   . Raise error if no such register or cannot insert contents.

7. In `append-to-register' and `prepend-to-register':
   Name the register in error msg.

[-- Attachment #2: register-2013-02-19.patch --]
[-- Type: application/octet-stream, Size: 13777 bytes --]

diff -c -w register.el register-patched-2013-02-19.el
*** register.el	Tue Feb 19 10:40:22 2013
--- register-patched-2013-02-19.el	Tue Feb 19 15:04:08 2013
***************
*** 83,90 ****
    :version "24.3")
  
  (defcustom register-separator nil
!   "Register containing the text to put between collected texts, or nil if none.
! 
  When collecting text with
  `append-to-register' (resp. `prepend-to-register') contents of
  this register is added to the beginning (resp. end) of the marked
--- 83,89 ----
    :version "24.3")
  
  (defcustom register-separator nil
!   "Register containing text to put between collected texts, or nil if none.
  When collecting text with
  `append-to-register' (resp. `prepend-to-register') contents of
  this register is added to the beginning (resp. end) of the marked
***************
*** 106,111 ****
--- 105,157 ----
        (push (cons register value) register-alist))
      value))
  
+ (defun register-insertable-p (register)
+   "Return non-nil if the value of REGISTER can be inserted.
+ That is, whether it can be passed to `insert-register' without raising
+ an error."
+   (let ((val  (get-register register)))
+     (or (and (registerv-p val)  (registerv-insert-func val))
+         (consp val)
+         (stringp val)
+         (numberp val)
+         (and (markerp val)  (marker-position val))
+         (and (fboundp 'semantic-foreign-tag-p)
+              semantic-mode
+              (semantic-foreign-tag-p val)))))
+ 
+ (defun register-jumpable-p (register &optional msgp)
+   "Return non-nil if the value of REGISTER can be jumped to.
+ That is, whether it can be passed to `jump-to-register' without
+ raising an error.
+ 
+ Non-nil optional arg MSGP means the function can query the user
+ whether to revisit a file whose buffer no longer exists."
+   (let ((val  (get-register register)))
+     (or (and (registerv-p val)  (registerv-jump-func val))
+         (and (consp val)
+              (or (frame-configuration-p (car val))
+                  (window-configuration-p (car val))
+                  (eq (car val) 'file)
+                  (and (eq (car val) 'file-query)
+                       (or (find-buffer-visiting (nth 1 val))
+                           (and msgp
+                                (y-or-n-p (format "Visit file `%s' again? "
+                                                  (nth 1 val))))))))
+         (and (markerp val)  (marker-buffer val))
+         (and (fboundp 'semantic-foreign-tag-p)
+              semantic-mode
+              (semantic-foreign-tag-p val)))))
+ 
+ (defun register-printable-p (register)
+   "Return non-nil if the value of REGISTER can be printed.
+ That is, whether the value is a recognized register value."
+   (let ((val  (get-register register)))
+     (or (and (registerv-p val)  (registerv-print-func val))
+         (consp val)                     ; Includes rectangle.
+         (numberp val)
+         (markerp val)
+         (stringp val))))
+ 
  (defun point-to-register (register &optional arg)
    "Store current location of point in register REGISTER.
  With prefix argument, store current frame configuration.
***************
*** 148,159 ****
  delete any existing frames that the frame configuration doesn't mention.
  \(Otherwise, these frames are iconified.)"
    (interactive "cJump to register: \nP")
    (let ((val (get-register register)))
!     (cond
!      ((registerv-p val)
!       (cl-assert (registerv-jump-func val) nil
!               "Don't know how to jump to register %s"
!               (single-key-description register))
        (funcall (registerv-jump-func val) (registerv-data val)))
       ((and (consp val) (frame-configuration-p (car val)))
        (set-frame-configuration (car val) (not delete))
--- 194,205 ----
  delete any existing frames that the frame configuration doesn't mention.
  \(Otherwise, these frames are iconified.)"
    (interactive "cJump to register: \nP")
+   (unless (get-register register)
+     (error "No such register: `%s'" (single-key-description register)))
+   (unless (register-jumpable-p register (called-interactively-p 'interactive))
+     (error "Cannot jump to register `%s'" (single-key-description register)))
    (let ((val (get-register register)))
!     (cond ((registerv-p val)
             (funcall (registerv-jump-func val) (registerv-data val)))
            ((and (consp val) (frame-configuration-p (car val)))
             (set-frame-configuration (car val) (not delete))
***************
*** 234,264 ****
    (interactive "cView register: ")
    (let ((val (get-register register)))
      (if (null val)
! 	(message "Register %s is empty" (single-key-description register))
        (with-output-to-temp-buffer "*Output*"
  	(describe-register-1 register t)))))
  
  (defun list-registers ()
    "Display a list of nonempty registers saying briefly what they contain."
    (interactive)
    (let ((list (copy-sequence register-alist)))
      (setq list (sort list (lambda (a b) (< (car a) (car b)))))
      (with-output-to-temp-buffer "*Output*"
        (dolist (elt list)
  	(when (get-register (car elt))
  	  (describe-register-1 (car elt))
! 	  (terpri))))))
  
  (defun describe-register-1 (register &optional verbose)
-   (princ "Register ")
-   (princ (single-key-description register))
-   (princ " contains ")
    (let ((val (get-register register)))
!     (cond
       ((registerv-p val)
!       (if (registerv-print-func val)
!           (funcall (registerv-print-func val) (registerv-data val))
!         (princ "[UNPRINTABLE CONTENTS].")))
  
       ((numberp val)
        (princ val))
--- 280,314 ----
    (interactive "cView register: ")
    (let ((val (get-register register)))
      (if (null val)
!         (error "No such register: `%s'" (single-key-description register))
        (with-output-to-temp-buffer "*Output*"
          (describe-register-1 register t)))))
  
  (defun list-registers ()
    "Display a list of nonempty registers saying briefly what they contain."
    (interactive)
+   (if (null register-alist)
+       (error "No registers to describe")
      (let ((list (copy-sequence register-alist)))
        (setq list (sort list (lambda (a b) (< (car a) (car b)))))
        (with-output-to-temp-buffer "*Output*"
          (dolist (elt list)
            (when (get-register (car elt))
              (describe-register-1 (car elt))
!             (terpri)))))))
  
  (defun describe-register-1 (register &optional verbose)
    (let ((val  (get-register register)))
!     (princ "Register `")
!     (princ (single-key-description register))
!     (princ "' contains ")
!     (cond ((not (register-printable-p register))
!            (if verbose
!                (format "garbage (?):\n%S" val))
!              "unprintable contents.")
! 
            ((registerv-p val)
!            (funcall (registerv-print-func val) (registerv-data val)))
  
            ((numberp val)
             (princ val))
***************
*** 266,275 ****
       ((markerp val)
        (let ((buf (marker-buffer val)))
  	(if (null buf)
! 	    (princ "a marker in no buffer")
! 	  (princ "a buffer position:\n    buffer ")
  	  (princ (buffer-name buf))
! 	  (princ ", position ")
  	  (princ (marker-position val)))))
  
       ((and (consp val) (window-configuration-p (car val)))
--- 316,325 ----
            ((markerp val)
             (let ((buf (marker-buffer val)))
               (if (null buf)
!                  (princ "a marker in no buffer.")
!                (princ "a buffer position:\n    buffer `")
                 (princ (buffer-name buf))
!                (princ "', position ")
                 (princ (marker-position val)))))
  
            ((and (consp val) (window-configuration-p (car val)))
***************
*** 279,292 ****
        (princ "a frame configuration."))
  
       ((and (consp val) (eq (car val) 'file))
!       (princ "the file ")
        (prin1 (cdr val))
!       (princ "."))
  
       ((and (consp val) (eq (car val) 'file-query))
!       (princ "a file-query reference:\n    file ")
        (prin1 (car (cdr val)))
!       (princ ",\n    position ")
        (princ (car (cdr (cdr val))))
        (princ "."))
  
--- 329,342 ----
             (princ "a frame configuration."))
  
            ((and (consp val) (eq (car val) 'file))
!            (princ "the file `")
             (prin1 (cdr val))
!            (princ "'."))
  
            ((and (consp val) (eq (car val) 'file-query))
!            (princ "a file-query reference:\n    file `")
             (prin1 (car (cdr val)))
!            (princ "',\n    position ")
             (princ (car (cdr (cdr val))))
             (princ "."))
  
***************
*** 312,320 ****
  	    (princ "the text:\n")
  	    (princ val))
  	(cond
! 	 ;; Extract first N characters starting with first non-whitespace.
  	 ((string-match (format "[^ \t\n].\\{,%d\\}"
! 				;; Deduct 6 for the spaces inserted below.
  				(min 20 (max 0 (- (window-width) 6))))
  			val)
  	  (princ "text starting with\n    ")
--- 362,370 ----
                   (princ "the text:\n")
                   (princ val))
               (cond
!                ;; Extract first N chars, starting with first non-whitespace.
                 ((string-match (format "[^ \t\n].\\{,%d\\}"
!                                       ;; Deduct 6 for spaces inserted below.
                                        (min 20 (max 0 (- (window-width) 6))))
                                val)
                  (princ "text starting with\n    ")
***************
*** 322,331 ****
  	 ((string-match "^[ \t\n]+$" val)
  	  (princ "whitespace"))
  	 (t
! 	  (princ "the empty string")))))
!      (t
!       (princ "Garbage:\n")
!       (if verbose (prin1 val))))))
  
  (defun insert-register (register &optional arg)
    "Insert contents of register REGISTER.  (REGISTER is a character.)
--- 372,378 ----
                 ((string-match "^[ \t\n]+$" val)
                  (princ "whitespace"))
                 (t
!                 (princ "the empty string"))))))))
  
  (defun insert-register (register &optional arg)
    "Insert contents of register REGISTER.  (REGISTER is a character.)
***************
*** 334,345 ****
  Interactively, second arg is non-nil if prefix arg is supplied."
    (interactive "*cInsert register: \nP")
    (push-mark)
    (let ((val (get-register register)))
!     (cond
!      ((registerv-p val)
!       (cl-assert (registerv-insert-func val) nil
!               "Don't know how to insert register %s"
!               (single-key-description register))
        (funcall (registerv-insert-func val) (registerv-data val)))
       ((consp val)
        (insert-rectangle val))
--- 381,393 ----
  Interactively, second arg is non-nil if prefix arg is supplied."
    (interactive "*cInsert register: \nP")
    (push-mark)
+   (unless (get-register register)
+     (error "No such register: `%s'" (single-key-description register)))
+   (unless (register-insertable-p register)
+     (error "Cannot insert the contents of register `%s'"
+            (single-key-description register)))
    (let ((val (get-register register)))
!     (cond ((registerv-p val)
             (funcall (registerv-insert-func val) (registerv-data val)))
            ((consp val)
             (insert-rectangle val))
***************
*** 352,361 ****
       ((and (fboundp 'semantic-foreign-tag-p)
  	   semantic-mode
  	   (semantic-foreign-tag-p val))
!       (semantic-insert-foreign-tag val))
!      (t
!       (error "Register does not contain text"))))
!   (if (not arg) (exchange-point-and-mark)))
  
  (defun copy-to-register (register start end &optional delete-flag)
    "Copy region into register REGISTER.
--- 400,407 ----
            ((and (fboundp 'semantic-foreign-tag-p)
                  semantic-mode
                  (semantic-foreign-tag-p val))
!            (semantic-insert-foreign-tag val))))
!   (unless arg (exchange-point-and-mark)))
  
  (defun copy-to-register (register start end &optional delete-flag)
    "Copy region into register REGISTER.
***************
*** 382,388 ****
      (set-register
       register (cond ((not reg) text)
                      ((stringp reg) (concat reg separator text))
!                     (t (error "Register does not contain text")))))
    (setq deactivate-mark t)
    (cond (delete-flag
  	 (delete-region start end))
--- 428,435 ----
      (set-register
       register (cond ((not reg) text)
                      ((stringp reg) (concat reg separator text))
!                     (t (error "Register `%s' does not contain text"
!                               (single-key-description register))))))
    (setq deactivate-mark t)
    (cond (delete-flag
  	 (delete-region start end))
***************
*** 401,407 ****
      (set-register
       register (cond ((not reg) text)
                      ((stringp reg) (concat text separator reg))
!                     (t (error "Register does not contain text")))))
    (setq deactivate-mark t)
    (cond (delete-flag
  	 (delete-region start end))
--- 448,455 ----
      (set-register
       register (cond ((not reg) text)
                      ((stringp reg) (concat text separator reg))
!                     (t (error "Register `%s' does not contain text"
!                               (single-key-description register))))))
    (setq deactivate-mark t)
    (cond (delete-flag
  	 (delete-region start end))

Diff finished.  Tue Feb 19 15:05:49 2013

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch] minor patch for register.el
  2013-02-19 23:18 [patch] minor patch for register.el Drew Adams
@ 2013-02-20  2:18 ` Masatake YAMATO
  2013-02-20  4:30   ` Drew Adams
  2013-02-20 17:05 ` Stefan Monnier
  2013-02-21 20:29 ` Bastien
  2 siblings, 1 reply; 15+ messages in thread
From: Masatake YAMATO @ 2013-02-20  2:18 UTC (permalink / raw)
  To: drew.adams; +Cc: emacs-devel

Hi,

( I'm cheering those who are working on improving register:)
  I used to try but I could not show a good result[1][2].
  I still believe register facility should be imporved 
  massively. )

> 4. In `list-registers': Raise error if `register-alist' is empty.

Just question.
Is this error raising really needed?

`list-processes' says nothing even if no child process is.
`list-directory' says nothing even if the directory given as argument is 
empty.

[1] https://lists.gnu.org/archive/html/emacs-devel/2004-03/msg00345.html
[2] https://lists.gnu.org/archive/html/emacs-devel/2004-03/msg00349.html


Masatake YAMATO



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch] minor patch for register.el
  2013-02-20  2:18 ` Masatake YAMATO
@ 2013-02-20  4:30   ` Drew Adams
  2013-02-20 14:48     ` John Yates
  0 siblings, 1 reply; 15+ messages in thread
From: Drew Adams @ 2013-02-20  4:30 UTC (permalink / raw)
  To: 'Masatake YAMATO'; +Cc: emacs-devel

> ( I'm cheering those who are working on improving register:)
>   I used to try but I could not show a good result[1][2].
>   I still believe register facility should be imporved 
>   massively. )
>
> [1] 
> https://lists.gnu.org/archive/html/emacs-devel/2004-03/msg00345.html
> [2] 
> https://lists.gnu.org/archive/html/emacs-devel/2004-03/msg00349.html

I don't disagree that registers could be more useful.  Dunno about "massively",
but I'm sure some more could be done.  (More could also be done with Lisp
variables for interactive use, IMO.)

Something I use are some trivial commands that copy register contents to a
variable and vice versa.  Likewise, copy between a register and the last kill.
Nothing spectacular.  (I took the `register-*able-p' functions for the patch
from that code, BTW.)

> > 4. In `list-registers': Raise error if `register-alist' is empty.
> 
> Just question.  Is this error raising really needed?

Personally, I don't really care one way or the other - (a) show a message and do
nothing or (b) raise an error.

IMO, a message (error or not) is more useful in this case than opening an empty
`*Output*' buffer showing there are no registers.  In my case, FWIW, that pops
up a new frame - silly, if not quite annoying.

(I probably should also have proposed a new name for that buffer, BTW -
`*Output*' suggests that little reflection went into the name.  `*Registers*' or
`*Register List*' would be better.)

Now, there could be a question whether `list-registers' also has some
non-interactive use.  If so, and if we are concerned about a message (error or
not) when not used interactively, then we can add a MSGP optional arg etc.  The
same holds for some of the other commands here.

I think it's good the way I defined it (show an error msg if no registers), but
I don't feel strongly about it at all.  I do think it's a bit silly to create a
window or frame showing an empty `*Output*' buffer.)

> `list-processes' says nothing even if no child process is.
> `list-directory' says nothing even if the directory given as 
> argument is empty.

FWIW, I don't see either of those as very comparable to `list-registers'.

Certainly not the Dired case.  You can work in an empty directory, doing lots of
things (including creating files and subdirs, search below in multiple subdirs,
etc.).




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch] minor patch for register.el
  2013-02-20  4:30   ` Drew Adams
@ 2013-02-20 14:48     ` John Yates
  2013-02-20 14:53       ` Christopher Schmidt
  2013-02-20 15:27       ` Drew Adams
  0 siblings, 2 replies; 15+ messages in thread
From: John Yates @ 2013-02-20 14:48 UTC (permalink / raw)
  To: Drew Adams; +Cc: Masatake YAMATO, emacs-devel

On Tue, Feb 19, 2013 at 11:30 PM, Drew Adams <drew.adams@oracle.com> wrote:

>> Just question.  Is this error raising really needed?
>
> Personally, I don't really care one way or the other - (a) show a message and do
> nothing or (b) raise an error.

Raising an error would be tantamount to suggesting that it is user's
responsibility to _know_ a priori that the set is empty and to avoid
invoking the function.  Encountering a normal boundary state is not an
error.

I run with debug-on-error set.  Raising errors for non-error
conditions is very distracting.

/john



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch] minor patch for register.el
  2013-02-20 14:48     ` John Yates
@ 2013-02-20 14:53       ` Christopher Schmidt
  2013-02-20 15:27       ` Drew Adams
  1 sibling, 0 replies; 15+ messages in thread
From: Christopher Schmidt @ 2013-02-20 14:53 UTC (permalink / raw)
  To: emacs-devel

John Yates <john@yates-sheets.org> writes:
> Raising an error would be tantamount to suggesting that it is user's
> responsibility to _know_ a priori that the set is empty and to avoid
> invoking the function.  Encountering a normal boundary state is not an
> error.
>
> I run with debug-on-error set.  Raising errors for non-error
> conditions is very distracting.

I think user-error should be used here.

        Christopher



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch] minor patch for register.el
  2013-02-20 14:48     ` John Yates
  2013-02-20 14:53       ` Christopher Schmidt
@ 2013-02-20 15:27       ` Drew Adams
  1 sibling, 0 replies; 15+ messages in thread
From: Drew Adams @ 2013-02-20 15:27 UTC (permalink / raw)
  To: 'John Yates'; +Cc: 'Masatake YAMATO', emacs-devel

> > Personally, I don't really care one way or the other - (a) 
> > show a message and do nothing or (b) raise an error.
> 
> Raising an error would be tantamount to suggesting that it is user's
> responsibility to _know_ a priori that the set is empty and to avoid
> invoking the function.  Encountering a normal boundary state is not an
> error.
> 
> I run with debug-on-error set.  Raising errors for non-error
> conditions is very distracting.

Again, I don't care which we do here: message or error.  But if you are worried
about an error then you'd better look throughout register.el at the other
commands.  They pretty much all already raise errors for similar situations -
see the cl assertions.

Feel free to change the error raising in the patch to `message' calls, as far as
I am concerned.  Or use `user-error', as Christopher suggests.

The point, for me, is not to open an empty window/frame here: display a message
instead.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch]  minor patch for register.el
  2013-02-19 23:18 [patch] minor patch for register.el Drew Adams
  2013-02-20  2:18 ` Masatake YAMATO
@ 2013-02-20 17:05 ` Stefan Monnier
  2013-02-20 23:15   ` Drew Adams
  2013-02-21 20:29 ` Bastien
  2 siblings, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2013-02-20 17:05 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> 1. Adds functions `register-insertable-p', `register-jumpable-p', and
> `register-printable-p'.

Why are these needed?  The text description of the change sounds good,
but the corresponding code ends up duplicating a lot of `cond' code with
no obvious direct benefit.

The rest of the patch looks good (see further comments below).  Tho,
please don't turn the "(cond\n..." into "(cond ...)" which ends up
reindenting all the code gratuitously.

> @@ -148,12 +194,12 @@
>  delete any existing frames that the frame configuration doesn't mention.
>  \(Otherwise, these frames are iconified.)"
>    (interactive "cJump to register: \nP")
> +  (unless (get-register register)
> +    (error "No such register: `%s'" (single-key-description register)))
> +  (unless (register-jumpable-p register (called-interactively-p 'interactive))
> +    (error "Cannot jump to register `%s'" (single-key-description register)))
>    (let ((val (get-register register)))
> -    (cond
> -     ((registerv-p val)
> -      (cl-assert (registerv-jump-func val) nil
> -              "Don't know how to jump to register %s"
> -              (single-key-description register))
> +    (cond ((registerv-p val)
>        (funcall (registerv-jump-func val) (registerv-data val)))
>       ((and (consp val) (frame-configuration-p (car val)))
>        (set-frame-configuration (car val) (not delete))

Using cl-assert indeed sounds wrong.  It should probably be
a user-error instead.  But what is the benefit of moving the test?

> -	(message "Register %s is empty" (single-key-description register))
> +        (error "No such register: `%s'" (single-key-description register))

Make it a user-error.

> +  (if (null register-alist)
> +      (error "No registers to describe")

Same here.


        Stefan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch]  minor patch for register.el
  2013-02-20 17:05 ` Stefan Monnier
@ 2013-02-20 23:15   ` Drew Adams
  2013-03-11 18:01     ` Stefan Monnier
  0 siblings, 1 reply; 15+ messages in thread
From: Drew Adams @ 2013-02-20 23:15 UTC (permalink / raw)
  To: 'Stefan Monnier'; +Cc: emacs-devel

> > Adds functions `register-insertable-p', 
> > `register-jumpable-p', and `register-printable-p'.
> 
> Why are these needed?  The text description of the change sounds good,
> but the corresponding code ends up duplicating a lot of 
> `cond' code with no obvious direct benefit.

General predicates usable elsewhere also.
Unified error msg if register does not satisfy the predicate.

> Using cl-assert indeed sounds wrong.  It should probably be
> a user-error instead.

And the same error msg is appropriate even for cases where `registerv-p' is not
true.

> But what is the benefit of moving the test?

Moving what test?  Doing a `get-register' test outside the cond?  Testing
jumpability outside the cond?  In both cases: to raise the right error msg.

E.g., do this with the current code: `C-x r j w'.  The error msg you get is not
appropriate: "Register doesn't contain a buffer position or configuration".
Dunno whether that msg is really appropriate for `file' or `file-query'.  But
even if it is, the real error in this case is that `w' is not a register.

Do you want a patch with a change that indents the cond clauses as before and
uses `user-error' instead of `error'?  Or do you prefer to make whatever changes
you want yourself?




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch]  minor patch for register.el
  2013-02-19 23:18 [patch] minor patch for register.el Drew Adams
  2013-02-20  2:18 ` Masatake YAMATO
  2013-02-20 17:05 ` Stefan Monnier
@ 2013-02-21 20:29 ` Bastien
  2013-02-21 21:20   ` Drew Adams
  2 siblings, 1 reply; 15+ messages in thread
From: Bastien @ 2013-02-21 20:29 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

Hi Drew,

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

> If the attached patch is OK I will send a change log entry.

just out of curiosity, did you have a look at register-list.el?

You can install it from ELPA then define some registers then
C-x r L to list them.

I don't have time right now to make a patch against register.el
to enhance it with features from register-list.el, but that was
(and still is) my plan -- but I want to clean the dust, perhaps
use tabulated-list-mode for it and some other things.

In any case, maybe you can get some ideas from this library.

Best,

-- 
 Bastien



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch]  minor patch for register.el
  2013-02-21 20:29 ` Bastien
@ 2013-02-21 21:20   ` Drew Adams
  2013-02-21 21:25     ` Bastien
  0 siblings, 1 reply; 15+ messages in thread
From: Drew Adams @ 2013-02-21 21:20 UTC (permalink / raw)
  To: 'Bastien'; +Cc: emacs-devel

> just out of curiosity, did you have a look at register-list.el?

Nope, I know nothing about it.  Can you give me a URL for downloading it?

> You can install it from ELPA

I don't want to install it, but I would like to look at the source somewhere.

> define some registers then C-x r L to list them.

Sounds good.

> I don't have time right now to make a patch against register.el
> to enhance it with features from register-list.el, but that was
> (and still is) my plan -- but I want to clean the dust, perhaps
> use tabulated-list-mode for it and some other things.
> 
> In any case, maybe you can get some ideas from this library.

Feel free to do what you describe, from my point of view.
At least based on your description it sounds like an improvement.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch]  minor patch for register.el
  2013-02-21 21:20   ` Drew Adams
@ 2013-02-21 21:25     ` Bastien
  2013-02-21 21:29       ` Drew Adams
  0 siblings, 1 reply; 15+ messages in thread
From: Bastien @ 2013-02-21 21:25 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

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

>> just out of curiosity, did you have a look at register-list.el?
>
> Nope, I know nothing about it.  Can you give me a URL for
> downloading it?

http://lumiere.ens.fr/~guerry/u/register-list.el

> Feel free to do what you describe, from my point of view.
> At least based on your description it sounds like an improvement.

I pointed at this library because I see there are some changes
done in register.el and I hope those changes won't conflict with
the ones I plan to make.  But as I cannot announce a date when
I'll be able to provide a patch, I cannot freeze register.el
just because I want to work on it... hence this semi-solution
of providing you the link so you can get some ideas.

-- 
 Bastien



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch]  minor patch for register.el
  2013-02-21 21:25     ` Bastien
@ 2013-02-21 21:29       ` Drew Adams
  2013-02-21 21:34         ` Drew Adams
  0 siblings, 1 reply; 15+ messages in thread
From: Drew Adams @ 2013-02-21 21:29 UTC (permalink / raw)
  To: 'Bastien'; +Cc: emacs-devel

> > Nope, I know nothing about it.  Can you give me a URL for
> > downloading it?
> 
> http://lumiere.ens.fr/~guerry/u/register-list.el

Thanks.

> > Feel free to do what you describe, from my point of view.
> > At least based on your description it sounds like an improvement.
> 
> I pointed at this library because I see there are some changes
> done in register.el and I hope those changes won't conflict with
> the ones I plan to make.  But as I cannot announce a date when
> I'll be able to provide a patch, I cannot freeze register.el
> just because I want to work on it... hence this semi-solution
> of providing you the link so you can get some ideas.

If any changes are made to register.el before you get around to improving it a
ta facon, and if those changes make it harder for you, you can always get back
today's version from VC.

The patch I sent was pretty trivial.  And it probably doesn't matter whether it
gets applied or not.  IOW, if it makes life easier not to apply it, then don't.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch]  minor patch for register.el
  2013-02-21 21:29       ` Drew Adams
@ 2013-02-21 21:34         ` Drew Adams
  0 siblings, 0 replies; 15+ messages in thread
From: Drew Adams @ 2013-02-21 21:34 UTC (permalink / raw)
  To: 'Bastien'; +Cc: emacs-devel

> http://lumiere.ens.fr/~guerry/u/register-list.el

FWIW, looks good to me.  (Haven't tried it yet, but it sounds useful.)




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [patch]  minor patch for register.el
  2013-02-20 23:15   ` Drew Adams
@ 2013-03-11 18:01     ` Stefan Monnier
  2013-03-11 20:46       ` Drew Adams
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2013-03-11 18:01 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

>> But what is the benefit of moving the test?
> Moving what test?

Moving the code of the test from "inline" to separate predicate
functions (which ends up duplicating code).

> Testing jumpability outside the cond?

For example, yes.

> In both cases: to raise the right error msg.

I don't see the connection between the two.

> E.g., do this with the current code: `C-x r j w'.  The error msg you
> get is not appropriate: "Register doesn't contain a buffer position or
> configuration".

I agree that changing the message is a good idea, but rather than
duplicate (error "No such register: `%s'" ...), I'd rather add an
optional `not-empty' arg to get-register and raise the (user-)error in
there once and forall.
[ BTW, I think "register is empty" is more correct than "no such
  register".  ]

As mentioned above, I don't see how that's connected to the use of
separate predicate functions, so maybe I'm missing something.

> Do you want a patch with a change that indents the cond clauses as
> before and uses `user-error' instead of `error'?

Yes, that would be appreciated.


        Stefan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [patch]  minor patch for register.el
  2013-03-11 18:01     ` Stefan Monnier
@ 2013-03-11 20:46       ` Drew Adams
  0 siblings, 0 replies; 15+ messages in thread
From: Drew Adams @ 2013-03-11 20:46 UTC (permalink / raw)
  To: 'Stefan Monnier'; +Cc: emacs-devel

See my reply to Bastien.

If you or he wants to patch register.el along the lines I suggested before he
makes his more substantial (and welcome, IMO) changes, please feel free to do
so.

> [ BTW, I think "register is empty" is more correct than "no such
>   register". ]

It's all about the ambiguity of nil as the return value of `get-register'.  It
can mean that there is no such register or that the contents are empty (e.g.,
the register has an entry in `register-alist' but its value is (REGISTER .
nil)).

As long as we do not distinguish those two cases we cannot tell the user which
is the case for a given input char.

"Register is empty" really makes sense (is helpful) only if the register
previously was not empty.  Otherwise it can be misleading.

"No such register" is more useful in general, IMO, since there is no simple way
(see above) to tell whether a given register previously was non-empty.

When you type a character that has never been defined as a register, the message
should tell you that.  This is the more likely cause of a nil `get-register'
value, IMO: you hit the wrong char.  And in any case it is the more important
cause for a newbie.  The message should target this easy-to-make mistake.

Put differently, we do not distinguish an empty register from a non-existent
register.  And given that, it is more helpful for users, IMO, to speak of both
cases as non-existence.

Note that we already use more specific error msgs for an empty register when the
command is to insert ("Register doesn't contain text") or to jump ("Register
doesn't contain a buffer position or configuration") or to increment ("Register
doesn't contain a number or text").

That is helpful because it emphasizes what the command is for.

However, it could also be argued that these messages can also be misleading when
`get-register' returns nil, because they really suggest a type error, rather
than an empty or non-existent register.  Such a message makes more sense when
the contents are not empty but are of the wrong type - e.g., a number as content
when you try to `C-x r i'.

But again, please do whatever you like.  And I hope that Bastien will soon find
the time to finish his intended update.  That will be useful.




^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-03-11 20:46 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-19 23:18 [patch] minor patch for register.el Drew Adams
2013-02-20  2:18 ` Masatake YAMATO
2013-02-20  4:30   ` Drew Adams
2013-02-20 14:48     ` John Yates
2013-02-20 14:53       ` Christopher Schmidt
2013-02-20 15:27       ` Drew Adams
2013-02-20 17:05 ` Stefan Monnier
2013-02-20 23:15   ` Drew Adams
2013-03-11 18:01     ` Stefan Monnier
2013-03-11 20:46       ` Drew Adams
2013-02-21 20:29 ` Bastien
2013-02-21 21:20   ` Drew Adams
2013-02-21 21:25     ` Bastien
2013-02-21 21:29       ` Drew Adams
2013-02-21 21:34         ` Drew Adams

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).