unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* doc-view cache file permissions
@ 2007-10-30 18:50 Glenn Morris
  2007-10-30 20:11 ` Stefan Monnier
  2007-10-30 20:34 ` Tassilo Horn
  0 siblings, 2 replies; 32+ messages in thread
From: Glenn Morris @ 2007-10-30 18:50 UTC (permalink / raw)
  To: Tassilo Horn; +Cc: emacs-devel


doc-view creates world-readable image files in /tmp from private
documents:

chmod 600 foo.pdf
emacs -Q foo.pdf

-> /tmp/doc-view/foo.pdf-MD5/page-1.png is world-readable

Is there any reason to not just make the top-level cache directory
private?


Also, it seems to use `concat' to make a lot of file names, when
something like `expand-file-name' would probably be better.

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

* Re: doc-view cache file permissions
  2007-10-30 18:50 doc-view cache file permissions Glenn Morris
@ 2007-10-30 20:11 ` Stefan Monnier
  2007-10-30 20:57   ` Glenn Morris
  2007-10-30 20:34 ` Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2007-10-30 20:11 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Tassilo Horn, emacs-devel

> doc-view creates world-readable image files in /tmp from private
> documents:

> chmod 600 foo.pdf
> emacs -Q foo.pdf

-> /tmp/doc-view/foo.pdf-MD5/page-1.png is world-readable

> Is there any reason to not just make the top-level cache directory
> private?

Oh, I didn't catch this one.  I did sent him a minute ago a patch to try
and fix other security issues in the handling of temp files.

> Also, it seems to use `concat' to make a lot of file names, when
> something like `expand-file-name' would probably be better.

The patch I just sent him fixes those indeed.


        Stefan


Index: doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.13
diff -u -u -b -r1.13 doc-view.el
--- doc-view.el	30 Oct 2007 17:45:40 -0000	1.13
+++ doc-view.el	30 Oct 2007 20:01:24 -0000
@@ -103,9 +103,10 @@
 ;; Todo:
 ;; - better menu.
 ;; - don't use `find-file'.
