[PATCH] delete-trailing-whitespace on active region

[PATCH] delete-trailing-whitespace on active region

[Deniz Dogan]

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

I have found myself in need of deleting trailing whitespace only in a
specific region from time to time.  Therefore, I wrote this patch
which changes the behavior of the command to act only on the region if
it is active.

So before I commit this, I'd like to check it with you guys.

Is this new behavior okay?  If so, is this patch okay?

--
Deniz Dogan

[-- Attachment #2: dtw.patch --]
[-- Type: application/octet-stream, Size: 1820 bytes --]

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2011-02-01 21:22:21 +0000
+++ lisp/simple.el	2011-02-09 18:15:31 +0000
@@ -614,22 +614,25 @@
     (if (looking-at "^[ \t]*\n\\'")
 	(delete-region (point) (point-max)))))
 
-(defun delete-trailing-whitespace ()
+(defun delete-trailing-whitespace (&optional start end)
   "Delete all the trailing whitespace across the current buffer.
 All whitespace after the last non-whitespace character in a line is deleted.
 This respects narrowing, created by \\[narrow-to-region] and friends.
-A formfeed is not considered whitespace by this function."
-  (interactive "*")
+A formfeed is not considered whitespace by this function.
+If the region is active, only delete whitespace within the region."
+  (interactive "*r")
   (save-match-data
     (save-excursion
-      (goto-char (point-min))
-      (while (re-search-forward "\\s-$" nil t)
-	(skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
-	;; Don't delete formfeeds, even if they are considered whitespace.
-	(save-match-data
-	  (if (looking-at ".*\f")
-	      (goto-char (match-end 0))))
-	(delete-region (point) (match-end 0))))))
+      (let ((start (or (and (region-active-p) start) (point-min)))
+            (end (or (and (region-active-p) end) (point-max))))
+        (goto-char start)
+        (while (re-search-forward "\\s-$" end t)
+          (skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
+          ;; Don't delete formfeeds, even if they are considered whitespace.
+          (save-match-data
+            (if (looking-at ".*\f")
+                (goto-char (match-end 0))))
+          (delete-region (point) (match-end 0)))))))
 
 (defun newline-and-indent ()
   "Insert a newline, then indent according to major mode.

Re: [PATCH] delete-trailing-whitespace on active region

[Stefan Monnier]

> +  (interactive "*r")

Using "r" and then checking region-active-p sounds clever, but is wrong:
- "r" will fail if the mark has never been set in the current buffer.
- checking region-active-p is wrong in non-interactive use.
I.e. the `interactive' spec should do the region-active-p check.
Oh, and it should use use-region-p instead of region-active-p.


        Stefan

Re: [PATCH] delete-trailing-whitespace on active region

[Deniz Dogan]

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

2011/2/10 Stefan Monnier <monnier@iro.umontreal.ca>:
>> +  (interactive "*r")
>
> Using "r" and then checking region-active-p sounds clever, but is wrong:
> - "r" will fail if the mark has never been set in the current buffer.
> - checking region-active-p is wrong in non-interactive use.
> I.e. the `interactive' spec should do the region-active-p check.
> Oh, and it should use use-region-p instead of region-active-p.
>

Thanks for the review

Attached is a reworked patch. Is it better now?

-- 
Deniz Dogan

[-- Attachment #2: dtw2.patch --]
[-- Type: application/octet-stream, Size: 1824 bytes --]

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2011-02-01 21:22:21 +0000
+++ lisp/simple.el	2011-02-10 15:30:53 +0000
@@ -614,22 +614,26 @@
     (if (looking-at "^[ \t]*\n\\'")
 	(delete-region (point) (point-max)))))
 
