whitespace-cleanup from /etc/TODO

whitespace-cleanup from /etc/TODO

[Yoni Rabkin Katzenell]

Hello, I'm running GNU Emacs 22.0.50.1 (i686-pc-linux-gnu, X toolkit,
 Xaw3d scroll bars) of 2006-07-23 on ardbeg.

In /etc/TODO there is the following: "** whitespace-cleanup should work
only on the region if the region is active.".

Looking at whitespace.el I see that `whitespace-cleanup' will work on
the region if (and transient-mark-mode mark-active), otherwise it will
work on the entire buffer. I don't know if this is what you mean by
"...only on the region if the region is active", but it seems to be
already doing the correct thing.

The confusing bit is that `whitespace-cleanup' will always print the
following message when done:

(if (not whitespace-silent)
    (message "%s clean" buffer-file-name))

So if you judge according to the message, you can easily misunderstand
that `whitespace-cleanup' is unconditionally operating on the entire
buffer.

The following trivial patch will print an appropriate message when
`whitespace-cleanup' is run on a region, otherwise it will print that
`whitespace-cleanup' has run on the entire buffer.

diff -c /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el /home/yrk/devel/sandbox/emacs-hacking/whitespace.el
*** /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el	2006-08-13 21:23:58.542642560 +0300
--- /home/yrk/devel/sandbox/emacs-hacking/whitespace.el	2006-08-13 21:12:38.085087832 +0300
***************
*** 139,144 ****
--- 139,148 ----
  (make-variable-buffer-local 'whitespace-highlighted-space)
  (put 'whitespace-highlighted-space 'permanent-local nil)
  
+ (defvar whitespace-region-only nil
+   "Keep track of whether whitespace has been run on a region or buffer.")
+ (make-variable-buffer-local 'whitespace-region-only)
+ 
  ;; For flavors of Emacs which don't define `defgroup' and `defcustom'.
  (eval-when-compile
    (if (not (fboundp 'defgroup))
***************
*** 522,528 ****
    (interactive)
    (if (and transient-mark-mode mark-active)
        (whitespace-cleanup-region (region-beginning) (region-end))
!     (whitespace-cleanup-internal)))
  
  (defun whitespace-cleanup-internal ()
    ;; If this buffer really contains a file, then run, else quit.
--- 526,534 ----
    (interactive)
    (if (and transient-mark-mode mark-active)
        (whitespace-cleanup-region (region-beginning) (region-end))
!     (progn
!       (setq whitespace-region-only nil)
!       (whitespace-cleanup-internal))))
  
  (defun whitespace-cleanup-internal ()
    ;; If this buffer really contains a file, then run, else quit.
***************
*** 569,577 ****
  	;; Call this recursively till everything is taken care of
  	(if whitespace-any
  	    (whitespace-cleanup-internal)
  	  (progn
! 	    (if (not whitespace-silent)
! 		(message "%s clean" buffer-file-name))
  	    (whitespace-update-modeline)))
  	(setq tab-width whitespace-tabwith-saved))))
  
--- 575,586 ----
  	;; Call this recursively till everything is taken care of
  	(if whitespace-any
  	    (whitespace-cleanup-internal)
+ 	  ;; if we are done, talk to the user
  	  (progn
! 	    (unless whitespace-silent
! 	      (if whitespace-region-only
! 		  (message "The region is now clean")
! 		(message "%s is now clean" buffer-file-name)))
  	    (whitespace-update-modeline)))
  	(setq tab-width whitespace-tabwith-saved))))
  
