bug#5062: 23.1.50; image-toggle-display overwrites nxml-mode local key map

[Juri Linkov <juri@jurta.org>]

>>> Better would be to save the major mode, and then switch to
>>> "saved-major-mode + image-minor-mode".  No need to modify anybody's
>>> keymap.  And I think that image-minor-mode should only be used while
>>> displaying text: basically, image-toggle-display should toggle between
>>> image-mode and the other major-mode (complemented with
>>> image-minor-mode).
>>> That would be cleaner,
>> I see now how this is implemented with `doc-view-previous-major-mode'
>> in doc-view.  Perhaps we should "sync" this with image-mode to use
>> the same logic.
>
> Yes.  Those two modes really need to be made more alike (ideally, they
> should be merged, tho there might be some good reasons to keep some of
> it separate).

Even though doc-view is a superset of image-mode, currently I see no way
to merge them cleanly.  But using the same handling of major/minor modes
would be good.  The patch below leaves the role for `image-minor-mode'
to only provide the `C-c C-c' binding, and also toggles between
`image-mode' and `image-mode-maybe' in `image-toggle-display' .

If this is the right direction, I'd finish refactoring of image.el
(some variable settings could be moved from `image-toggle-display' to
`image-mode', etc.)

One open question is about using `image-mode-maybe'.  With this
patch, it's essentially a combination of a non-image major mode
(`normal-mode' with image-mode entries removed from `auto-mode-alist')
and `image-minor-mode'.  So when `image-mode-maybe' is present in
`auto-mode-alist' this means "to set a non-image major mode and
image-minor-mode".

It would be more nice to get rid of `image-mode-maybe', and to use
`image-minor-mode' in `auto-mode-alist'.  But I don't know how to
specify both a non-image major mode and image-minor-mode
at the same time using `auto-mode-alist'.

The approach currently used in doc-view for PS files is more ugly than
using `image-mode-maybe'.  It adds to ps-mode.el the line:

  (declare-function doc-view-minor-mode "doc-view")

and calls `(doc-view-minor-mode 1)' at the end of `ps-mode'.
I think this should be changed to use the solution described above
adding e.g. `doc-view-mode-maybe' (or a better name) as a combination
of a non-image major mode and image minor-mode.

Index: lisp/image-mode.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/image-mode.el,v
retrieving revision 1.59
diff -u -w -b -C 2 -r1.59 image-mode.el
cvs diff: conflicting specifications of output style
*** lisp/image-mode.el	28 Nov 2009 20:45:22 -0000	1.59
--- lisp/image-mode.el	29 Nov 2009 16:02:54 -0000
***************
*** 287,290 ****
--- 287,293 ----
  (make-variable-buffer-local 'image-type)
  
+ (defvar image-mode-previous-major-mode nil
+   "Internal variable to save the major mode.")
+ 
  (defvar image-mode-map
    (let ((map (make-sparse-keymap)))
***************
*** 307,311 ****
    "Major mode keymap for viewing images in Image mode.")
  
! (defvar image-mode-text-map
    (let ((map (make-sparse-keymap)))
      (define-key map "\C-c\C-c" 'image-toggle-display)
--- 310,314 ----
    "Major mode keymap for viewing images in Image mode.")
  
! (defvar image-minor-mode-map
    (let ((map (make-sparse-keymap)))
      (define-key map "\C-c\C-c" 'image-toggle-display)
***************
*** 323,335 ****
    (kill-all-local-variables)
    (setq major-mode 'image-mode)
-   ;; Use our own bookmarking function for images.
-   (set (make-local-variable 'bookmark-make-record-function)
-        'image-bookmark-make-record)
  
!   ;; Keep track of [vh]scroll when switching buffers
!   (image-mode-setup-winprops)
  
-   (add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
-   (if (display-images-p)
        (if (not (image-get-display-property))
  	  (image-toggle-display)
--- 326,335 ----
    (kill-all-local-variables)
    (setq major-mode 'image-mode)
  
!   (unless (display-images-p)
!     (setq image-type "text")
!     (image-toggle-display-text)
!     (error "Cannot display image"))
  
    (if (not (image-get-display-property))
        (image-toggle-display)
***************
*** 339,352 ****
  	(setq cursor-type nil truncate-lines t
  	      image-type (plist-get (cdr (image-get-display-property)) :type)))
!     (setq image-type "text")
!     (use-local-map image-mode-text-map))
    (setq mode-name (format "Image[%s]" image-type))
    (run-mode-hooks 'image-mode-hook)
-   (if (display-images-p)
        (message "%s" (concat
  		     (substitute-command-keys
  		      "Type \\[image-toggle-display] to view as ")
  		     (if (image-get-display-property)
! 			 "text" "an image") "."))))
  
  ;;;###autoload
--- 339,358 ----
      (setq cursor-type nil truncate-lines t
  	  image-type (plist-get (cdr (image-get-display-property)) :type)))
! 
!   ;; Use our own bookmarking function for images.
!   (set (make-local-variable 'bookmark-make-record-function)
!        'image-bookmark-make-record)
! 
!   ;; Keep track of [vh]scroll when switching buffers
!   (image-mode-setup-winprops)
! 
!   (add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
    (setq mode-name (format "Image[%s]" image-type))
    (run-mode-hooks 'image-mode-hook)
    (message "%s" (concat
  		 (substitute-command-keys
  		  "Type \\[image-toggle-display] to view as ")
  		 (if (image-get-display-property)
! 		     "text" "an image") ".")))
  
  ;;;###autoload
***************
*** 355,386 ****
  With arg, turn Image minor mode on if arg is positive, off otherwise.
  See the command `image-mode' for more information on this mode."
!   nil (:eval (format " Image[%s]" image-type)) image-mode-text-map
    :group 'image
    :version "22.1"
!   (if (not image-minor-mode)
!       (image-toggle-display-text)
!     (image-mode-setup-winprops)
      (add-hook 'change-major-mode-hook (lambda () (image-minor-mode -1)) nil t)
!     (if (display-images-p)
!         (condition-case err
!             (progn
!               (if (not (image-get-display-property))
!                   (image-toggle-display)
!                 (setq cursor-type nil truncate-lines t
!                       image-type (plist-get (cdr (image-get-display-property))
!                                             :type)))
!               (message "%s"
!                        (concat
!                         (substitute-command-keys
!                          "Type \\[image-toggle-display] to view the image as ")
!                         (if (image-get-display-property)
!                             "text" "an image") ".")))
!           (error
!            (image-toggle-display-text)
!            (funcall
!             (if (called-interactively-p 'any) 'error 'message)
!             "Cannot display image: %s" (cdr err))))
!       (setq image-type "text")
!       (use-local-map image-mode-text-map))))
  
  ;;;###autoload
--- 361,371 ----
  With arg, turn Image minor mode on if arg is positive, off otherwise.
  See the command `image-mode' for more information on this mode."
!   nil (:eval (if image-type (format " Image[%s]" image-type) " Image"))
!   image-minor-mode-map
    :group 'image
    :version "22.1"
!   (if image-minor-mode
        (add-hook 'change-major-mode-hook (lambda () (image-minor-mode -1)) nil t)
!     (set (make-local-variable 'image-mode-previous-major-mode) major-mode)))
  
  ;;;###autoload
***************
*** 395,399 ****
  information on these modes."
    (interactive)
!   (let* ((mode-alist
  	  (delq nil (mapcar
  		     (lambda (elt)
--- 380,387 ----
  information on these modes."
    (interactive)
!   ;; image-mode-maybe = normal-mode + image-minor-mode
!   (if image-mode-previous-major-mode
!       (funcall image-mode-previous-major-mode)
!     (let ((auto-mode-alist
  	   (delq nil (mapcar
  		      (lambda (elt)
***************
*** 401,411 ****
  				     '(image-mode image-mode-maybe))
  			 elt))
! 		     auto-mode-alist))))
!     (if (assoc-default buffer-file-name mode-alist 'string-match)
! 	(let ((auto-mode-alist mode-alist)
! 	      (magic-mode-alist nil))
! 	  (set-auto-mode)
! 	  (image-minor-mode t))
!       (image-mode))))
  
  (defun image-toggle-display-text ()
--- 389,405 ----
  				      '(image-mode image-mode-maybe))
  			  elt))
! 		      auto-mode-alist)))
! 	  (magic-fallback-mode-alist
! 	   (delq nil (mapcar
! 		      (lambda (elt)
! 			(unless (memq (or (car-safe (cdr elt)) (cdr elt))
! 				      '(image-mode image-mode-maybe))
! 			  elt))
! 		      magic-fallback-mode-alist))))
!       (normal-mode)
!       (set (make-local-variable 'image-mode-previous-major-mode) major-mode)))
!   (image-minor-mode 1))
  
  (defun image-toggle-display-text ()
***************
*** 431,441 ****
  						  read-only front-sticky))
  	(set-buffer-modified-p modified)
- 	(kill-local-variable 'cursor-type)
- 	(kill-local-variable 'truncate-lines)
- 	(kill-local-variable 'auto-hscroll-mode)
- 	(use-local-map image-mode-text-map)
  	(setq image-type "text")
! 	(if (eq major-mode 'image-mode)
! 	    (setq mode-name "Image[text]"))
  	(if (called-interactively-p 'any)
  	    (message "Repeat this command to go back to displaying the image")))
--- 425,430 ----
  						  read-only front-sticky))
  	(set-buffer-modified-p modified)
  	(setq image-type "text")
! 	(image-mode-maybe)
  	(if (called-interactively-p 'any)
  	    (message "Repeat this command to go back to displaying the image")))
***************
*** 477,482 ****
        ;; Allow navigation of large images
        (set (make-local-variable 'auto-hscroll-mode) nil)
-       (use-local-map image-mode-map)
        (setq image-type type)
        (if (eq major-mode 'image-mode)
  	  (setq mode-name (format "Image[%s]" type)))
--- 466,471 ----
        ;; Allow navigation of large images
        (set (make-local-variable 'auto-hscroll-mode) nil)
        (setq image-type type)
+       (image-mode)
        (if (eq major-mode 'image-mode)
  	  (setq mode-name (format "Image[%s]" type)))

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