-(defun delete-trailing-whitespace ()
+(defun delete-trailing-whitespace (&optional start end)
   "Delete all the trailing whitespace across the current buffer.
 All whitespace after the last non-whitespace character in a line is deleted.
 This respects narrowing, created by \\[narrow-to-region] and friends.
-A formfeed is not considered whitespace by this function."
-  (interactive "*")
+A formfeed is not considered whitespace by this function.
+If the region is active, only delete whitespace within the region."
+  (interactive (if (use-region-p)
+                   (list (region-beginning) (region-end))
+                 (list (point-min) (point-max))))
+  (barf-if-buffer-read-only)
   (save-match-data
     (save-excursion
-      (goto-char (point-min))
-      (while (re-search-forward "\\s-$" nil t)
-	(skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
-	;; Don't delete formfeeds, even if they are considered whitespace.
-	(save-match-data
-	  (if (looking-at ".*\f")
-	      (goto-char (match-end 0))))
-	(delete-region (point) (match-end 0))))))
+      (goto-char start)
+      (while (re-search-forward "\\s-$" end t)
+        (skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
+        ;; Don't delete formfeeds, even if they are considered whitespace.
+        (save-match-data
+          (if (looking-at ".*\f")
+              (goto-char (match-end 0))))
+        (delete-region (point) (match-end 0))))))
 
 (defun newline-and-indent ()
   "Insert a newline, then indent according to major mode.

Re: [PATCH] delete-trailing-whitespace on active region

[Stefan Monnier]

> Attached is a reworked patch. Is it better now?

Yes, please install after fixing the details below.

> -- 
> Deniz Dogan

> === modified file 'lisp/simple.el'
> --- lisp/simple.el	2011-02-01 21:22:21 +0000
> +++ lisp/simple.el	2011-02-10 15:30:53 +0000
> @@ -614,22 +614,26 @@
>      (if (looking-at "^[ \t]*\n\\'")
>  	(delete-region (point) (point-max)))))
 
> -(defun delete-trailing-whitespace ()
> +(defun delete-trailing-whitespace (&optional start end)
>    "Delete all the trailing whitespace across the current buffer.
>  All whitespace after the last non-whitespace character in a line is deleted.
>  This respects narrowing, created by \\[narrow-to-region] and friends.
> -A formfeed is not considered whitespace by this function."
> -  (interactive "*")
> +A formfeed is not considered whitespace by this function.
> +If the region is active, only delete whitespace within the region."
> +  (interactive (if (use-region-p)
> +                   (list (region-beginning) (region-end))
> +                 (list (point-min) (point-max))))
> +  (barf-if-buffer-read-only)

Nitpick: the barf-if-buffer-read-only should be in the interactive form.
You can keep passing nil rather than point-min/max when the region is
not used (see below).

>    (save-match-data
>      (save-excursion
> -      (goto-char (point-min))
> -      (while (re-search-forward "\\s-$" nil t)
> -	(skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
> -	;; Don't delete formfeeds, even if they are considered whitespace.
> -	(save-match-data
> -	  (if (looking-at ".*\f")
> -	      (goto-char (match-end 0))))
> -	(delete-region (point) (match-end 0))))))
> +      (goto-char start)

But here you shouldn't assume that start/end won't be nil, since the
function might be called without arguments from Elisp code.


        Stefan

Re: [PATCH] delete-trailing-whitespace on active region

[Johan Bockgård]

Deniz Dogan <deniz.a.m.dogan@gmail.com> writes:

> +      (while (re-search-forward "\\s-$" end t)
> +        (skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
> +        ;; Don't delete formfeeds, even if they are considered whitespace.
> +        (save-match-data
> +          (if (looking-at ".*\f")
> +              (goto-char (match-end 0))))
> +        (delete-region (point) (match-end 0))))))

You can't use the value of `end' to limit the search like that, since
the deletion changes the size of the region. (Use narrowing or a
marker.)

Re: [PATCH] delete-trailing-whitespace on active region

[Stefan Monnier]

>> +      (while (re-search-forward "\\s-$" end t)
>> +        (skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
>> +        ;; Don't delete formfeeds, even if they are considered whitespace.
>> +        (save-match-data
>> +          (if (looking-at ".*\f")
>> +              (goto-char (match-end 0))))
>> +        (delete-region (point) (match-end 0))))))