***************
*** 579,584 ****
--- 588,594 ----
  (defun whitespace-cleanup-region (s e)
    "Whitespace cleanup on the region."
    (interactive "r")
+   (setq whitespace-region-only t)
    (save-excursion
      (save-restriction
        (narrow-to-region s e)

Diff finished.  Sun Aug 13 21:24:34 2006

-- 
   "Cut your own wood and it will warm you twice"

Re: whitespace-cleanup from /etc/TODO

[Adrian Aichner]

Yoni Rabkin Katzenell <yoni-r@actcom.com> writes:

> The following trivial patch will print an appropriate message when
> `whitespace-cleanup' is run on a region, otherwise it will print that
> `whitespace-cleanup' has run on the entire buffer.
>
> diff -c /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el /home/yrk/devel/sandbox/emacs-hacking/whitespace.el
> *** /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el	2006-08-13 21:23:58.542642560 +0300
> --- /home/yrk/devel/sandbox/emacs-hacking/whitespace.el	2006-08-13 21:12:38.085087832 +0300
> ***************
> --- 575,586 ----
>   	;; Call this recursively till everything is taken care of
>   	(if whitespace-any
>   	    (whitespace-cleanup-internal)
> + 	  ;; if we are done, talk to the user
>   	  (progn
> ! 	    (unless whitespace-silent
> ! 	      (if whitespace-region-only
> ! 		  (message "The region is now clean")

I suggest to retain the buffer-file-name argument in the case above.

Otherwise you're trading an old piece of valuable information against
a new (perhaps less important) one.

Adrian

> ! 		(message "%s is now clean" buffer-file-name)))
>   	    (whitespace-update-modeline)))
>   	(setq tab-width whitespace-tabwith-saved))))
>   

-- 
Adrian Aichner
 mailto:adrian@xemacs.org
 http://www.xemacs.org/

Re: whitespace-cleanup from /etc/TODO

[Yoni Rabkin Katzenell]

Adrian Aichner <adrian@xemacs.org> writes:

> Yoni Rabkin Katzenell <yoni-r@actcom.com> writes:
>
>> The following trivial patch will print an appropriate message when
>> `whitespace-cleanup' is run on a region, otherwise it will print that
>> `whitespace-cleanup' has run on the entire buffer.
>>
>> diff -c /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el /home/yrk/devel/sandbox/emacs-hacking/whitespace.el
>> *** /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el	2006-08-13 21:23:58.542642560 +0300
>> --- /home/yrk/devel/sandbox/emacs-hacking/whitespace.el	2006-08-13 21:12:38.085087832 +0300
>> ***************
>> --- 575,586 ----
>>   	;; Call this recursively till everything is taken care of
>>   	(if whitespace-any
>>   	    (whitespace-cleanup-internal)
>> + 	  ;; if we are done, talk to the user
>>   	  (progn
>> ! 	    (unless whitespace-silent
>> ! 	      (if whitespace-region-only
>> ! 		  (message "The region is now clean")
>
> I suggest to retain the buffer-file-name argument in the case above.
>
> Otherwise you're trading an old piece of valuable information against
> a new (perhaps less important) one.

But the case above does not describe an entire-buffer operation. The
case above is only when `whitespace-clean' has been called on a region
within the buffer.

> Adrian
>
>> ! 		(message "%s is now clean" buffer-file-name)))
>>   	    (whitespace-update-modeline)))
>>   	(setq tab-width whitespace-tabwith-saved))))

It does not make sense to me to tell the user that the entire file has
been cleaned when only a region of it has been cleaned. So I might be
misunderstanding your point.

-- 
   "Cut your own wood and it will warm you twice"

Re: whitespace-cleanup from /etc/TODO

[Stuart D. Herring]

> + (defvar whitespace-region-only nil
> +   "Keep track of whether whitespace has been run on a region or
> buffer.")
> + (make-variable-buffer-local 'whitespace-region-only)
> +
>   ;; For flavors of Emacs which don't define `defgroup' and `defcustom'.
>   (eval-when-compile
>     (if (not (fboundp 'defgroup))
> ***************
> *** 522,528 ****
>     (interactive)
>     (if (and transient-mark-mode mark-active)
>         (whitespace-cleanup-region (region-beginning) (region-end))
> !     (whitespace-cleanup-internal)))
>
>   (defun whitespace-cleanup-internal ()
>     ;; If this buffer really contains a file, then run, else quit.
> --- 526,534 ----
>     (interactive)
>     (if (and transient-mark-mode mark-active)
>         (whitespace-cleanup-region (region-beginning) (region-end))
> !     (progn
> !       (setq whitespace-region-only nil)
> !       (whitespace-cleanup-internal))))

This presumably works, but it's the long way around.  Just add an argument
REGION to `whitespace-cleanup-internal' instead of defining a new
top-level symbol.  Then invoke it from `whitespace-cleanup-region' with
that argument non-nil.  If there's some reason that doesn't work, at least
`let'-bind the variable rather than just setting it.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: whitespace-cleanup from /etc/TODO

[Yoni Rabkin Katzenell]

"Stuart D. Herring" <herring@lanl.gov> writes:

>> + (defvar whitespace-region-only nil
>> +   "Keep track of whether whitespace has been run on a region or
>> buffer.")
>> + (make-variable-buffer-local 'whitespace-region-only)
>> +
>>   ;; For flavors of Emacs which don't define `defgroup' and `defcustom'.
>>   (eval-when-compile
>>     (if (not (fboundp 'defgroup))
>> ***************
>> *** 522,528 ****
>>     (interactive)
>>     (if (and transient-mark-mode mark-active)
>>         (whitespace-cleanup-region (region-beginning) (region-end))
>> !     (whitespace-cleanup-internal)))
>>
>>   (defun whitespace-cleanup-internal ()
>>     ;; If this buffer really contains a file, then run, else quit.
>> --- 526,534 ----
>>     (interactive)
>>     (if (and transient-mark-mode mark-active)
>>         (whitespace-cleanup-region (region-beginning) (region-end))
>> !     (progn
>> !       (setq whitespace-region-only nil)
>> !       (whitespace-cleanup-internal))))
>
> This presumably works, but it's the long way around.  Just add an argument
> REGION to `whitespace-cleanup-internal' instead of defining a new
> top-level symbol.  Then invoke it from `whitespace-cleanup-region' with
> that argument non-nil.  If there's some reason that doesn't work, at least
> `let'-bind the variable rather than just setting it.

You are right. Here is a patch to solve the problem as you suggest:

diff -c /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el /home/yrk/devel/sandbox/emacs-hacking/whitespace.el
*** /home/yrk/devel/sandbox/emacs-hacking/whitespace-original.el	2006-08-13 21:23:58.000000000 +0300
--- /home/yrk/devel/sandbox/emacs-hacking/whitespace.el	2006-08-15 21:27:15.066569112 +0300
***************
*** 524,530 ****
        (whitespace-cleanup-region (region-beginning) (region-end))
      (whitespace-cleanup-internal)))
  
! (defun whitespace-cleanup-internal ()
    ;; If this buffer really contains a file, then run, else quit.
    (whitespace-check-whitespace-mode current-prefix-arg)
    (if (and buffer-file-name whitespace-mode)
--- 524,530 ----
        (whitespace-cleanup-region (region-beginning) (region-end))
      (whitespace-cleanup-internal)))
  
! (defun whitespace-cleanup-internal (&optional region-only)
    ;; If this buffer really contains a file, then run, else quit.
    (whitespace-check-whitespace-mode current-prefix-arg)
    (if (and buffer-file-name whitespace-mode)
***************
*** 569,577 ****
  	;; Call this recursively till everything is taken care of
  	(if whitespace-any
  	    (whitespace-cleanup-internal)
  	  (progn
! 	    (if (not whitespace-silent)
! 		(message "%s clean" buffer-file-name))
  	    (whitespace-update-modeline)))
  	(setq tab-width whitespace-tabwith-saved))))
  
--- 569,580 ----
  	;; Call this recursively till everything is taken care of
  	(if whitespace-any
  	    (whitespace-cleanup-internal)
+ 	  ;; if we are done, talk to the user
  	  (progn
! 	    (unless whitespace-silent
! 	      (if region-only
! 		  (message "The region is now clean")
! 		(message "%s is now clean" buffer-file-name)))
  	    (whitespace-update-modeline)))
  	(setq tab-width whitespace-tabwith-saved))))
  
***************
*** 582,588 ****
    (save-excursion
      (save-restriction
        (narrow-to-region s e)
!       (whitespace-cleanup-internal))
      (whitespace-buffer t)))
  
  (defun whitespace-buffer-leading ()
--- 585,591 ----
    (save-excursion
      (save-restriction
        (narrow-to-region s e)
!       (whitespace-cleanup-internal t))
      (whitespace-buffer t)))
  
  (defun whitespace-buffer-leading ()

Diff finished.  Tue Aug 15 21:30:17 2006

-- 
   "Cut your own wood and it will warm you twice"