Bug/Patch view.el

Bug/Patch view.el

[Jonathan Goldblatt]

Here is a little test script that demonstrates some problems in
view-search-no-match-lines followed by a patch to replace it.

Just run the form to see the anomalies.  It generates a file that
contains many lines "abc" with a very few "123" lines
interspered.  If you do /!a looking for a non-abc line you wind
up with the highlight on the line after the 123 line.  If you do
it twice and then try to go backwards with a p, the previous hit
isn't found at all??

The test script also shows problems with sit-for in a windowed
environment.  If you just do a sit-for it doesn't wait at all.
It seems like the work-around of doing a short sit-for followed
by another sit-for seems to wait much longer than the specified
time.  Also tried to use display-buffer with a specified frame
and didn't get the test buffer showing up in the new frame??

This was on 

Linux version 2.4.21 (root@xeon.localdomain) (gcc version 3.2.3
20030316 (Debian prerelease)) #1 Sat Mar 13 13:25:10 EST 2004

GNU Emacs 21.4.1 (i386-pc-linux-gnu, X toolkit, Xaw3d scroll
bars) of 2005-03-17 on trouble, modified by Debian


Hope this helps.

Test script:

(let* ((test-buffer (generate-new-buffer "test"))
       (test-frame
	(let ((pop-up-frames t))
	  (pop-to-buffer test-buffer 't)
	  (window-frame (selected-window))))
       (i 1)
       (j 1))
      (progn
	(with-current-buffer test-buffer
	  (while (< j 4)
	    (while (< i 200)
	      (insert "abc\n")
	      (setq i (1+ i)))
	    (insert "123\n")
	    (setq i 1)
	    (setq j (1+ j)))
	  (view-mode)
	  (goto-char (point-min))
	  (View-search-regexp-forward 1 "!a")
	  (message
"Simulated /!a.in view mode.
Notice highlight one line too far.
Line 200 is \"123\"
Press any key to continue.")
	  (sit-for 1)
	  (if (not(sit-for 60))
	  (read-char))
	  (View-search-last-regexp-forward 1)
	  (message
"Simulated n.
Notice highlight one line too far.
Current line is 401.
Press any key to continue.")
	  (sit-for 1)
	  (if (not(sit-for 60))
	      (read-char))
	  (View-search-last-regexp-backward 1)
	  (sit-for 1)
	  (if (not(sit-for 5))
	      (read-char))
	  (message
"That was a simulated p.
Notice can't find line 200?!.
Press any key to end.")
	  (sit-for 1)
	  (if (not(sit-for 60))
	      (read-char))
	  (if (y-or-n-p "Remove buffer?")
	      (kill-buffer test-buffer))
	  (if (y-or-n-p "Remove frame?")
	      (delete-frame test-frame)))))

Here's the patch:


2007-09-12  Jonathan Goldblatt  <jonathangoldblatt@yahoo.com>


	* view.el (view-search-no-match-lines): Rewrote to work
          in more, possibly all, cases.  

*** /usr/share/emacs/21.4/lisp/view.el	Tue Sep  4 08:52:36 2001
--- /home/jonathan/.emacs.d/lisp/view.el	Thu Sep  6 16:37:46 2007
***************
*** 972,995 ****
  
  (defun view-search-no-match-lines (times regexp)
    ;; Search for the TIMESt occurrence of line with no match for REGEXP.
!   (let ((back (and (< times 0) (setq times (- times)) -1))
! 	n)
!     (while (> times 0)
!       (save-excursion (beginning-of-line (if back (- times) (1+ times)))
! 		      (setq n (point)))
!       (setq times
! 	    (cond
! 	     ((< (count-lines (point) n) times) -1) ; Not enough lines.
! 	     ((or (null (re-search-forward regexp nil t back))
! 		  (if back (and (< (match-end 0) n)
! 				(> (count-lines (match-end 0) n) 1))
! 		    (and (< n (match-beginning 0))
! 			 (> (count-lines n (match-beginning 0)) 1))))
! 	      0)			; No match within lines.
! 	     (back (count-lines (max n (match-beginning 0)) (match-end 0)))
! 	     (t (count-lines (match-beginning 0) (min n (match-end 0))))))
!       (goto-char n))
!     (and (zerop times) (looking-at "^.*$"))))
  
  
  (provide 'view)
--- 987,1026 ----
  
  (defun view-search-no-match-lines (times regexp)
    ;; Search for the TIMESt occurrence of line with no match for REGEXP.
!   ;; times > 0 for forward search; < 0 for backward search
!   (let ((count (abs times))
! 	(hit-count 0)
!  	(fwd-one			;parm used by forward-line to
! 					;move 1 line in search
! 					;direction
! 	 (if (> times 0) 1 -1))
!  	(bkwd-one			;parm used by forward-line to
! 					;move 1 line in opposite
! 					;direction of search
! 	 (if (> times 0) -1 1))
! 	end-search
! 	searching)
!     (while (> count 0)
!       (setq searching 't)
!       (while searching
! 	(forward-line fwd-one)
! 	(if (/= 0 (forward-line fwd-one)) ;not bob or eob
! 	      (setq searching 'nil
! 		    count 0)
! 	  (forward-line bkwd-one)
! 	  (forward-line 1)
! 	  (setq end-search (point))
! 	  (forward-line -1)		;reset point
! 	  (setq searching (re-search-forward regexp end-search 't))
! 	  (if (not searching)
! 	      (progn
! 		(forward-line 0)
! 		(setq hit-count (1+ hit-count))))))
!       (setq count (1- count)))
!     (re-search-forward "^.*\n" (point-max) 't) ; setup to highlight
! 					       ; line
!     (forward-line -1)			; reset point after search
!     (= hit-count (abs times))))
  
  
  (provide 'view)

Re: Bug/Patch view.el

[Glenn Morris]

Jonathan Goldblatt wrote:

> Here is a little test script that demonstrates some problems in
> view-search-no-match-lines followed by a patch to replace it.

Thanks for the detailed report. Can't your version be simplified as
follows? I gave up trying to understand the original.

(defun view-search-no-match-lines (times regexp)
  (let ((fwd-one (if (< times 0) -1 1)))
    (setq times (abs times))
    (while (and (> times 0)
                (zerop (forward-line fwd-one)))
      (if (eobp)
          (setq times -1)
        (or (re-search-forward regexp (line-end-position) t)
            (setq times (1- times)))))
    (and (zerop times)
         (looking-at "^.*$"))))

Re: Bug/Patch view.el

[Jonathan Goldblatt]

X-Mailer: VM 7.18 under Emacs 21.4.1
FCC: /server/home/jonathan/Mail/correspondance/correspondance.gz
From: jonathangoldblatt@yahoo.com
--text follows this line--
>>>>> "GM" == Glenn Morris <rgm@gnu.org> writes:

GM> Thanks for the detailed report. 

You're welcome.

GM> Can't your version be simplified as follows?

Your code is so much cleverer and exhibits so far a greater
knowledge of elisp than mine that it doesn't seem appropriate to
describe it as a simplification; I'd describe it as a greatly
improved reimplementation of view-search-no-match-lines.
However, it doesn't seem to test bobp in the place where it tests
eobp, which would concern me, since the function searches both
forward and backward and it also doesn't seem to be concerned
with its return value which would also be a source of concern to
me, as I believe the caller uses the value.

I tested my implementation extensively enough that I'm convinced
that it works in "more, possibly all, cases," and I find it much
easier to test my own code than the code of others.  It's
possible that I will add the greater knowledge of elisp that I
have gained by reading your code into what I wrote to preserve
the investment that I have in testing, and add in comments to
make it clearer; however, it seemed straightforward to me.

Is there any real benefit to combining the loop over times with
the loop looking for a non-matching line?  I found that to be a
confusing to me.  I also found the short-circuiting of if tests by
using conditionals a bit confusing, and have the same question
about this practice.


GM> I gave up trying to understand the original.

Please forgive the obscurity of my code.  I thank you for your
comment.  

GM> (defun view-search-no-match-lines (times regexp)
GM>   (let ((fwd-one (if (< times 0) -1 1)))
GM>     (setq times (abs times))
GM>     (while (and (> times 0)
GM>                 (zerop (forward-line fwd-one)))
GM>       (if (eobp)
GM>           (setq times -1)
GM>         (or (re-search-forward regexp (line-end-position) t)
GM>             (setq times (1- times)))))
GM>     (and (zerop times)
GM>          (looking-at "^.*$"))))

Re: Bug/Patch view.el

[Glenn Morris]

Jonathan Goldblatt wrote:

> However, it doesn't seem to test bobp in the place where it tests
> eobp

There's no need. forward-line returns a non-zero value on reaching the
start of the buffer. It does at the end too, but with a slight
difference, as noted in the doc string:

  With positive N, a non-empty line at the end counts as one line
  successfully moved (for the return value).

Basically (forward-line -1) called anywhere on the first line in a
buffer returns non-zero. (forward-line 1) called on the last line,
before the end, moves you to eob and returns 0. Hence the need for an
extra eobp test to avoid a meaningless search and a false match.

> forward and backward and it also doesn't seem to be concerned with
> its return value which would also be a source of concern to me, as I
> believe the caller uses the value.

It returns the same value as the original: nil/t for failure/success.

> Is there any real benefit to combining the loop over times with the
> loop looking for a non-matching line?

Efficiency.

> I also found the short-circuiting of if tests by using conditionals
> a bit confusing, and have the same question about this practice.

Just personal preference, I think.

> GM> I gave up trying to understand the original.
>
> Please forgive the obscurity of my code.

I actually meant the original version in the file, not your version!
Sorry. The original looked like it was trying to do something clever,
but I couldn't figure it out...

Re: Bug/Patch view.el

[Glenn Morris]

Eventually I settled on this version, which reduces the number of
operations in the loop.

(defun view-search-no-match-lines (times regexp)
  "Search for the TIMESth occurrence of a line with no match for REGEXP.
If such a line is found, return non-nil and set the match-data to that line.
If TIMES is negative, search backwards."
  (let ((step 1)
        (noerror 'move))
    (when (< times 0)
      (setq times (- times)
            step -1
            noerror t))
    ;; Note that we do not check the current line.
    (while (and (> times 0)
                (zerop (forward-line step)))
      ;; Move only to handle eob in the forward case: on last line,
      ;; (forward-line 1) returns 0 before the end of line.
      (or (re-search-forward regexp (line-end-position) noerror)
          (setq times (1- times)))))
  (when (zerop times)
    (forward-line 0)
    (looking-at ".*")))

Re: Bug/Patch view.el

[Glenn Morris]

Glenn Morris wrote:

> Eventually I settled on this version, which reduces the number of
> operations in the loop.

Whoops, that can go wrong, so I went back to the previous version.