> You can't use the value of `end' to limit the search like that, since
> the deletion changes the size of the region. (Use narrowing or a
> marker.)

Good point, thanks.  ...or do it from the end.


        Stefan

Re: [PATCH] delete-trailing-whitespace on active region

[Deniz Dogan]

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

2011/2/11 Stefan Monnier <monnier@iro.umontreal.ca>:
>>> +      (while (re-search-forward "\\s-$" end t)
>>> +        (skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
>>> +        ;; Don't delete formfeeds, even if they are considered whitespace.
>>> +        (save-match-data
>>> +          (if (looking-at ".*\f")
>>> +              (goto-char (match-end 0))))
>>> +        (delete-region (point) (match-end 0))))))
>
>> You can't use the value of `end' to limit the search like that, since
>> the deletion changes the size of the region. (Use narrowing or a
>> marker.)
>
> Good point, thanks.  ...or do it from the end.
>

This patch is teaching me a lot...

Attached is a last attempt, if it's not good then I give up and hope
that someone else will take care of it.

Thanks,
Deniz Dogan

[-- Attachment #2: dtw3.patch --]
[-- Type: application/octet-stream, Size: 2037 bytes --]

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2011-02-01 21:22:21 +0000
+++ lisp/simple.el	2011-02-11 06:31:41 +0000
@@ -614,22 +614,30 @@
     (if (looking-at "^[ \t]*\n\\'")
 	(delete-region (point) (point-max)))))
 
-(defun delete-trailing-whitespace ()
+(defun delete-trailing-whitespace (&optional start end)
   "Delete all the trailing whitespace across the current buffer.
 All whitespace after the last non-whitespace character in a line is deleted.
 This respects narrowing, created by \\[narrow-to-region] and friends.
-A formfeed is not considered whitespace by this function."
-  (interactive "*")
+A formfeed is not considered whitespace by this function.
+If the region is active, only delete whitespace within the region."
+  (interactive (progn
+                 (barf-if-buffer-read-only)
+                 (if (use-region-p)
+                     (list (region-beginning) (region-end))
+                   (list nil nil))))
   (save-match-data
     (save-excursion
-      (goto-char (point-min))
-      (while (re-search-forward "\\s-$" nil t)
-	(skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
-	;; Don't delete formfeeds, even if they are considered whitespace.
-	(save-match-data
-	  (if (looking-at ".*\f")
-	      (goto-char (match-end 0))))
-	(delete-region (point) (match-end 0))))))
+      (let ((end-marker (set-marker (make-marker) (or end (point-max))))
+            (start (or start (point-min))))
+        (goto-char start)
+        (while (re-search-forward "\\s-$" end-marker t)
+          (skip-syntax-backward "-" (save-excursion (forward-line 0) (point)))
+          ;; Don't delete formfeeds, even if they are considered whitespace.
+          (save-match-data
+            (if (looking-at ".*\f")
+                (goto-char (match-end 0))))
+          (delete-region (point) (match-end 0)))
+        (set-marker end-marker nil)))))
 
 (defun newline-and-indent ()
   "Insert a newline, then indent according to major mode.

Re: [PATCH] delete-trailing-whitespace on active region

[Stefan Monnier]

> Attached is a last attempt, if it's not good then I give up and hope
> that someone else will take care of it.

Looks good, please install.

> +      (let ((end-marker (set-marker (make-marker) (or end (point-max))))

I'd use (copy-marker (or end (point-max))).


        Stefan "Actually, I'm tempted to mark make-marker as obsolete"

Re: [PATCH] delete-trailing-whitespace on active region

[Deniz Dogan]

2011/2/11 Stefan Monnier <monnier@iro.umontreal.ca>:
>> Attached is a last attempt, if it's not good then I give up and hope
>> that someone else will take care of it.
>
> Looks good, please install.
>
>> +      (let ((end-marker (set-marker (make-marker) (or end (point-max))))
>
> I'd use (copy-marker (or end (point-max))).
>

Thanks, I made that change and pushed.