-;; - `reload' without changing the slicing.
 ;; - Bind slicing to a drag event.
-;; - zoom
+;; - zoom (the whole document and/or just the region around the cursor).
+;; - get rid of the silly arrow in the fringe.
+;; - improve anti-aliasing (pdf-utils gets it better).
 
 (require 'dired)
 (require 'image-mode)
@@ -156,8 +157,8 @@
   :type 'file
   :group 'doc-view)
 
-(defcustom doc-view-cache-directory (concat temporary-file-directory
-					    "doc-view")
+(defcustom doc-view-cache-directory
+  (expand-file-name (concat "docview" (user-uid)) temporary-file-directory)
   "The base directory, where the PNG images will be saved."
   :type 'directory
   :group 'doc-view)
@@ -201,6 +202,8 @@
 
 (defvar doc-view-current-image nil
   "Only used internally.")
+(defvar doc-view-current-overlay)
+(defvar doc-view-pending-cache-flush nil)
 
 (defvar doc-view-current-info nil
   "Only used internally.")
@@ -303,16 +306,14 @@
 			 (setq contexts (concat contexts "  - \"" m "\"\n")))
 		       contexts)))))
     ;; Update the buffer
-    (let ((inhibit-read-only t))
-      (erase-buffer)
-      (let ((beg (point)))
 	(doc-view-insert-image (nth (1- page) doc-view-current-files)
 			       :pointer 'arrow)
-	(put-text-property beg (point) 'help-echo doc-view-current-info))
-      (insert "\n" doc-view-current-info)
+    (overlay-put doc-view-current-overlay 'help-echo doc-view-current-info)
       (goto-char (point-min))
-      (forward-char))
-    (set-buffer-modified-p nil)))
+    ;; This seems to be needed for set-window-hscroll (in
+    ;; image-forward-hscroll) to do something useful, I don't have time to
+    ;; debug this now.  :-(  --Stef
+    (forward-char)))
 
 (defun doc-view-next-page (&optional arg)
   "Browse ARG pages forward."
@@ -374,15 +375,30 @@
 It's a subdirectory of `doc-view-cache-directory'."
   (if doc-view-current-cache-dir
       doc-view-current-cache-dir
+    ;; Try and make sure doc-view-cache-directory exists and is safe.
+    (condition-case nil
+        (make-directory doc-view-cache-directory)
+      (file-already-exists
+       (cond
+        ((file-symlink-p doc-view-cache-directory)
+         (error "Danger: doc-view-cache-directory points to a symbolic link"))
+        ((not (file-directory-p doc-view-cache-directory))
+         (error "doc-view-cache-directory is not a directory"))
+        ((not (file-writable-p doc-view-cache-directory))
+         (error "Cannot write to doc-view-cache-directory"))
+        ((not (= (user-uid) (nth 2 (file-attributes doc-view-cache-directory))))
+         (error "Danger: doc-view-cache-directory does not belong to us")))))
+    ;; Now compute the subdirectory to use.
     (setq doc-view-current-cache-dir
 	  (file-name-as-directory
-	   (concat (file-name-as-directory doc-view-cache-directory)
+	   (expand-file-name
 		   (let ((doc buffer-file-name))
 		     (concat (file-name-nondirectory doc)
 			     "-"
 			     (with-temp-buffer
 			       (insert-file-contents-literally doc)
-			       (md5 (current-buffer))))))))))
+                        (md5 (current-buffer)))))
+            doc-view-cache-directory)))))
 
 (defun doc-view-remove-if (predicate list)
   "Return LIST with all items removed that satisfy PREDICATE."
@@ -393,7 +409,7 @@
 
 ;;;; Conversion Functions
 
-(defun doc-view-reconvert-doc (&rest args)
+(defun doc-view-reconvert-doc ()
   "Reconvert the current document.
 Should be invoked when the cached images aren't up-to-date."
   (interactive)
@@ -401,7 +417,7 @@
   ;; Clear the old cached files
   (when (file-exists-p (doc-view-current-cache-dir))
     (dired-delete-file (doc-view-current-cache-dir) 'always))
-  (doc-view-mode))
+  (doc-view-initiate-display))
 
 (defun doc-view-dvi->pdf-sentinel (proc event)
   "If DVI->PDF conversion was successful, convert the PDF to PNG now."
@@ -412,8 +428,8 @@
 	  mode-line-process nil)
     ;; Now go on converting this PDF to a set of PNG files.
     (let* ((pdf (process-get proc 'pdf-file))
-	   (png (concat (doc-view-current-cache-dir)
-			"page-%d.png")))
+	   (png (expand-file-name "page-%d.png"
+                                  (doc-view-current-cache-dir))))
       (doc-view-pdf/ps->png pdf png))))
 
 (defun doc-view-dvi->pdf (dvi pdf)
@@ -493,8 +509,8 @@
 	  mode-line-process nil)
     ;; Now we can transform to plain text.
     (doc-view-pdf->txt (process-get proc 'pdf-file)
-		       (concat (doc-view-current-cache-dir)
-			       "doc.txt"))))
+		       (expand-file-name "doc.txt"
+                                         (doc-view-current-cache-dir)))))
 
 (defun doc-view-ps->pdf (ps pdf)
   "Convert PS to PDF asynchronously."
@@ -516,18 +532,23 @@
   "Convert `buffer-file-name' to a set of png files, one file per page.
 Those files are saved in the directory given by the function
 `doc-view-current-cache-dir'."
-  (clear-image-cache)
-  (let ((png-file (concat (doc-view-current-cache-dir)
-			  "page-%d.png")))
-    (make-directory (doc-view-current-cache-dir) t)
+  ;; Let stale files still display while we recompute the new ones, so only
+  ;; flush the cache when the conversion is over.  One of the reasons why it
+  ;; is important to keep displaying the stale page is so that revert-buffer
+  ;; preserves the horizontal/vertical scroll settings (which are otherwise
+  ;; resets during the redisplay).
+  (setq doc-view-pending-cache-flush t)
+  (let ((png-file (expand-file-name "page-%d.png"
+                                    (doc-view-current-cache-dir))))
+    (make-directory (doc-view-current-cache-dir))
     (if (not (string= (file-name-extension buffer-file-name) "dvi"))
 	;; Convert to PNG images.
 	(doc-view-pdf/ps->png buffer-file-name png-file)
       ;; DVI files have to be converted to PDF before Ghostscript can process
       ;; it.
       (doc-view-dvi->pdf buffer-file-name
-			 (concat (file-name-as-directory doc-view-current-cache-dir)
-				 "doc.pdf")))))
+			 (expand-file-name "doc.pdf"
+                                           doc-view-current-cache-dir)))))
 
 ;;;; Slicing
 
@@ -583,9 +604,16 @@
 (defun doc-view-insert-image (file &rest args)
   "Insert the given png FILE.
 ARGS is a list of image descriptors."
+  (when doc-view-pending-cache-flush
+    (clear-image-cache)
+    (setq doc-view-pending-cache-flush nil))
   (let ((image (apply 'create-image file 'png nil args)))
     (setq doc-view-current-image image)
-    (insert-image image (concat "[" file "]") nil doc-view-current-slice)))
+    (move-overlay doc-view-current-overlay (point-min) (point-max))
+    (overlay-put doc-view-current-overlay 'display
+                 (if doc-view-current-slice
+                     (list (cons 'slice doc-view-current-slice) image)
+                   image))))
 
 (defun doc-view-sort (a b)
   "Return non-nil if A should be sorted before B.
@@ -605,7 +633,12 @@
     (doc-view-goto-page doc-view-current-page)))
 
 (defun doc-view-buffer-message ()
-  (insert (propertize "Welcome to DocView!" 'face 'bold)
+  ;; Only show this message initially, not when refreshing the buffer (in which
+  ;; case it's better to keep displaying the "stale" page while computing
+  ;; the fresh new ones).
+  (unless (overlay-get doc-view-current-overlay 'display)
+    (overlay-put doc-view-current-overlay 'display
+                 (concat (propertize "Welcome to DocView!" 'face 'bold)
           "\n"
           "
 If you see this buffer it means  that the document you want to view is being
@@ -616,7 +649,7 @@
 
 `q' : Bury this buffer.  Conversion will go on in background.
 `k' : Kill the conversion process and this buffer.
-`K' : Kill the conversion process.\n"))
+`K' : Kill the conversion process.\n"))))
 
 (defun doc-view-show-tooltip ()
   (interactive)
@@ -632,20 +665,17 @@
       (progn
 	(doc-view-kill-proc)
 	(setq buffer-read-only nil)
-	(erase-buffer)
-	(insert-file-contents buffer-file-name)
+        (delete-overlay doc-view-current-overlay)
 	;; Switch to the previously used major mode or fall back to fundamental
 	;; mode.
 	(if doc-view-previous-major-mode
 	    (funcall doc-view-previous-major-mode)
 	  (fundamental-mode))
-	(doc-view-minor-mode 1)
-	(set-buffer-modified-p nil))
+	(doc-view-minor-mode 1))
     ;; Switch to doc-view-mode
     (when (and (buffer-modified-p)
 	       (y-or-n-p "The buffer has been modified.  Save the changes? "))
       (save-buffer))
-    (erase-buffer)
     (doc-view-mode)))
 
 ;;;; Searching
@@ -664,11 +694,11 @@
 	(when (match-string 1) (incf page))
 	(when (match-string 2)
 	  (if (/= page lastpage)
-	      (setq matches (push (cons page
+	      (push (cons page
 					(list (buffer-substring
 					       (line-beginning-position)
 					       (line-end-position))))
-				  matches))
+                    matches)
 	    (setq matches (cons
 			   (append
 			    (or
@@ -698,8 +728,8 @@
   (interactive)
   ;; New search, so forget the old results.
   (setq doc-view-current-search-matches nil)
-  (let ((txt (concat (doc-view-current-cache-dir)
-		     "doc.txt")))
+  (let ((txt (expand-file-name "doc.txt"
+                               (doc-view-current-cache-dir))))
     (if (file-readable-p txt)
 	(progn
 	  (setq doc-view-current-search-matches
@@ -721,13 +751,13 @@
 	    ;; Doc is a PS, so convert it to PDF (which will be converted to
 	    ;; TXT thereafter).
 	    (doc-view-ps->pdf buffer-file-name
-			      (concat (doc-view-current-cache-dir)
-				      "doc.pdf")))
+			      (expand-file-name "doc.pdf"
+                                                (doc-view-current-cache-dir))))
 	   ((string= ext "dvi")
 	    ;; Doc is a DVI.  This means that a doc.pdf already exists in its
 	    ;; cache subdirectory.
-	    (doc-view-pdf->txt (concat (doc-view-current-cache-dir)
-				       "doc.pdf")
+	    (doc-view-pdf->txt (expand-file-name "doc.pdf"
+                                                 (doc-view-current-cache-dir))
 			       txt))
 	   (t (error "DocView doesn't know what to do"))))))))
 
@@ -761,7 +791,30 @@
 
 ;;;; User interface commands and the mode
 
-(put 'doc-view-mode 'mode-class 'special)
+;; (put 'doc-view-mode 'mode-class 'special)
+
+(defun doc-view-initiate-display ()
+  ;; Switch to image display if possible
+  (if (and (display-images-p)
+	   (image-type-available-p 'png))
+      (progn
+	(doc-view-buffer-message)
+	(setq doc-view-current-page (or doc-view-current-page 1))
+	(if (file-exists-p (doc-view-current-cache-dir))
+	    (progn
+	      (message "DocView: using cached files!")
+	      (doc-view-display buffer-file-name))
+	  (doc-view-convert-current-doc))
+	(message
+	 "%s"
+	 (substitute-command-keys
+	  (concat "Type \\[doc-view-toggle-display] to toggle between "
+		  "editing or viewing the document."))))
+    (message
+     "%s"
+     (substitute-command-keys
+      (concat "No image (png) support available.  Type \\[doc-view-toggle-display] "
+	      "to switch to an editing mode.")))))
 
 ;;;###autoload
 (defun doc-view-mode ()
@@ -783,37 +836,22 @@
   (make-local-variable 'doc-view-current-cache-dir)
   (make-local-variable 'doc-view-current-info)
   (make-local-variable 'doc-view-current-search-matches)
-  ;; The file should already be in the current buffer.  --Stef
-  ;; (insert-file-contents buffer-file-name)
+  (set (make-local-variable 'doc-view-current-overlay)
+       (make-overlay (point-min) (point-max) nil t))
+  (add-hook 'change-major-mode-hook
+            (lambda () (delete-overlay doc-view-current-overlay))
+            nil t)
+  (set (make-local-variable 'mode-line-position)
+       '(" P" (:eval (number-to-string doc-view-current-page))
+         "/" (:eval (number-to-string (length doc-view-current-files)))))
+  (set (make-local-variable 'cursor-type) nil)
   (use-local-map doc-view-mode-map)
-  (set (make-local-variable 'revert-buffer-function) 'doc-view-reconvert-doc)
+  (set (make-local-variable 'after-revert-hook) 'doc-view-reconvert-doc)
   (setq mode-name "DocView"
 	buffer-read-only t
 	major-mode 'doc-view-mode)
-  ;; Switch to image display if possible
-  (if (and (display-images-p)
-	   (image-type-available-p 'png))
-      (let ((inhibit-read-only t))
-	(erase-buffer)
-	(doc-view-buffer-message)
-        (set-buffer-modified-p nil)
-	(setq doc-view-current-page (or doc-view-current-page 1))
-	(if (file-exists-p (doc-view-current-cache-dir))
-	    (progn
-	      (message "DocView: using cached files!")
-	      (doc-view-display buffer-file-name))
-	  (doc-view-convert-current-doc))
-	(use-local-map doc-view-mode-map)
-	(message
-	 "%s"
-	 (substitute-command-keys
-	  (concat "Type \\[doc-view-toggle-display] to toggle between "
-		  "editing or viewing the document."))))
-    (message
-     "%s"
-     (substitute-command-keys
-      (concat "No image (png) support available.  Type \\[doc-view-toggle-display] "
-	      "to switch to an editing mode.")))))
+  (doc-view-initiate-display)
+  (run-mode-hooks 'doc-view-mode-hook))
 
 ;;;###autoload
 (define-minor-mode doc-view-minor-mode


Diffs between working revision and workfile end here.

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

* Re: doc-view cache file permissions
  2007-10-30 18:50 doc-view cache file permissions Glenn Morris
  2007-10-30 20:11 ` Stefan Monnier
@ 2007-10-30 20:34 ` Tassilo Horn
  1 sibling, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-10-30 20:34 UTC (permalink / raw)
  To: emacs-devel

Glenn Morris <rgm@gnu.org> writes:

> doc-view creates world-readable image files in /tmp from private
> documents:
>
> chmod 600 foo.pdf
> emacs -Q foo.pdf
>
> -> /tmp/doc-view/foo.pdf-MD5/page-1.png is world-readable
>
> Is there any reason to not just make the top-level cache directory
> private?
>
> Also, it seems to use `concat' to make a lot of file names, when
> something like `expand-file-name' would probably be better.

While I was fixing those issues and testing my changes a patch from
Stefan arrived here which goes far beyond that.  So well take Stefan's
patch and I'll revert my changes.  This guy seems to be on steroids. ;-)

Bye,
Tassilo

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

* Re: doc-view cache file permissions
  2007-10-30 20:11 ` Stefan Monnier
@ 2007-10-30 20:57   ` Glenn Morris
  2007-10-30 21:56     ` Stefan Monnier
  2007-10-30 22:14     ` doc-view cache file permissions Reiner Steib
  0 siblings, 2 replies; 32+ messages in thread
From: Glenn Morris @ 2007-10-30 20:57 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Tassilo Horn, emacs-devel

Stefan Monnier wrote:

> +    ;; Try and make sure doc-view-cache-directory exists and is safe.
> +    (condition-case nil
> +        (make-directory doc-view-cache-directory)
> +      (file-already-exists
> +       (cond
> +        ((file-symlink-p doc-view-cache-directory)
> +         (error "Danger: doc-view-cache-directory points to a symbolic link"))
> +        ((not (file-directory-p doc-view-cache-directory))
> +         (error "doc-view-cache-directory is not a directory"))
> +        ((not (file-writable-p doc-view-cache-directory))
> +         (error "Cannot write to doc-view-cache-directory"))
> +        ((not (= (user-uid) (nth 2 (file-attributes doc-view-cache-directory))))
> +         (error "Danger: doc-view-cache-directory does not belong to us")))))

Is that worthy of being the basis of a more generally available
`make-secure-directory' function?

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

* Re: doc-view cache file permissions
  2007-10-30 20:57   ` Glenn Morris
@ 2007-10-30 21:56     ` Stefan Monnier
  2007-10-31  7:22       ` Tassilo Horn
  2007-10-30 22:14     ` doc-view cache file permissions Reiner Steib
  1 sibling, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2007-10-30 21:56 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Tassilo Horn, emacs-devel

>> +    ;; Try and make sure doc-view-cache-directory exists and is safe.
>> +    (condition-case nil
>> +        (make-directory doc-view-cache-directory)
>> +      (file-already-exists
>> +       (cond
>> +        ((file-symlink-p doc-view-cache-directory)
>> +         (error "Danger: doc-view-cache-directory points to a symbolic link"))
>> +        ((not (file-directory-p doc-view-cache-directory))
>> +         (error "doc-view-cache-directory is not a directory"))
>> +        ((not (file-writable-p doc-view-cache-directory))
>> +         (error "Cannot write to doc-view-cache-directory"))
>> +        ((not (= (user-uid) (nth 2 (file-attributes doc-view-cache-directory))))
>> +         (error "Danger: doc-view-cache-directory does not belong to us")))))

> Is that worthy of being the basis of a more generally available
> `make-secure-directory' function?

Could be.  Although I might want to call it "make directory-secure"
since the point is to be able to use a "known" directory which may
already exist.  Note that the code I just installed is simpler (and
safer).  Also I'm not 100% that the code is actually safe.  I'm pretty
sure it's safer.

Otherwise, if you really want a brand new directory, you can use
make-temp-file.


        Stefan

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

* Re: doc-view cache file permissions
  2007-10-30 20:57   ` Glenn Morris
  2007-10-30 21:56     ` Stefan Monnier
@ 2007-10-30 22:14     ` Reiner Steib
  2007-10-31  0:52       ` Stefan Monnier
  1 sibling, 1 reply; 32+ messages in thread
From: Reiner Steib @ 2007-10-30 22:14 UTC (permalink / raw)
  To: emacs-devel

On Tue, Oct 30 2007, Glenn Morris wrote:

> Stefan Monnier wrote:
>> +    ;; Try and make sure doc-view-cache-directory exists and is safe.
[...]
> Is that worthy of being the basis of a more generally available
> `make-secure-directory' function?

Isn't `make-temp-file' with `dir-flag' set supposed to create a secure
directory?

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

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

* Re: doc-view cache file permissions
  2007-10-30 22:14     ` doc-view cache file permissions Reiner Steib
@ 2007-10-31  0:52       ` Stefan Monnier
  0 siblings, 0 replies; 32+ messages in thread
From: Stefan Monnier @ 2007-10-31  0:52 UTC (permalink / raw)
  To: emacs-devel

>>> +    ;; Try and make sure doc-view-cache-directory exists and is safe.
> [...]
>> Is that worthy of being the basis of a more generally available
>> `make-secure-directory' function?

> Isn't `make-temp-file' with `dir-flag' set supposed to create a secure
> directory?

Yes, indeed.  But the author of doc-view decided to try and keep the png
images (generated from the .pdf or .dvi file) and reuse them across
invocations if the file hasn't changed in between.
So the "temporary" directory needs to be at a well-known location, so
make-temp-file can't be used.


        Stefan

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

* Re: doc-view cache file permissions
  2007-10-30 21:56     ` Stefan Monnier
@ 2007-10-31  7:22       ` Tassilo Horn
  2007-10-31  7:56         ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-10-31  7:22 UTC (permalink / raw)
  To: emacs-devel

Hi Stefan,

I really like your changes, they make doc-view even more convenient.
But there's a little bug.  This patch fixes it.

--8<---------------cut here---------------start------------->8---
2007-10-31  Tassilo Horn  <tassilo@member.fsf.org>

	* doc-view.el (doc-view-cache-directory): Fix bug where an integer
	was given to concat.
--8<---------------cut here---------------end--------------->8---


--8<---------------cut here---------------start------------->8---
Index: lisp/doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.15
diff -u -r1.15 doc-view.el
--- lisp/doc-view.el	31 Oct 2007 03:10:32 -0000	1.15
+++ lisp/doc-view.el	31 Oct 2007 07:21:34 -0000
@@ -157,7 +157,8 @@
   :group 'doc-view)
 
 (defcustom doc-view-cache-directory
-  (expand-file-name (concat "docview" (user-uid)) temporary-file-directory)
+  (expand-file-name (concat "docview" (format "%d" (user-uid)))
+		    temporary-file-directory)
   "The base directory, where the PNG images will be saved."
   :type 'directory
   :group 'doc-view)
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

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

* Re: doc-view cache file permissions
  2007-10-31  7:22       ` Tassilo Horn
@ 2007-10-31  7:56         ` Tassilo Horn
  2007-10-31 15:10           ` Stefan Monnier
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-10-31  7:56 UTC (permalink / raw)
  To: emacs-devel

Hi again,

furthermore I noticed that `g' is bound twice in d-v-mode-map.  I
changed the bindings so that `g' is bound to d-v-goto-page and `r' is
bound to d-v-reconvert-doc now.

--8<---------------cut here---------------start------------->8---
2007-10-31  Tassilo Horn  <tassilo@member.fsf.org>

	* doc-view.el (doc-view-cache-directory): Fix bug where an integer
	was given to concat.
	(doc-view-mode-map): Now g is bound to d-v-goto-page and r is
	bound to d-v-reconvert-doc.  Before g was bound to both so the
	former was inaccessible.
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
Index: lisp/doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.15
diff -u -r1.15 doc-view.el
--- lisp/doc-view.el	31 Oct 2007 03:10:32 -0000	1.15
+++ lisp/doc-view.el	31 Oct 2007 07:55:58 -0000
@@ -157,7 +157,8 @@
   :group 'doc-view)
 
 (defcustom doc-view-cache-directory
-  (expand-file-name (concat "docview" (user-uid)) temporary-file-directory)
+  (expand-file-name (concat "docview" (format "%d" (user-uid)))
+		    temporary-file-directory)
   "The base directory, where the PNG images will be saved."
   :type 'directory
   :group 'doc-view)
@@ -250,7 +251,7 @@
     ;; Toggle between text and image display or editing
     (define-key map (kbd "C-c C-c")   'doc-view-toggle-display)
     ;; Reconvert the current document
-    (define-key map (kbd "g")         'doc-view-reconvert-doc)
+    (define-key map (kbd "r")         'doc-view-reconvert-doc)
     map)
   "Keymap used by `doc-view-mode' when displaying a doc as a set of images.")
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

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

* Re: doc-view cache file permissions
  2007-10-31  7:56         ` Tassilo Horn
@ 2007-10-31 15:10           ` Stefan Monnier
  2007-10-31 17:46             ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2007-10-31 15:10 UTC (permalink / raw)
  To: emacs-devel

> furthermore I noticed that `g' is bound twice in d-v-mode-map.  I
> changed the bindings so that `g' is bound to d-v-goto-page and `r' is
> bound to d-v-reconvert-doc now.

I understand that `r' (or `C-r') is often used by pdf viewers to
refresh/reload, but `g' is the standard binding in Emacs for that, so I'd
rather we keep `g' for `revert-buffer', add `r' that does the same, and then
just use the standard M-g M-g binding to goto a specific page.

Also I prefer `revert-buffer' because it also reverts the buffer's content
rather than only the display.

So I suggest the patch below instead,


        Stefan


PS: I also think that C-S-n should be C-s and C-S-p should be C-r (but it's
a bit more than just changing bindings and I haven't written the
corresponding code).


Index: doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.15
diff -u -u -b -r1.15 doc-view.el
--- doc-view.el	31 Oct 2007 03:10:32 -0000	1.15
+++ doc-view.el	31 Oct 2007 15:07:50 -0000
@@ -226,7 +226,7 @@
     (define-key map (kbd "DEL")       'doc-view-scroll-down-or-previous-page)
     (define-key map (kbd "M-<")       'doc-view-first-page)
     (define-key map (kbd "M->")       'doc-view-last-page)
-    (define-key map (kbd "g")         'doc-view-goto-page)
+    (define-key map [remap goto-line] 'doc-view-goto-page)
     ;; Killing/burying the buffer (and the process)
     (define-key map (kbd "q")         'bury-buffer)
     (define-key map (kbd "k")         'doc-view-kill-proc-and-buffer)
@@ -250,7 +250,8 @@
     ;; Toggle between text and image display or editing
     (define-key map (kbd "C-c C-c")   'doc-view-toggle-display)
     ;; Reconvert the current document
-    (define-key map (kbd "g")         'doc-view-reconvert-doc)
+    (define-key map (kbd "g")         'revert-buffer)
+    (define-key map (kbd "r")         'revert-buffer)
     map)
   "Keymap used by `doc-view-mode' when displaying a doc as a set of images.")

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

* Re: doc-view cache file permissions
  2007-10-31 15:10           ` Stefan Monnier
@ 2007-10-31 17:46             ` Tassilo Horn
  2007-10-31 18:21               ` Stefan Monnier
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-10-31 17:46 UTC (permalink / raw)
  To: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> furthermore I noticed that `g' is bound twice in d-v-mode-map.  I
>> changed the bindings so that `g' is bound to d-v-goto-page and `r' is
>> bound to d-v-reconvert-doc now.
>
> I understand that `r' (or `C-r') is often used by pdf viewers to
> refresh/reload, but `g' is the standard binding in Emacs for that, so
> I'd rather we keep `g' for `revert-buffer', add `r' that does the
> same, and then just use the standard M-g M-g binding to goto a
> specific page.
>
> Also I prefer `revert-buffer' because it also reverts the buffer's
> content rather than only the display.
>
> So I suggest the patch below instead,

That's fine with me.

> PS: I also think that C-S-n should be C-s and C-S-p should be C-r (but
> it's a bit more than just changing bindings and I haven't written the
> corresponding code).

You mean a real interactive search?  Of course, that would be fabulous!

Bye,
Tassilo

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

* Re: doc-view cache file permissions
  2007-10-31 17:46             ` Tassilo Horn
@ 2007-10-31 18:21               ` Stefan Monnier
  2007-10-31 19:44                 ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2007-10-31 18:21 UTC (permalink / raw)
  To: emacs-devel

>> PS: I also think that C-S-n should be C-s and C-S-p should be C-r (but
>> it's a bit more than just changing bindings and I haven't written the
>> corresponding code).

> You mean a real interactive search?  Of course, that would be fabulous!

Not necessarily, but after the initial C-s, a second C-s can be used to jump
to the next match or a C-r can be used to jump to the previous one.


        Stefan

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

* Re: doc-view cache file permissions
  2007-10-31 18:21               ` Stefan Monnier
@ 2007-10-31 19:44                 ` Tassilo Horn
  2007-10-31 20:41                   ` Stefan Monnier
  2007-11-02 20:44                   ` Juri Linkov
  0 siblings, 2 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-10-31 19:44 UTC (permalink / raw)
  To: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> PS: I also think that C-S-n should be C-s and C-S-p should be C-r
>>> (but it's a bit more than just changing bindings and I haven't
>>> written the corresponding code).
>
>> You mean a real interactive search?  Of course, that would be
>> fabulous!
>
> Not necessarily, but after the initial C-s, a second C-s can be used
> to jump to the next match or a C-r can be used to jump to the previous
> one.

Sounds good to me, but what would you do to enter an new search phrase?
A prefix arg?

Bye,
Tassilo

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

* Re: doc-view cache file permissions
  2007-10-31 19:44                 ` Tassilo Horn
@ 2007-10-31 20:41                   ` Stefan Monnier
  2007-11-02 20:44                   ` Juri Linkov
  1 sibling, 0 replies; 32+ messages in thread
From: Stefan Monnier @ 2007-10-31 20:41 UTC (permalink / raw)
  To: emacs-devel

>>>> PS: I also think that C-S-n should be C-s and C-S-p should be C-r
>>>> (but it's a bit more than just changing bindings and I haven't
>>>> written the corresponding code).
>> 
>>> You mean a real interactive search?  Of course, that would be
>>> fabulous!
>> 
>> Not necessarily, but after the initial C-s, a second C-s can be used
>> to jump to the next match or a C-r can be used to jump to the previous
>> one.

> Sounds good to me, but what would you do to enter an new search phrase?

Some other command or RET?  Basically, provide an interface as close as
possible to isearch.  Actually incremental search might also be doable, tho
not as useful as isearch since the matched text isn't highlighted.


        Stefan

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

* Re: doc-view cache file permissions
  2007-10-31 19:44                 ` Tassilo Horn
  2007-10-31 20:41                   ` Stefan Monnier
@ 2007-11-02 20:44                   ` Juri Linkov
  2007-11-02 23:53                     ` Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Juri Linkov @ 2007-11-02 20:44 UTC (permalink / raw)
  To: emacs-devel

>>>> PS: I also think that C-S-n should be C-s and C-S-p should be C-r
>>>> (but it's a bit more than just changing bindings and I haven't
>>>> written the corresponding code).
>>
>>> You mean a real interactive search?  Of course, that would be
>>> fabulous!
>>
>> Not necessarily, but after the initial C-s, a second C-s can be used
>> to jump to the next match or a C-r can be used to jump to the previous
>> one.
>
> Sounds good to me, but what would you do to enter an new search phrase?
> A prefix arg?

If implementing a complete isearch interface is impossible, it suggest
using the same interface as used by Info-search (`s' in Info).

-- 
Juri Linkov
http://www.jurta.org/emacs/

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

* Re: doc-view cache file permissions
  2007-11-02 20:44                   ` Juri Linkov
@ 2007-11-02 23:53                     ` Tassilo Horn
  2007-11-03 14:07                       ` Stefan Monnier
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-11-02 23:53 UTC (permalink / raw)
  To: emacs-devel

Juri Linkov <juri@jurta.org> writes:

>>> Not necessarily, but after the initial C-s, a second C-s can be used
>>> to jump to the next match or a C-r can be used to jump to the
>>> previous one.
>>
>> Sounds good to me, but what would you do to enter an new search
>> phrase?  A prefix arg?
>
> If implementing a complete isearch interface is impossible, it suggest
> using the same interface as used by Info-search (`s' in Info).

I don't see any reason why some isearch-like behavior wouldn't be
possible.  Of course, we cannot highlight of the matches, so another C-s
should jump to the next page with a match instead of the next match
which could be on the same page.

But I really don't like the Info-like search, because you cannot jump
from match to match quickly, or is there some better way than `s RET'?
I like using isearch (or the current doc-view search) for navigation in
files, so jumping quickly forward and backward is a must.

Bye,
Tassilo

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

* Re: doc-view cache file permissions
  2007-11-02 23:53                     ` Tassilo Horn
@ 2007-11-03 14:07                       ` Stefan Monnier
  2007-11-03 19:20                         ` Tassilo Horn
  2007-11-05 12:01                         ` isearch for doc-view.el (was: doc-view cache file permissions) Tassilo Horn
  0 siblings, 2 replies; 32+ messages in thread
From: Stefan Monnier @ 2007-11-03 14:07 UTC (permalink / raw)
  To: emacs-devel

> I don't see any reason why some isearch-like behavior wouldn't be
> possible.  Of course, we cannot highlight of the matches, so another C-s
> should jump to the next page with a match instead of the next match
> which could be on the same page.

We may even be able to reuse the isearch machinery and just set
isearch-search-fun-function.  If not, maybe a couple more hoks can be
added to isearch for that.


        Stefan

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

* Re: doc-view cache file permissions
  2007-11-03 14:07                       ` Stefan Monnier
@ 2007-11-03 19:20                         ` Tassilo Horn
  2007-11-05 12:01                         ` isearch for doc-view.el (was: doc-view cache file permissions) Tassilo Horn
  1 sibling, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-11-03 19:20 UTC (permalink / raw)
  To: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I don't see any reason why some isearch-like behavior wouldn't be
>> possible.  Of course, we cannot highlight of the matches, so another
>> C-s should jump to the next page with a match instead of the next
>> match which could be on the same page.
>
> We may even be able to reuse the isearch machinery and just set
> isearch-search-fun-function.

Of course, that would be ideal.  I won't have any time tomorrow, but on
moday I think I can look into it.

Bye,
Tassilo

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

* isearch for doc-view.el (was: doc-view cache file permissions)
  2007-11-03 14:07                       ` Stefan Monnier
  2007-11-03 19:20                         ` Tassilo Horn
@ 2007-11-05 12:01                         ` Tassilo Horn
  2007-11-05 12:43                           ` isearch for doc-view.el David Kastrup
  2007-11-05 15:14                           ` Stefan Monnier
  1 sibling, 2 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-11-05 12:01 UTC (permalink / raw)
  To: emacs-devel

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> I don't see any reason why some isearch-like behavior wouldn't be
>> possible.  Of course, we cannot highlight of the matches, so another
>> C-s should jump to the next page with a match instead of the next
>> match which could be on the same page.
>
> We may even be able to reuse the isearch machinery and just set
> isearch-search-fun-function.

I'm trying to implement that now, but I have some problems with it.  I
set isearch-search-fun-function buffer-locally to
doc-view-isearch-function in doc-view-mode.  If I describe this variable
in a doc-view buffer, it says it hat the desired value.  But when I hit
C-s to start isearch and edebug isearch-search-fun it shows up that
isearch-search-fun-function is nil then.  But why is that?

This variable is neither set to nil in isearch.el nor doc-view.el.

After I leave isearch with C-g the variable's buffer-local value has
disappeared.

Here's my patch.  It's just a simple start, but doc-view-search-forward
seems to work correctly.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: doc-view.patch --]
[-- Type: text/x-patch, Size: 9312 bytes --]

Index: lisp/doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.18
diff -u -r1.18 doc-view.el
--- lisp/doc-view.el	1 Nov 2007 03:53:32 -0000	1.18
+++ lisp/doc-view.el	5 Nov 2007 11:41:33 -0000
@@ -101,11 +101,11 @@
 
 ;; Todo:
 ;; - better menu.
-;; - don't use `find-file'.
 ;; - Bind slicing to a drag event.
 ;; - zoom (the whole document and/or just the region around the cursor).
 ;; - get rid of the silly arrow in the fringe.
 ;; - improve anti-aliasing (pdf-utils gets it better).
+;; - Make it work with auto-compression-mode.
 
 (require 'dired)
 (require 'image-mode)
@@ -197,11 +197,9 @@
 (defvar doc-view-current-cache-dir nil
   "Only used internally.")
 
-(defvar doc-view-current-search-matches nil
-  "Only used internally.")
-
 (defvar doc-view-current-image nil
   "Only used internally.")
+
 (defvar doc-view-current-overlay)
 (defvar doc-view-pending-cache-flush nil)
 
@@ -211,6 +209,10 @@
 (defvar doc-view-previous-major-mode nil
   "Only used internally.")
 
+(defvar doc-view-last-isearch-function nil
+  "Only used internally.
+Used to distinguish between forward and backward isearch.")
+
 ;;;; DocView Keymaps
 
 (defvar doc-view-mode-map
@@ -237,10 +239,9 @@
     (define-key map (kbd "s m")       'doc-view-set-slice-using-mouse)
     (define-key map (kbd "s r")       'doc-view-reset-slice)
     ;; Searching
-    (define-key map (kbd "C-s")       'doc-view-search)
-    (define-key map (kbd "<find>")    'doc-view-search)
-    (define-key map (kbd "C-S-n")     'doc-view-search-next-match)
-    (define-key map (kbd "C-S-p")     'doc-view-search-previous-match)
+    (define-key map (kbd "C-s")       'isearch-forward)
+    (define-key map (kbd "<find>")    'isearch-forward)
+    (define-key map (kbd "C-r")       'isearch-backward)
     ;; Scrolling
     (define-key map [remap forward-char]  'image-forward-hscroll)
     (define-key map [remap backward-char] 'image-backward-hscroll)
@@ -294,17 +295,7 @@
 	   ;; Tell user if converting isn't finished yet
 	   (if doc-view-current-converter-process
 	       " (still converting...)\n"
-	     "\n")
-	   ;; Display context infos if this page matches the last search
-	   (when (and doc-view-current-search-matches
-		      (assq doc-view-current-page
-			    doc-view-current-search-matches))
-	     (concat (propertize "Search matches:\n" 'face 'bold)
-		     (let ((contexts ""))
-		       (dolist (m (cdr (assq doc-view-current-page
-					     doc-view-current-search-matches)))
-			 (setq contexts (concat contexts "  - \"" m "\"\n")))
-		       contexts)))))
+	     "\n")))
     ;; Update the buffer
     (doc-view-insert-image (nth (1- page) doc-view-current-files)
                            :pointer 'arrow)
@@ -500,9 +491,9 @@
       (setq doc-view-current-converter-process nil
 	    mode-line-process nil)
       ;; If the user looks at the DocView buffer where the conversion was
-      ;; performed, search anew.  This time it will be queried for a regexp.
+      ;; performed, search anew.
       (when (eq current-buffer proc-buffer)
-	(doc-view-search)))))
+	(funcall doc-view-last-isearch-function)))))
 
 (defun doc-view-pdf->txt (pdf txt)
   "Convert PDF to TXT asynchronously."
@@ -693,68 +684,21 @@
 
 ;;;; Searching
 
-(defun doc-view-search-internal (regexp file)
-  "Return a list of FILE's pages that contain text matching REGEXP.
-The value is an alist of the form (PAGE CONTEXTS) where PAGE is
-the pagenumber and CONTEXTS are all lines of text containing a match."
-  (with-temp-buffer
-    (insert-file-contents file)
-    (let ((page 1)
-	  (lastpage 1)
-	  matches)
-      (while (re-search-forward (concat "\\(?:\\([\f]\\)\\|\\("
-					regexp "\\)\\)") nil t)
-	(when (match-string 1) (incf page))
-	(when (match-string 2)
-	  (if (/= page lastpage)
-	      (push (cons page
-                          (list (buffer-substring
-                                 (line-beginning-position)
-                                 (line-end-position))))
-                    matches)
-	    (setq matches (cons
-			   (append
-			    (or
-			     ;; This page already is a match.
-			     (car matches)
-			     ;; This is the first match on page.
-			     (list page))
-			    (list (buffer-substring
-				   (line-beginning-position)
-				   (line-end-position))))
-			   (cdr matches))))
-	  (setq lastpage page)))
-      (nreverse matches))))
-
-(defun doc-view-search-no-of-matches (list)
-  "Extract the number of matches from the search result LIST."
-  (let ((no 0))
-    (dolist (p list)
-      (setq no (+ no (1- (length p)))))
-    no))
-
-(defun doc-view-search ()
-  "Query for a regexp and search the current document.
-If the current document hasn't been transformed to plain text
-till now do that first.  You should try searching anew when the
-conversion finished."
-  (interactive)
-  ;; New search, so forget the old results.
-  (setq doc-view-current-search-matches nil)
+(defun doc-view-isearch-function ()
+  "Return the function to use for the search."
+  (message "fooooo")
+  (if isearch-forward 'doc-view-search-forward 'doc-view-search-backward))
+
+(defun doc-view-isearch-text-file ()
+  "Return the text file used for searching or nil if it doesn't exist.
+If the text file doesn't exist start the conversion."
   (let ((txt (expand-file-name "doc.txt"
                                (doc-view-current-cache-dir))))
     (if (file-readable-p txt)
-	(progn
-	  (setq doc-view-current-search-matches
-		(doc-view-search-internal
-		 (read-from-minibuffer "Regexp: ")
-		 txt))
-	  (message "DocView: search yielded %d matches."
-		   (doc-view-search-no-of-matches
-		    doc-view-current-search-matches)))
-      ;; We must convert to TXT first!
+	txt
+      ;; We must convert to TXT first.
       (if doc-view-current-converter-process
-	  (message "DocView: please wait till conversion finished.")
+	  nil
 	(let ((ext (file-name-extension buffer-file-name)))
 	  (cond
 	   ((string= ext "pdf")
@@ -772,35 +716,42 @@
 	    (doc-view-pdf->txt (expand-file-name "doc.pdf"
                                                  (doc-view-current-cache-dir))
 			       txt))
-	   (t (error "DocView doesn't know what to do"))))))))
-
-(defun doc-view-search-next-match (arg)
-  "Go to the ARGth next matching page."
-  (interactive "p")
-  (let* ((next-pages (doc-view-remove-if
-		      (lambda (i) (<= (car i) doc-view-current-page))
-		      doc-view-current-search-matches))
-	 (page (car (nth (1- arg) next-pages))))
-    (if page
-	(doc-view-goto-page page)
-      (when (and
-	     doc-view-current-search-matches
-	     (y-or-n-p "No more matches after current page.  Wrap to first match? "))
-	(doc-view-goto-page (caar doc-view-current-search-matches))))))
+	   (t (error "DocView doesn't know what to do")))
+	  nil)))))
 
-(defun doc-view-search-previous-match (arg)
-  "Go to the ARGth previous matching page."
-  (interactive "p")
-  (let* ((prev-pages (doc-view-remove-if
-		      (lambda (i) (>= (car i) doc-view-current-page))
-		      doc-view-current-search-matches))
-	 (page (car (nth (1- arg) (nreverse prev-pages)))))
-    (if page
-	(doc-view-goto-page page)
-      (when (and
-	     doc-view-current-search-matches
-	     (y-or-n-p "No more matches before current page.  Wrap to last match? "))
-	(doc-view-goto-page (caar (last doc-view-current-search-matches)))))))
+(defun doc-view-search-forward (regexp &rest args)
+  (interactive "sRegexp: ")
+  (message "Regexp = %s" regexp)
+  (setq doc-view-last-isearch-function 'doc-view-search-forward)
+  (let* ((txt (doc-view-isearch-text-file))
+	 (page doc-view-current-page)
+	 (cur-page page)
+	 (pages (length doc-view-current-files))
+	 (cur-buf (current-buffer)))
+    (if (not txt)
+	(message "DocView: Please wait till the conversian has finished.")
+      (set-buffer (find-file-noselect txt t))
+      (goto-char (point-min))
+      ;; Go to the beginning of the current page.
+      (unless (= cur-page 0)
+	(search-forward "\f" nil t (- cur-page 1)))
+      ;; Now do a regexp search forward and goto that page.
+      (let ((search-again t))
+	(while search-again
+	  (re-search-forward (concat "\\(?:\\([\f]\\)\\|\\("
+				     regexp "\\)\\)") nil t)
+	  (when (match-string 1) (incf page))
+	  (when (and (match-string 2)
+		     (/= cur-page page))
+	    (message "match-str = %s" (buffer-substring (line-beginning-position)
+							(line-end-position)))
+	    (setq search-again nil))
+	  (when (> page pages)
+	    (setq search-again nil
+		  page nil)))
+	(set-buffer cur-buf)
+	(when page
+	  (doc-view-goto-page page))))))
 
 ;;;; User interface commands and the mode
 
@@ -848,9 +799,11 @@
   (make-local-variable 'doc-view-current-slice)
   (make-local-variable 'doc-view-current-cache-dir)
   (make-local-variable 'doc-view-current-info)
-  (make-local-variable 'doc-view-current-search-matches)
+  (make-local-variable 'doc-view-last-isearch-function)
   (set (make-local-variable 'doc-view-current-overlay)
        (make-overlay (point-min) (point-max) nil t))
+  (set (make-local-variable 'isearch-search-fun-function)
+       'doc-view-isearch-function)
   (add-hook 'change-major-mode-hook
             (lambda () (delete-overlay doc-view-current-overlay))
             nil t)

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Bye,
Tassilo

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

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

* Re: isearch for doc-view.el
  2007-11-05 12:01                         ` isearch for doc-view.el (was: doc-view cache file permissions) Tassilo Horn
@ 2007-11-05 12:43                           ` David Kastrup
  2007-11-05 13:19                             ` Tassilo Horn
  2007-11-05 15:14                           ` Stefan Monnier
  1 sibling, 1 reply; 32+ messages in thread
From: David Kastrup @ 2007-11-05 12:43 UTC (permalink / raw)
  To: emacs-devel

Tassilo Horn <tassilo@member.fsf.org> writes:

> Here's my patch.  It's just a simple start, but doc-view-search-forward
> seems to work correctly.
>
> Index: lisp/doc-view.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
> retrieving revision 1.18
> diff -u -r1.18 doc-view.el
> --- lisp/doc-view.el	1 Nov 2007 03:53:32 -0000	1.18
> +++ lisp/doc-view.el	5 Nov 2007 11:41:33 -0000

[...]

>      ;; Searching
> -    (define-key map (kbd "C-s")       'doc-view-search)
> -    (define-key map (kbd "<find>")    'doc-view-search)
> -    (define-key map (kbd "C-S-n")     'doc-view-search-next-match)
> -    (define-key map (kbd "C-S-p")     'doc-view-search-previous-match)
> +    (define-key map (kbd "C-s")       'isearch-forward)
> +    (define-key map (kbd "<find>")    'isearch-forward)
> +    (define-key map (kbd "C-r")       'isearch-backward)

Is a definition even necessary?  Can't you just inherit the normal
bindings?

-- 
David Kastrup

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

* Re: isearch for doc-view.el
  2007-11-05 12:43                           ` isearch for doc-view.el David Kastrup
@ 2007-11-05 13:19                             ` Tassilo Horn
  0 siblings, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-11-05 13:19 UTC (permalink / raw)
  To: emacs-devel

David Kastrup <dak@gnu.org> writes:

Hi David,

>> Index: lisp/doc-view.el
>> ===================================================================
>> RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
>> retrieving revision 1.18
>> diff -u -r1.18 doc-view.el
>> --- lisp/doc-view.el	1 Nov 2007 03:53:32 -0000	1.18
>> +++ lisp/doc-view.el	5 Nov 2007 11:41:33 -0000
>
> [...]
>
>>      ;; Searching
>> -    (define-key map (kbd "C-s")       'doc-view-search)
>> -    (define-key map (kbd "<find>")    'doc-view-search)
>> -    (define-key map (kbd "C-S-n")     'doc-view-search-next-match)
>> -    (define-key map (kbd "C-S-p")     'doc-view-search-previous-match)
>> +    (define-key map (kbd "C-s")       'isearch-forward)
>> +    (define-key map (kbd "<find>")    'isearch-forward)
>> +    (define-key map (kbd "C-r")       'isearch-backward)
>
> Is a definition even necessary?  Can't you just inherit the normal
> bindings?

Indeed, I removed the definitions and it still works as expected.
Thanks for the hint.

Bye,
Tassilo

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

* Re: isearch for doc-view.el
  2007-11-05 12:01                         ` isearch for doc-view.el (was: doc-view cache file permissions) Tassilo Horn
  2007-11-05 12:43                           ` isearch for doc-view.el David Kastrup
@ 2007-11-05 15:14                           ` Stefan Monnier
  2007-11-05 16:42                             ` CEDET/senator kill the buffer-local value of isearch-search-fun-function (was: isearch for doc-view.el) Tassilo Horn
  2007-11-05 21:01                             ` isearch for doc-view.el Tassilo Horn
  1 sibling, 2 replies; 32+ messages in thread
From: Stefan Monnier @ 2007-11-05 15:14 UTC (permalink / raw)
  To: emacs-devel

> I'm trying to implement that now, but I have some problems with it.  I
> set isearch-search-fun-function buffer-locally to
> doc-view-isearch-function in doc-view-mode.  If I describe this variable
> in a doc-view buffer, it says it hat the desired value.  But when I hit
> C-s to start isearch and edebug isearch-search-fun it shows up that
> isearch-search-fun-function is nil then.  But why is that?

Don't know.  It seems to work more or less here.
(other than an error because the pdf->png sentinel called doc-view-search...
with 0 args).
Also avoid the set-buffer....set-buffer style: use with-current-buffer
instead, it's cleaner and more robust.


        Stefan

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

* CEDET/senator kill the buffer-local value of isearch-search-fun-function (was: isearch for doc-view.el)
  2007-11-05 15:14                           ` Stefan Monnier
@ 2007-11-05 16:42                             ` Tassilo Horn
  2007-11-05 21:01                             ` isearch for doc-view.el Tassilo Horn
  1 sibling, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-11-05 16:42 UTC (permalink / raw)
  To: emacs-devel; +Cc: zappo

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'm trying to implement that now, but I have some problems with it.
>> I set isearch-search-fun-function buffer-locally to
>> doc-view-isearch-function in doc-view-mode.  If I describe this
>> variable in a doc-view buffer, it says it hat the desired value.  But
>> when I hit C-s to start isearch and edebug isearch-search-fun it
>> shows up that isearch-search-fun-function is nil then.  But why is
>> that?
>
> Don't know.  It seems to work more or less here.

Here it does not.

I have the same problem with e.g. longlines-mode, which sets
isearch-search-fun-function buffer-locally, too.  Here's what I do.

,----
| 1. C-x C-f somefile
| 2. M-x longlines-mode
| 3. C-h v isearch-search-fun-function => longlines-search-function
| 4. C-s foo C-g
| 5. C-h v isearch-search-fun-function => nil
`----

If I edebug-defun isearch-search-string before I start step 4 I can see
that the value of isearch-search-fun-function already is nil in that
function.

However, I can only reproduce this with `emacs -q', not with `emacs -Q',
so it seems some addon package I have installed causes this problem.

Ok, now I found the culprit by grepping all sources of the addon
packages I use.  It is CEDET (1.0_pre4).  In senator.el there's some
setting and killing of the buffer-local value of
isearch-search-fun-function.  I deinstalled cedet, and now it works.

Eric, can you have a look at this?

> (other than an error because the pdf->png sentinel called
> doc-view-search...  with 0 args).

Thanks, I deleted this.

> Also avoid the set-buffer....set-buffer style: use with-current-buffer
> instead, it's cleaner and more robust.

Of course, you're right.

Bye,
Tassilo

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

* Re: isearch for doc-view.el
  2007-11-05 15:14                           ` Stefan Monnier
  2007-11-05 16:42                             ` CEDET/senator kill the buffer-local value of isearch-search-fun-function (was: isearch for doc-view.el) Tassilo Horn
@ 2007-11-05 21:01                             ` Tassilo Horn
  2007-11-05 21:20                               ` Stefan Monnier
  2007-11-06  8:34                               ` Tassilo Horn
  1 sibling, 2 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-11-05 21:01 UTC (permalink / raw)
  To: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Don't know.  It seems to work more or less here.

After spending quite some time to make isearch working I have given up.
It seems to be rather complicated, so I only changed the current search
facility to use an isearch-like UI.  Now you hit C-s and enter the
regexp and each additional C-s or C-r jumps to the next/previous match.
To enter a new regexp, you have to provide a prefix arg to C-s or C-r.

Then I tried fixing the auto-compression-mode bug Reiner poined out in
<v9640nx9n9.fsf@marauder.physik.uni-ulm.de>.  Because I could not find
an elegant solution to work with compressed files transparently I took a
trivial approach:  doc-view-mode now offers to save the uncompressed
file somewhere and then this file is viewed.

Here's the ChangeLog entry:

--8<---------------cut here---------------start------------->8---
2007-11-05  Tassilo Horn  <tassilo@member.fsf.org>

	* doc-view.el (doc-view-mode-map, doc-view-menu)
	(doc-view-pdf->txt-sentinel): Adapt to new search UI.
	(doc-view-search-backward): New function.
	(doc-view-search): Query new regexp if prefix arg is given, else
	jump to next/previous match.
	(doc-view-mode): Handle compressed files.
--8<---------------cut here---------------end--------------->8---

And here's the patch:

--8<---------------cut here---------------start------------->8---
Index: lisp/doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.18
diff -u -r1.18 doc-view.el
--- lisp/doc-view.el	1 Nov 2007 03:53:32 -0000	1.18
+++ lisp/doc-view.el	5 Nov 2007 21:00:08 -0000
@@ -71,14 +71,14 @@
 ;; You can also search within the document.  The command `doc-view-search'
 ;; (bound to `C-s') queries for a search regexp and initializes a list of all
 ;; matching pages and messages how many match-pages were found.  After that you
-;; can jump to the next page containing a match with
-;; `doc-view-search-next-match' (bound to `C-S-n') or to the previous matching
-;; page with `doc-view-search-previous-match' (bound to `C-S-p').  This works
-;; by searching a plain text representation of the document.  If that doesn't
-;; already exist the first invokation of `doc-view-search' starts the
-;; conversion.  When that finishes and you're still viewing the document
-;; (i.e. you didn't switch to another buffer) you're queried for the regexp
-;; then.
+;; can jump to the next page containing a match with an additional `C-s'.  With
+;; `C-r' you can do the same, but backwards.  To search for a new regexp give a
+;; prefix arg to one of the search functions, e.g. by typing `C-u C-s'.  The
+;; searching works by using a plain text representation of the document.  If
+;; that doesn't already exist the first invokation of `doc-view-search' (or
+;; `doc-view-search-backward') starts the conversion.  When that finishes and
+;; you're still viewing the document (i.e. you didn't switch to another buffer)
+;; you're queried for the regexp then.
 ;;
 ;; Dired users can simply hit `v' on a document file.  If it's a PS, PDF or DVI
 ;; it will be opened using `doc-view-mode'.
@@ -239,8 +239,7 @@
     ;; Searching
     (define-key map (kbd "C-s")       'doc-view-search)
     (define-key map (kbd "<find>")    'doc-view-search)
-    (define-key map (kbd "C-S-n")     'doc-view-search-next-match)
-    (define-key map (kbd "C-S-p")     'doc-view-search-previous-match)
+    (define-key map (kbd "C-r")       'doc-view-search-backward)
     ;; Scrolling
     (define-key map [remap forward-char]  'image-forward-hscroll)
     (define-key map [remap backward-char] 'image-backward-hscroll)
@@ -264,6 +263,7 @@
     ["Reset Slice"		doc-view-reset-slice]
     "---"
     ["Search"			doc-view-search]
+    ["Search Backwards"         doc-view-search-backward]
     ["Toggle display"		doc-view-toggle-display]
     ))
 
@@ -502,7 +502,7 @@
       ;; If the user looks at the DocView buffer where the conversion was
       ;; performed, search anew.  This time it will be queried for a regexp.
       (when (eq current-buffer proc-buffer)
-	(doc-view-search)))))
+	(doc-view-search nil)))))
 
 (defun doc-view-pdf->txt (pdf txt)
   "Convert PDF to TXT asynchronously."
@@ -733,46 +733,56 @@
       (setq no (+ no (1- (length p)))))
     no))
 
-(defun doc-view-search ()
+(defun doc-view-search-backward (arg)
+  "Call `doc-view-search' for backward search."
+  (interactive "P")
+  (doc-view-search arg t))
+
+(defun doc-view-search (arg &optional backward)
   "Query for a regexp and search the current document.
 If the current document hasn't been transformed to plain text
 till now do that first.  You should try searching anew when the
 conversion finished."
-  (interactive)
-  ;; New search, so forget the old results.
-  (setq doc-view-current-search-matches nil)
-  (let ((txt (expand-file-name "doc.txt"
-                               (doc-view-current-cache-dir))))
-    (if (file-readable-p txt)
-	(progn
-	  (setq doc-view-current-search-matches
-		(doc-view-search-internal
-		 (read-from-minibuffer "Regexp: ")
-		 txt))
-	  (message "DocView: search yielded %d matches."
-		   (doc-view-search-no-of-matches
-		    doc-view-current-search-matches)))
-      ;; We must convert to TXT first!
-      (if doc-view-current-converter-process
-	  (message "DocView: please wait till conversion finished.")
-	(let ((ext (file-name-extension buffer-file-name)))
-	  (cond
-	   ((string= ext "pdf")
-	    ;; Doc is a PDF, so convert it to TXT
-	    (doc-view-pdf->txt buffer-file-name txt))
-	   ((string= ext "ps")
-	    ;; Doc is a PS, so convert it to PDF (which will be converted to
-	    ;; TXT thereafter).
-	    (doc-view-ps->pdf buffer-file-name
-			      (expand-file-name "doc.pdf"
-                                                (doc-view-current-cache-dir))))
-	   ((string= ext "dvi")
-	    ;; Doc is a DVI.  This means that a doc.pdf already exists in its
-	    ;; cache subdirectory.
-	    (doc-view-pdf->txt (expand-file-name "doc.pdf"
-                                                 (doc-view-current-cache-dir))
-			       txt))
-	   (t (error "DocView doesn't know what to do"))))))))
+  (interactive "P")
+  (if (and (not arg)
+	   doc-view-current-search-matches)
+      (if backward
+	  (doc-view-search-previous-match 1)
+	(doc-view-search-next-match 1))
+    ;; New search, so forget the old results.
+    (setq doc-view-current-search-matches nil)
+    (let ((txt (expand-file-name "doc.txt"
+				 (doc-view-current-cache-dir))))
+      (if (file-readable-p txt)
+	  (progn
+	    (setq doc-view-current-search-matches
+		  (doc-view-search-internal
+		   (read-from-minibuffer "Regexp: ")
+		   txt))
+	    (message "DocView: search yielded %d matches."
+		     (doc-view-search-no-of-matches
+		      doc-view-current-search-matches)))
+	;; We must convert to TXT first!
+	(if doc-view-current-converter-process
+	    (message "DocView: please wait till conversion finished.")
+	  (let ((ext (file-name-extension buffer-file-name)))
+	    (cond
+	     ((string= ext "pdf")
+	      ;; Doc is a PDF, so convert it to TXT
+	      (doc-view-pdf->txt buffer-file-name txt))
+	     ((string= ext "ps")
+	      ;; Doc is a PS, so convert it to PDF (which will be converted to
+	      ;; TXT thereafter).
+	      (doc-view-ps->pdf buffer-file-name
+				(expand-file-name "doc.pdf"
+						  (doc-view-current-cache-dir))))
+	     ((string= ext "dvi")
+	      ;; Doc is a DVI.  This means that a doc.pdf already exists in its
+	      ;; cache subdirectory.
+	      (doc-view-pdf->txt (expand-file-name "doc.pdf"
+						   (doc-view-current-cache-dir))
+				 txt))
+	     (t (error "DocView doesn't know what to do")))))))))
 
 (defun doc-view-search-next-match (arg)
   "Go to the ARGth next matching page."
@@ -835,36 +845,47 @@
 You can use \\<doc-view-mode-map>\\[doc-view-toggle-display] to
 toggle between displaying the document or editing it as text."
   (interactive)
-  (let* ((prev-major-mode (if (eq major-mode 'doc-view-mode)
-			      doc-view-previous-major-mode
-			    major-mode)))
-    (kill-all-local-variables)
-    (set (make-local-variable 'doc-view-previous-major-mode) prev-major-mode))
-  (make-local-variable 'doc-view-current-files)
-  (make-local-variable 'doc-view-current-image)
-  (make-local-variable 'doc-view-current-page)
-  (make-local-variable 'doc-view-current-converter-process)
-  (make-local-variable 'doc-view-current-timer)
-  (make-local-variable 'doc-view-current-slice)
-  (make-local-variable 'doc-view-current-cache-dir)
-  (make-local-variable 'doc-view-current-info)
-  (make-local-variable 'doc-view-current-search-matches)
-  (set (make-local-variable 'doc-view-current-overlay)
-       (make-overlay (point-min) (point-max) nil t))
-  (add-hook 'change-major-mode-hook
-            (lambda () (delete-overlay doc-view-current-overlay))
-            nil t)
-  (set (make-local-variable 'mode-line-position)
-       '(" P" (:eval (number-to-string doc-view-current-page))
-         "/" (:eval (number-to-string (length doc-view-current-files)))))
-  (set (make-local-variable 'cursor-type) nil)
-  (use-local-map doc-view-mode-map)
-  (set (make-local-variable 'after-revert-hook) 'doc-view-reconvert-doc)
-  (setq mode-name "DocView"
-	buffer-read-only t
-	major-mode 'doc-view-mode)
-  (doc-view-initiate-display)
-  (run-mode-hooks 'doc-view-mode-hook))
+  (if jka-compr-really-do-compress
+      ;; This is a compressed file uncompressed by auto-compression-mode.
+      (when (y-or-n-p (concat "DocView: Cannot convert compressed file.  "
+			      "Save it uncompressed first? "))
+	(let ((file (read-file-name
+		     "File: "
+		     (file-name-directory buffer-file-name))))
+	  (write-region (point-min) (point-max) file)
+	  (kill-buffer nil)
+	  (find-file file)
+	  (doc-view-mode)))
+    (let* ((prev-major-mode (if (eq major-mode 'doc-view-mode)
+				doc-view-previous-major-mode
+			      major-mode)))
+      (kill-all-local-variables)
+      (set (make-local-variable 'doc-view-previous-major-mode) prev-major-mode))
+    (make-local-variable 'doc-view-current-files)
+    (make-local-variable 'doc-view-current-image)
+    (make-local-variable 'doc-view-current-page)
+    (make-local-variable 'doc-view-current-converter-process)
+    (make-local-variable 'doc-view-current-timer)
+    (make-local-variable 'doc-view-current-slice)
+    (make-local-variable 'doc-view-current-cache-dir)
+    (make-local-variable 'doc-view-current-info)
+    (make-local-variable 'doc-view-current-search-matches)
+    (set (make-local-variable 'doc-view-current-overlay)
+	 (make-overlay (point-min) (point-max) nil t))
+    (add-hook 'change-major-mode-hook
+	      (lambda () (delete-overlay doc-view-current-overlay))
+	      nil t)
+    (set (make-local-variable 'mode-line-position)
+	 '(" P" (:eval (number-to-string doc-view-current-page))
+	   "/" (:eval (number-to-string (length doc-view-current-files)))))
+    (set (make-local-variable 'cursor-type) nil)
+    (use-local-map doc-view-mode-map)
+    (set (make-local-variable 'after-revert-hook) 'doc-view-reconvert-doc)
+    (setq mode-name "DocView"
+	  buffer-read-only t
+	  major-mode 'doc-view-mode)
+    (doc-view-initiate-display)
+    (run-mode-hooks 'doc-view-mode-hook)))
 
 ;;;###autoload
 (define-minor-mode doc-view-minor-mode
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

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

* Re: isearch for doc-view.el
  2007-11-05 21:01                             ` isearch for doc-view.el Tassilo Horn
@ 2007-11-05 21:20                               ` Stefan Monnier
  2007-11-05 21:51                                 ` Tassilo Horn
  2007-11-06  8:34                               ` Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2007-11-05 21:20 UTC (permalink / raw)
  To: emacs-devel

> After spending quite some time to make isearch working I have given up.
> It seems to be rather complicated, so I only changed the current search
> facility to use an isearch-like UI.  Now you hit C-s and enter the
> regexp and each additional C-s or C-r jumps to the next/previous match.
> To enter a new regexp, you have to provide a prefix arg to C-s or C-r.

The patch you sent earlier seemed to work (more or less), so please add
a big comment in the code describing as much as you can of your
experience: what you did and what were the problems you encountered.

> Then I tried fixing the auto-compression-mode bug Reiner poined out in
> <v9640nx9n9.fsf@marauder.physik.uni-ulm.de>.  Because I could not find
> an elegant solution to work with compressed files transparently I took a
> trivial approach:  doc-view-mode now offers to save the uncompressed
> file somewhere and then this file is viewed.

A possibly related problem is when the buffer's content is not in-sync
with the file's: doc-view "should" then show the rendition of the
buffer's content rather than the file's.


        Stefan

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

* Re: isearch for doc-view.el
  2007-11-05 21:20                               ` Stefan Monnier
@ 2007-11-05 21:51                                 ` Tassilo Horn
  2007-11-06  0:44                                   ` Juri Linkov
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-11-05 21:51 UTC (permalink / raw)
  To: emacs-devel

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> After spending quite some time to make isearch working I have given
>> up.  It seems to be rather complicated, so I only changed the current
>> search facility to use an isearch-like UI.  Now you hit C-s and enter
>> the regexp and each additional C-s or C-r jumps to the next/previous
>> match.  To enter a new regexp, you have to provide a prefix arg to
>> C-s or C-r.
>
> The patch you sent earlier seemed to work (more or less), so please
> add a big comment in the code describing as much as you can of your
> experience: what you did and what were the problems you encountered.

I attach the latest version of doc-view.el using the isearch approach.
The problem was that the function doing the search
(doc-view-search-forward) was called in a loop when entering something
at the isearch-prompt, e.g. I enter "p" at the prompt and then
isearch-search-string calls the function in isearch-search-fun-function
(doc-view-isearch-function) which calls doc-view-search-forward
endlessly.  I don't know why, but I don't know how isearch works
exactly.

But even if we'd get that working no real isearch feeling comes up due
to the missing match highlighting.  Currently I display all lines
containing a match in a tooltip and each C-s or C-r jumps directly to
the next/previous page with a match.  With isearch we could only display
the current match.  So we had to decide if another C-s jumps to the next
page with a match (thus only the first match in a page will be displayed
in a tooltip) or to the next match, which would do nothing visible
(except the tooltip) if the next match is on the same page.

And it's much slower than the current search facility, because isearch
really searches for each step forward or backward wheras the current
approach searches once and then it knows to which pages to jump.


[-- Attachment #2: doc-view.el --]
[-- Type: application/emacs-lisp, Size: 32896 bytes --]

[-- Attachment #3: Type: text/plain, Size: 699 bytes --]


>> Then I tried fixing the auto-compression-mode bug Reiner poined out
>> in <v9640nx9n9.fsf@marauder.physik.uni-ulm.de>.  Because I could not
>> find an elegant solution to work with compressed files transparently
>> I took a trivial approach: doc-view-mode now offers to save the
>> uncompressed file somewhere and then this file is viewed.
>
> A possibly related problem is when the buffer's content is not in-sync
> with the file's: doc-view "should" then show the rendition of the
> buffer's content rather than the file's.

I don't get you.  When you edit the document in an editing mode and hit
`C-c C-c' you'll be queried to save the changes before the conversion is
started.

Bye,
Tassilo

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

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

* Re: isearch for doc-view.el
  2007-11-05 21:51                                 ` Tassilo Horn
@ 2007-11-06  0:44                                   ` Juri Linkov
  2007-11-06  8:25                                     ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Juri Linkov @ 2007-11-06  0:44 UTC (permalink / raw)
  To: emacs-devel

> But even if we'd get that working no real isearch feeling comes up due
> to the missing match highlighting.  Currently I display all lines
> containing a match in a tooltip and each C-s or C-r jumps directly to
> the next/previous page with a match.  With isearch we could only display
> the current match.  So we had to decide if another C-s jumps to the next
> page with a match (thus only the first match in a page will be displayed
> in a tooltip) or to the next match, which would do nothing visible
> (except the tooltip) if the next match is on the same page.

That's what I meant when I suggested using a different search UI.  It is
confusing to provide isearch keys with nothing more common with isearch.

If highlighting of the matches on the rendered images is impossible,
what do you think about the following approach: display the plain text
of the current page below its rendered image (just where you currently
display the page information like "Page 1 of 1"), and let isearch to
operate on this text with all its standard features: highlighting of
all matches, etc.

Isearch on the plain text will provide enough cues to visually locate
the place of the found string on the rendered image.

-- 
Juri Linkov
http://www.jurta.org/emacs/

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

* Re: isearch for doc-view.el
  2007-11-06  0:44                                   ` Juri Linkov
@ 2007-11-06  8:25                                     ` Tassilo Horn
  2007-11-06 22:29                                       ` Juri Linkov
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-11-06  8:25 UTC (permalink / raw)
  To: emacs-devel

Juri Linkov <juri@jurta.org> writes:

Hi Juri,

> If highlighting of the matches on the rendered images is impossible,
> what do you think about the following approach: display the plain text
> of the current page below its rendered image (just where you currently
> display the page information like "Page 1 of 1"), and let isearch to
> operate on this text with all its standard features: highlighting of
> all matches, etc.
>
> Isearch on the plain text will provide enough cues to visually locate
> the place of the found string on the rendered image.

There are several problems with it:

  1. The image scrolling commands only scroll on images, so you couldn't
     scroll down to the text with <down> or C-n.

     The "Page 1 of 200" text isn't there anymore due to this.  Now it's
     in the mode-line.  (Thanks to Stefan)

  2. My main problem with the search approach was that the real isearch
     happens in another buffer in the background and the doc-view buffer
     has to be updated accordingly by switching to the right page.  With
     your approach that would be even harder.  Here, you would have to
     do the same plus copying the page with the current match from the
     text representation's buffer to the doc-view buffer plus an
     additional isearch in the doc-view buffer to highlight the match.

     Or do you mean that doc-view should display all images of a
     document at once, like:  page1.png
                              page1 text
                              page2.png
                              page2 text...

Bye,
Tassilo

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

* Re: isearch for doc-view.el
  2007-11-05 21:01                             ` isearch for doc-view.el Tassilo Horn
  2007-11-05 21:20                               ` Stefan Monnier
@ 2007-11-06  8:34                               ` Tassilo Horn
  1 sibling, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2007-11-06  8:34 UTC (permalink / raw)
  To: emacs-devel

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

Hi,

there was a missing require which I didn't think about and the
doc-string of doc-view-search wasn't updated.  This patch fixes those
problems.

--8<---------------cut here---------------start------------->8---
2007-11-06  Tassilo Horn  <tassilo@member.fsf.org>

	* doc-view.el (doc-view-mode-map, doc-view-menu)
	(doc-view-pdf->txt-sentinel): Adapt to new search UI.
	(doc-view-search-backward): New function.
	(doc-view-search): Query new regexp if prefix arg is given, else
	jump to next/previous match.
	(doc-view-mode): Handle compressed files.
	(jka-compr): Required for compressed files.
--8<---------------cut here---------------end--------------->8---


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: doc-view.patch --]
[-- Type: text/x-patch, Size: 10335 bytes --]

Index: lisp/doc-view.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/doc-view.el,v
retrieving revision 1.18
diff -u -r1.18 doc-view.el
--- lisp/doc-view.el	1 Nov 2007 03:53:32 -0000	1.18
+++ lisp/doc-view.el	6 Nov 2007 08:33:43 -0000
@@ -71,14 +71,14 @@
 ;; You can also search within the document.  The command `doc-view-search'
 ;; (bound to `C-s') queries for a search regexp and initializes a list of all
 ;; matching pages and messages how many match-pages were found.  After that you
-;; can jump to the next page containing a match with
-;; `doc-view-search-next-match' (bound to `C-S-n') or to the previous matching
-;; page with `doc-view-search-previous-match' (bound to `C-S-p').  This works
-;; by searching a plain text representation of the document.  If that doesn't
-;; already exist the first invokation of `doc-view-search' starts the
-;; conversion.  When that finishes and you're still viewing the document
-;; (i.e. you didn't switch to another buffer) you're queried for the regexp
-;; then.
+;; can jump to the next page containing a match with an additional `C-s'.  With
+;; `C-r' you can do the same, but backwards.  To search for a new regexp give a
+;; prefix arg to one of the search functions, e.g. by typing `C-u C-s'.  The
+;; searching works by using a plain text representation of the document.  If
+;; that doesn't already exist the first invokation of `doc-view-search' (or
+;; `doc-view-search-backward') starts the conversion.  When that finishes and
+;; you're still viewing the document (i.e. you didn't switch to another buffer)
+;; you're queried for the regexp then.
 ;;
 ;; Dired users can simply hit `v' on a document file.  If it's a PS, PDF or DVI
 ;; it will be opened using `doc-view-mode'.
@@ -109,6 +109,7 @@
 
 (require 'dired)
 (require 'image-mode)
+(require 'jka-compr)
 
 ;;;; Customization Options
 
@@ -239,8 +240,7 @@
     ;; Searching
     (define-key map (kbd "C-s")       'doc-view-search)
     (define-key map (kbd "<find>")    'doc-view-search)
-    (define-key map (kbd "C-S-n")     'doc-view-search-next-match)
-    (define-key map (kbd "C-S-p")     'doc-view-search-previous-match)
+    (define-key map (kbd "C-r")       'doc-view-search-backward)
     ;; Scrolling
     (define-key map [remap forward-char]  'image-forward-hscroll)
     (define-key map [remap backward-char] 'image-backward-hscroll)
@@ -264,6 +264,7 @@
     ["Reset Slice"		doc-view-reset-slice]
     "---"
     ["Search"			doc-view-search]
+    ["Search Backwards"         doc-view-search-backward]
     ["Toggle display"		doc-view-toggle-display]
     ))
 
@@ -502,7 +503,7 @@
       ;; If the user looks at the DocView buffer where the conversion was
       ;; performed, search anew.  This time it will be queried for a regexp.
       (when (eq current-buffer proc-buffer)
-	(doc-view-search)))))
+	(doc-view-search nil)))))
 
 (defun doc-view-pdf->txt (pdf txt)
   "Convert PDF to TXT asynchronously."
@@ -733,46 +734,57 @@
       (setq no (+ no (1- (length p)))))
     no))
 
-(defun doc-view-search ()
-  "Query for a regexp and search the current document.
+(defun doc-view-search-backward (new-query)
+  "Call `doc-view-search' for backward search.
+If prefix NEW-QUERY is given, ask for a new regexp."
+  (interactive "P")
+  (doc-view-search arg t))
+
+(defun doc-view-search (new-query &optional backward)
+  "Jump to the next match or initiate a new search if NEW-QUERY is given.
 If the current document hasn't been transformed to plain text
-till now do that first.  You should try searching anew when the
-conversion finished."
-  (interactive)
-  ;; New search, so forget the old results.
-  (setq doc-view-current-search-matches nil)
-  (let ((txt (expand-file-name "doc.txt"
-                               (doc-view-current-cache-dir))))
-    (if (file-readable-p txt)
-	(progn
-	  (setq doc-view-current-search-matches
-		(doc-view-search-internal
-		 (read-from-minibuffer "Regexp: ")
-		 txt))
-	  (message "DocView: search yielded %d matches."
-		   (doc-view-search-no-of-matches
-		    doc-view-current-search-matches)))
-      ;; We must convert to TXT first!
-      (if doc-view-current-converter-process
-	  (message "DocView: please wait till conversion finished.")
-	(let ((ext (file-name-extension buffer-file-name)))
-	  (cond
-	   ((string= ext "pdf")
-	    ;; Doc is a PDF, so convert it to TXT
-	    (doc-view-pdf->txt buffer-file-name txt))
-	   ((string= ext "ps")
-	    ;; Doc is a PS, so convert it to PDF (which will be converted to
-	    ;; TXT thereafter).
-	    (doc-view-ps->pdf buffer-file-name
-			      (expand-file-name "doc.pdf"
-                                                (doc-view-current-cache-dir))))
-	   ((string= ext "dvi")
-	    ;; Doc is a DVI.  This means that a doc.pdf already exists in its
-	    ;; cache subdirectory.
-	    (doc-view-pdf->txt (expand-file-name "doc.pdf"
-                                                 (doc-view-current-cache-dir))
-			       txt))
-	   (t (error "DocView doesn't know what to do"))))))))
+till now do that first.
+If BACKWARD is non-nil, jump to the previous match."
+  (interactive "P")
+  (if (and (not arg)
+	   doc-view-current-search-matches)
+      (if backward
+	  (doc-view-search-previous-match 1)
+	(doc-view-search-next-match 1))
+    ;; New search, so forget the old results.
+    (setq doc-view-current-search-matches nil)
+    (let ((txt (expand-file-name "doc.txt"
+				 (doc-view-current-cache-dir))))
+      (if (file-readable-p txt)
+	  (progn
+	    (setq doc-view-current-search-matches
+		  (doc-view-search-internal
+		   (read-from-minibuffer "Regexp: ")
+		   txt))
+	    (message "DocView: search yielded %d matches."
+		     (doc-view-search-no-of-matches
+		      doc-view-current-search-matches)))
+	;; We must convert to TXT first!
+	(if doc-view-current-converter-process
+	    (message "DocView: please wait till conversion finished.")
+	  (let ((ext (file-name-extension buffer-file-name)))
+	    (cond
+	     ((string= ext "pdf")
+	      ;; Doc is a PDF, so convert it to TXT
+	      (doc-view-pdf->txt buffer-file-name txt))
+	     ((string= ext "ps")
+	      ;; Doc is a PS, so convert it to PDF (which will be converted to
+	      ;; TXT thereafter).
+	      (doc-view-ps->pdf buffer-file-name
+				(expand-file-name "doc.pdf"
+						  (doc-view-current-cache-dir))))
+	     ((string= ext "dvi")
+	      ;; Doc is a DVI.  This means that a doc.pdf already exists in its
+	      ;; cache subdirectory.
+	      (doc-view-pdf->txt (expand-file-name "doc.pdf"
+						   (doc-view-current-cache-dir))
+				 txt))
+	     (t (error "DocView doesn't know what to do")))))))))
 
 (defun doc-view-search-next-match (arg)
   "Go to the ARGth next matching page."
@@ -835,36 +847,47 @@
 You can use \\<doc-view-mode-map>\\[doc-view-toggle-display] to
 toggle between displaying the document or editing it as text."
   (interactive)
-  (let* ((prev-major-mode (if (eq major-mode 'doc-view-mode)
-			      doc-view-previous-major-mode
-			    major-mode)))
-    (kill-all-local-variables)
-    (set (make-local-variable 'doc-view-previous-major-mode) prev-major-mode))
-  (make-local-variable 'doc-view-current-files)
-  (make-local-variable 'doc-view-current-image)
-  (make-local-variable 'doc-view-current-page)
-  (make-local-variable 'doc-view-current-converter-process)
-  (make-local-variable 'doc-view-current-timer)
-  (make-local-variable 'doc-view-current-slice)
-  (make-local-variable 'doc-view-current-cache-dir)
-  (make-local-variable 'doc-view-current-info)
-  (make-local-variable 'doc-view-current-search-matches)
-  (set (make-local-variable 'doc-view-current-overlay)
-       (make-overlay (point-min) (point-max) nil t))
-  (add-hook 'change-major-mode-hook
-            (lambda () (delete-overlay doc-view-current-overlay))
-            nil t)
-  (set (make-local-variable 'mode-line-position)
-       '(" P" (:eval (number-to-string doc-view-current-page))
-         "/" (:eval (number-to-string (length doc-view-current-files)))))
-  (set (make-local-variable 'cursor-type) nil)
-  (use-local-map doc-view-mode-map)
-  (set (make-local-variable 'after-revert-hook) 'doc-view-reconvert-doc)
-  (setq mode-name "DocView"
-	buffer-read-only t
-	major-mode 'doc-view-mode)
-  (doc-view-initiate-display)
-  (run-mode-hooks 'doc-view-mode-hook))
+  (if jka-compr-really-do-compress
+      ;; This is a compressed file uncompressed by auto-compression-mode.
+      (when (y-or-n-p (concat "DocView: Cannot convert compressed file.  "
+			      "Save it uncompressed first? "))
+	(let ((file (read-file-name
+		     "File: "
+		     (file-name-directory buffer-file-name))))
+	  (write-region (point-min) (point-max) file)
+	  (kill-buffer nil)
+	  (find-file file)
+	  (doc-view-mode)))
+    (let* ((prev-major-mode (if (eq major-mode 'doc-view-mode)
+				doc-view-previous-major-mode
+			      major-mode)))
+      (kill-all-local-variables)
+      (set (make-local-variable 'doc-view-previous-major-mode) prev-major-mode))
+    (make-local-variable 'doc-view-current-files)
+    (make-local-variable 'doc-view-current-image)
+    (make-local-variable 'doc-view-current-page)
+    (make-local-variable 'doc-view-current-converter-process)
+    (make-local-variable 'doc-view-current-timer)
+    (make-local-variable 'doc-view-current-slice)
+    (make-local-variable 'doc-view-current-cache-dir)
+    (make-local-variable 'doc-view-current-info)
+    (make-local-variable 'doc-view-current-search-matches)
+    (set (make-local-variable 'doc-view-current-overlay)
+	 (make-overlay (point-min) (point-max) nil t))
+    (add-hook 'change-major-mode-hook
+	      (lambda () (delete-overlay doc-view-current-overlay))
+	      nil t)
+    (set (make-local-variable 'mode-line-position)
+	 '(" P" (:eval (number-to-string doc-view-current-page))
+	   "/" (:eval (number-to-string (length doc-view-current-files)))))
+    (set (make-local-variable 'cursor-type) nil)
+    (use-local-map doc-view-mode-map)
+    (set (make-local-variable 'after-revert-hook) 'doc-view-reconvert-doc)
+    (setq mode-name "DocView"
+	  buffer-read-only t
+	  major-mode 'doc-view-mode)
+    (doc-view-initiate-display)
+    (run-mode-hooks 'doc-view-mode-hook)))
 
 ;;;###autoload
 (define-minor-mode doc-view-minor-mode

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Bye,
Tassilo

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

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

* Re: isearch for doc-view.el
  2007-11-06  8:25                                     ` Tassilo Horn
@ 2007-11-06 22:29                                       ` Juri Linkov
  2007-11-07  8:41                                         ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Juri Linkov @ 2007-11-06 22:29 UTC (permalink / raw)
  To: emacs-devel

>> Isearch on the plain text will provide enough cues to visually locate
>> the place of the found string on the rendered image.
>
> There are several problems with it:
>
>   1. The image scrolling commands only scroll on images, so you couldn't
>      scroll down to the text with <down> or C-n.
>
>      The "Page 1 of 200" text isn't there anymore due to this.  Now it's
>      in the mode-line.  (Thanks to Stefan)
>
>   2. My main problem with the search approach was that the real isearch
>      happens in another buffer in the background and the doc-view buffer
>      has to be updated accordingly by switching to the right page.  With
>      your approach that would be even harder.  Here, you would have to
>      do the same plus copying the page with the current match from the
>      text representation's buffer to the doc-view buffer plus an
>      additional isearch in the doc-view buffer to highlight the match.

It seems the hardest problem here is fixing scrolling commands to take
into account the text below the image.  Copying the text of the current
page and starting isearch on it should be easy.  Basically isearch in
Info already works this way - when isearch goes to another Info file
then the text in the current Info buffer gets replaced with the text
from another file and isearch continues on the new text.

>      Or do you mean that doc-view should display all images of a
>      document at once, like:  page1.png
>                               page1 text
>                               page2.png
>                               page2 text...

No, I meant displaying only one image and text from the same image at once:

                                page1.png
                                page1 text

But maybe for small documents (with small number of pages) displaying
all images at once is more preferable?

-- 
Juri Linkov
http://www.jurta.org/emacs/

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

* Re: isearch for doc-view.el
  2007-11-06 22:29                                       ` Juri Linkov
@ 2007-11-07  8:41                                         ` Tassilo Horn
  2007-11-10 21:57                                           ` Juri Linkov
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2007-11-07  8:41 UTC (permalink / raw)
  To: emacs-devel

Juri Linkov <juri@jurta.org> writes:

>>> Isearch on the plain text will provide enough cues to visually
>>> locate the place of the found string on the rendered image.
>>
>> There are several problems with it:
>>
>>   1. The image scrolling commands only scroll on images, so you couldn't
>>      scroll down to the text with <down> or C-n.
>>
>>      The "Page 1 of 200" text isn't there anymore due to this.  Now it's
>>      in the mode-line.  (Thanks to Stefan)
>>
>>   2. My main problem with the search approach was that the real isearch
>>      happens in another buffer in the background and the doc-view buffer
>>      has to be updated accordingly by switching to the right page.  With
>>      your approach that would be even harder.  Here, you would have to
>>      do the same plus copying the page with the current match from the
>>      text representation's buffer to the doc-view buffer plus an
>>      additional isearch in the doc-view buffer to highlight the match.
>
> It seems the hardest problem here is fixing scrolling commands to take
> into account the text below the image.  Copying the text of the
> current page and starting isearch on it should be easy.

Feel free to give it a try.  I'm don't know enough about the isearch
internals to make it working in an acceptable amount of time, but I
won't be against it if it feels good.  I usability problem I see is that
because of isearch the buffer is scrolled to the matching text and the
user would have to scroll back to the image after every C-s.

Basically I'm completely satisfied with the behavior I implemented in
<87640fzs9o.fsf@baldur.tsdh.de>, but probably others are more demanding.

> No, I meant displaying only one image and text from the same image at
> once:
>                                 page1.png
>                                 page1 text
>
> But maybe for small documents (with small number of pages) displaying
> all images at once is more preferable?

I don't know if the benefits would be great enough to complicate the
code.  At least most navigation and search commends would need to be
updated accordingly.

If someone implements that and it works, it's fine with me, but I won't
put it on my todo list for now.

Bye,
Tassilo

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

* Re: isearch for doc-view.el
  2007-11-07  8:41                                         ` Tassilo Horn
@ 2007-11-10 21:57                                           ` Juri Linkov
  0 siblings, 0 replies; 32+ messages in thread
From: Juri Linkov @ 2007-11-10 21:57 UTC (permalink / raw)
  To: emacs-devel

>> It seems the hardest problem here is fixing scrolling commands to take
>> into account the text below the image.  Copying the text of the
>> current page and starting isearch on it should be easy.
>
> Feel free to give it a try.  I'm don't know enough about the isearch
> internals to make it working in an acceptable amount of time, but I
> won't be against it if it feels good.  I usability problem I see is that
> because of isearch the buffer is scrolled to the matching text and the
> user would have to scroll back to the image after every C-s.
>
> Basically I'm completely satisfied with the behavior I implemented in
> <87640fzs9o.fsf@baldur.tsdh.de>, but probably others are more demanding.

Installed.  I'll try using your changes in doc-mode and see what further
improvements I could suggest.

-- 
Juri Linkov
http://www.jurta.org/emacs/

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

end of thread, other threads:[~2007-11-10 21:57 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-30 18:50 doc-view cache file permissions Glenn Morris
2007-10-30 20:11 ` Stefan Monnier
2007-10-30 20:57   ` Glenn Morris
2007-10-30 21:56     ` Stefan Monnier
2007-10-31  7:22       ` Tassilo Horn
2007-10-31  7:56         ` Tassilo Horn
2007-10-31 15:10           ` Stefan Monnier
2007-10-31 17:46             ` Tassilo Horn
2007-10-31 18:21               ` Stefan Monnier
2007-10-31 19:44                 ` Tassilo Horn
2007-10-31 20:41                   ` Stefan Monnier
2007-11-02 20:44                   ` Juri Linkov
2007-11-02 23:53                     ` Tassilo Horn
2007-11-03 14:07                       ` Stefan Monnier
2007-11-03 19:20                         ` Tassilo Horn
2007-11-05 12:01                         ` isearch for doc-view.el (was: doc-view cache file permissions) Tassilo Horn
2007-11-05 12:43                           ` isearch for doc-view.el David Kastrup
2007-11-05 13:19                             ` Tassilo Horn
2007-11-05 15:14                           ` Stefan Monnier
2007-11-05 16:42                             ` CEDET/senator kill the buffer-local value of isearch-search-fun-function (was: isearch for doc-view.el) Tassilo Horn
2007-11-05 21:01                             ` isearch for doc-view.el Tassilo Horn
2007-11-05 21:20                               ` Stefan Monnier
2007-11-05 21:51                                 ` Tassilo Horn
2007-11-06  0:44                                   ` Juri Linkov
2007-11-06  8:25                                     ` Tassilo Horn
2007-11-06 22:29                                       ` Juri Linkov
2007-11-07  8:41                                         ` Tassilo Horn
2007-11-10 21:57                                           ` Juri Linkov
2007-11-06  8:34                               ` Tassilo Horn
2007-10-30 22:14     ` doc-view cache file permissions Reiner Steib
2007-10-31  0:52       ` Stefan Monnier
2007-10-30 20:34 ` Tassilo Horn

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).