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

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

[Brent Goodrick]

Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the emacs-pretest-bug@gnu.org mailing list.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug.  If you can, give
a recipe starting from `emacs -Q':

Insure that you have Emacs 23 built on Debian Squeeze Linux with
librsvg2 library (or similar OS's and libraries), just to get the
image-mode to interact with nxml-mode in its keybindings for a .svg
file which is an XML derivative for SVG files. Then proceed as
follows:

1. Store the following XML content into a file called /tmp/test.svg

--- cut below this line ---
<?xml version="1.0" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg width="400px" height="400px" viewBox="0 0 4000 4000"
     xmlns="http://www.w3.org/2000/svg" version="1.1">
  <title>Sample Title</title>
  <desc>Sample Description</desc>
  <g transform="translate(200,200)">
    <rect x="0" y="0" width="400" height="400"
          fill="none" stroke="blue" stroke-width="10px"/>
    <g transform="translate(50,25)">
      <text x="0" y="0" fill="white" stroke="none">10px</text>
    </g>
  </g>
  <g transform="translate(200,1000)">
    <rect x="0" y="0" width="400" height="400"
          fill="none" stroke="green" stroke-width="10px"/>
    <g transform="translate(50,25)">
      <text x="0" y="0" fill="white" stroke="none">1px</text>
    </g>
  </g>
</svg>
--- cut above this line ---

2. Run emacs -Q and wait for it to load and map into the display.

3. Type C-x C-f /tmp/test.svg and see that the image of the file is
displayed.

4. Type C-c C-c and note that the XML is shown. All correct behavior
so far.

5. Type C-h k C-M-n and notice that the key for C-M-n is bound to
`forward-list' which is not correct because the .svg file is a xml
file, that, by default, should be bound to `nxml-forward-element' by
the defvar for nxml-mode-map inside
share/emacs/23.1.50/lisp/nxml/nxml-mode.el.gz. Notice also that
nxml-mode applies its own local map with this call inside the
`nxml-mode' function:

  (use-local-map nxml-mode-map)

But compare that with `image-toggle-display' where it either calls:

  (use-local-map image-mode-text-map)

or

  (use-local-map image-mode-map)

Neither of which respects the nxml-mode's bindings.

For validation, you can inject a "trampoline" function on
`use-local-map' to show who overwrites the local map, as follows:

(progn
  (defun xx-new-use-local-map (&rest args)
    (message "xx-new-use-local-map called from:\n")
    (backtrace)
    (apply xx-orig-use-local-map args))
  (when (not (boundp 'xx-orig-use-local-map))
    (setq xx-orig-use-local-map (symbol-function 'use-local-map))
    (fset 'use-local-map 'xx-new-use-local-map)))

where the "xx-*" functions were arbitrary for this bug report. Once
xx-new-use-local-map is injected as given above, then use C-h C-e to
see what happens when you first execute C-x C-f /tmp/test.svg, and
then see it again when hitting C-c C-c while in the test.svg
buffer. You will see that image-mode function obliterates the local
map set by the `nxml-mode' function.

I don't know what the correct solution here should be, and many
questions arise:

a. Should image-mode be a minor mode of nxml-mode, or,
b. Should `image-mode' stash a copy of the current buffers
(current-local-map), and restore it afterwards, but also applying the
special binding for C-c C-c, or,
c. Should `image-mode' make the other modes local map be the parent of
its own local map?

I'm thinking (c)?

Thanks to whomever added SVG capabilities to Emacs!

bgoodr

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
For information about debugging Emacs, please read the file
/home/brentg/install/Linux.x86_64/share/emacs/23.1.50/etc/DEBUG.

Emacs did not crash.

In GNU Emacs 23.1.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.18.2)
 of 2009-10-31 on hungover
Windowing system distributor `The X.Org Foundation', version 11.0.10605000
configured using `configure  '--with-x-toolkit' '--with-xft'
'--prefix=/home/brentg/install/Linux.x86_64''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: nXML

Minor modes in effect:
  image-minor-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-x C-f C-v / t m p / C-x h <backspace> C-b C-b C-b
C-g C-x C-f C-SPC M-b C-b <backspace> / t m p C-d C-d
C-d C-d C-d / t e s t . s v g <return> C-v <help-echo>
<help-echo> <help-echo> <down-mouse-2> <mouse-2> C-x
C-s C-x C-v <return> C-c C-c C-h k C-M-n M-x r e p
o r t - e m <tab> <return>

Recent messages:
File mode specification error: (error "Cannot determine image type")
call-interactively: End of buffer
Mark set
Saving file /tmp/test.svg...
Wrote /tmp/test.svg
Using vacuous schema
Type C-c C-c to view the image as text.
Repeat this command to go back to displaying the image
Type C-x 1 to delete the help window.

Load-path shadows:
None found.

Features:
(shadow mail-extr message ecomplete rfc822 mml mml-sec password-cache
mm-decode mm-bodies mm-encode mailcap mail-parse rfc2231 rfc2047 rfc2045
qp ietf-drums mailabbrev nnheader gnus-util netrc time-date mm-util
mail-prsvr gmm-utils wid-edit mailheader canlock sha1 hex-util hashcash
mail-utils emacsbug sendmail help-fns help-mode view nxml-uchnm rng-xsd
xsd-regexp rng-cmpct regexp-opt rng-nxml rng-valid rng-loc rng-uri
rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn nxml-ns
easymenu nxml-mode nxml-outln nxml-rap nxml-util nxml-glyph nxml-enc
xmltok cl cl-19 jka-compr image-mode tooltip ediff-hook vc-hooks
lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image fringe
lisp-mode register page menu-bar rfn-eshadow timer select scroll-bar
mldrag mouse jit-lock font-lock syntax facemenu font-core frame cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev loaddefs button minibuffer faces cus-face text-properties overlay
md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind gtk
x-toolkit x multi-tty emacs)

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

[Stefan Monnier]

> 2. Run emacs -Q and wait for it to load and map into the display.
> 3. Type C-x C-f /tmp/test.svg and see that the image of the file is
> displayed.
> 4. Type C-c C-c and note that the XML is shown. All correct behavior
> so far.
> 5. Type C-h k C-M-n and notice that the key for C-M-n is bound to
> `forward-list' which is not correct because the .svg file is a xml

Indeed, SVG files should be handled like postscript files, i.e. use
image-minor-mode rather than image-mode.
Can someone figure out how to to do that?


        Stefan

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

[Brent Goodrick]

On Fri, Nov 27, 2009 at 6:25 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> > 2. Run emacs -Q and wait for it to load and map into the display.
> > 3. Type C-x C-f /tmp/test.svg and see that the image of the file is
> > displayed.
> > 4. Type C-c C-c and note that the XML is shown. All correct behavior
> > so far.
> > 5. Type C-h k C-M-n and notice that the key for C-M-n is bound to
> > `forward-list' which is not correct because the .svg file is a xml
>
> Indeed, SVG files should be handled like postscript files, i.e. use
> image-minor-mode rather than image-mode.
> Can someone figure out how to to do that?

Hmmm, seems that image-minor-mode is enabled in this case, with a
major mode of nxml-mode.  But image-minor-mode ultimately calls
image-toggle-display, and image-toggle-display is calling
use-local-map to obliterate whatever map is already there.
image-minor-mode can also call use-local-map, too.  All of that
arrangement seems to be set up by the image-mode-maybe function.

I wonder what the correct "policy" is for a minor mode w.r.t.
keybindings that can shadow a major mode?  Should the major mode's
keymap be the top-most local keymap, with all minor modes as parent
maps of that top-most local keymap? Or is it the other way around,
with each minor mode "pushing" its own local mode map to be top-most,
and causing the current major modes local map to be that parent of
that new one that was "pushed"?

bg

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

[Juri Linkov]

> Indeed, SVG files should be handled like postscript files, i.e. use
> image-minor-mode rather than image-mode.

Currently PS files are handled by DocView, but there is the same problem
for .xbm/.xpm files that use c-mode.

> Can someone figure out how to to do that?

The problem is in `(use-local-map image-mode-text-map)'.

After switching to "text" mode, it sets the local map to
image-mode-text-map with a single key binding `C-c C-c'
instead of restoring the original map of the major mode.

The following patch fixes this bug.  It saves a copy of the original
map to the internal buffer-local variable `image-mode-local-map'
with the additional image-mode specific key binding `C-c C-c' to
switch back to the image mode.  And on switching to the text mode
restores the original major mode map from this variable.

Index: lisp/image-mode.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/image-mode.el,v
retrieving revision 1.58
diff -u -r1.58 image-mode.el
--- lisp/image-mode.el	11 Nov 2009 05:49:13 -0000	1.58
+++ lisp/image-mode.el	28 Nov 2009 17:48:00 -0000
@@ -306,11 +306,8 @@
     map)
   "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)
-    map)
-  "Major mode keymap for viewing images as text in Image mode.")
+(defvar image-mode-local-map nil)
+(make-variable-buffer-local 'image-mode-local-map)
 
 (defvar bookmark-make-record-function)
 
@@ -329,6 +326,9 @@
   ;; Keep track of [vh]scroll when switching buffers
   (image-mode-setup-winprops)
 
+  (setq image-mode-local-map (copy-keymap (current-local-map)))
+  (define-key image-mode-local-map "\C-c\C-c" 'image-toggle-display)
+
   (add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
   (if (display-images-p)
       (if (not (image-get-display-property))
@@ -339,7 +339,7 @@
 	(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))
+    (use-local-map image-mode-local-map))
   (setq mode-name (format "Image[%s]" image-type))
   (run-mode-hooks 'image-mode-hook)
   (if (display-images-p)
@@ -354,12 +354,16 @@
   "Toggle Image minor mode.
 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
+  nil (:eval (format " Image[%s]" image-type)) nil
   :group 'image
   :version "22.1"
   (if (not image-minor-mode)
       (image-toggle-display-text)
     (image-mode-setup-winprops)
+
+    (setq image-mode-local-map (copy-keymap (current-local-map)))
+    (define-key image-mode-local-map "\C-c\C-c" 'image-toggle-display)
+
     (add-hook 'change-major-mode-hook (lambda () (image-minor-mode -1)) nil t)
     (if (display-images-p)
 	(if (not (image-get-display-property))
@@ -367,7 +371,7 @@
 	  (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))
+      (use-local-map image-mode-local-map))
     (if (display-images-p)
 	(message "%s" (concat
 		       (substitute-command-keys
@@ -425,7 +429,7 @@
 	(kill-local-variable 'cursor-type)
 	(kill-local-variable 'truncate-lines)
 	(kill-local-variable 'auto-hscroll-mode)
-	(use-local-map image-mode-text-map)
+	(use-local-map image-mode-local-map)
 	(setq image-type "text")
 	(if (eq major-mode 'image-mode)
 	    (setq mode-name "Image[text]"))

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

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

[Stefan Monnier]

> After switching to "text" mode, it sets the local map to
> image-mode-text-map with a single key binding `C-c C-c'
> instead of restoring the original map of the major mode.

> The following patch fixes this bug.  It saves a copy of the original
> map to the internal buffer-local variable `image-mode-local-map'
> with the additional image-mode specific key binding `C-c C-c' to
> switch back to the image mode.  And on switching to the text mode
> restores the original major mode map from this variable.

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,


        Stefan


> Index: lisp/image-mode.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/image-mode.el,v
> retrieving revision 1.58
> diff -u -r1.58 image-mode.el
> --- lisp/image-mode.el	11 Nov 2009 05:49:13 -0000	1.58
> +++ lisp/image-mode.el	28 Nov 2009 17:48:00 -0000
> @@ -306,11 +306,8 @@
>      map)
>    "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)
> -    map)
> -  "Major mode keymap for viewing images as text in Image mode.")
> +(defvar image-mode-local-map nil)
> +(make-variable-buffer-local 'image-mode-local-map)
 
>  (defvar bookmark-make-record-function)
 
> @@ -329,6 +326,9 @@
>    ;; Keep track of [vh]scroll when switching buffers
>    (image-mode-setup-winprops)
 
> +  (setq image-mode-local-map (copy-keymap (current-local-map)))
> +  (define-key image-mode-local-map "\C-c\C-c" 'image-toggle-display)
> +
>    (add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
>    (if (display-images-p)
>        (if (not (image-get-display-property))
> @@ -339,7 +339,7 @@
>  	(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))
> +    (use-local-map image-mode-local-map))
>    (setq mode-name (format "Image[%s]" image-type))
>    (run-mode-hooks 'image-mode-hook)
>    (if (display-images-p)
> @@ -354,12 +354,16 @@
>    "Toggle Image minor mode.
>  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
> +  nil (:eval (format " Image[%s]" image-type)) nil
>    :group 'image
>    :version "22.1"
>    (if (not image-minor-mode)
>        (image-toggle-display-text)
>      (image-mode-setup-winprops)
> +
> +    (setq image-mode-local-map (copy-keymap (current-local-map)))
> +    (define-key image-mode-local-map "\C-c\C-c" 'image-toggle-display)
> +
>      (add-hook 'change-major-mode-hook (lambda () (image-minor-mode -1)) nil t)
>      (if (display-images-p)
>  	(if (not (image-get-display-property))
> @@ -367,7 +371,7 @@
>  	  (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))
> +      (use-local-map image-mode-local-map))
>      (if (display-images-p)
>  	(message "%s" (concat
>  		       (substitute-command-keys
> @@ -425,7 +429,7 @@
>  	(kill-local-variable 'cursor-type)
>  	(kill-local-variable 'truncate-lines)
>  	(kill-local-variable 'auto-hscroll-mode)
> -	(use-local-map image-mode-text-map)
> +	(use-local-map image-mode-local-map)
>  	(setq image-type "text")
>  	(if (eq major-mode 'image-mode)
>  	    (setq mode-name "Image[text]"))

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

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

[Juri Linkov]

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

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

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

[Stefan Monnier]

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


        Stefan

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

[Juri Linkov]

>>> 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/

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

[Stefan Monnier]

> Even though doc-view is a superset of image-mode, currently I see no way
> to merge them cleanly.

As mentioned, it may not be easy or even desirable to merge
them completely.  But we should strive to keep them as close as possible
and to make doc-view use image-mode.el functions and data
wherever possible.  Maybe if one cannot be a derived mode of them other,
we could make a "shared parent" mode (like an abstract class).

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

I haven't looked in detail (partly because it doesn't apply to the
current code because of some other change I installed in the mean time),
but it looks good.

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

IIRC auto-mode-alist can point to minor-modes (the auto-mode-alist entry
needs to look something like ("regexp" minor-mode t)), but I don't think
it can be used here (the constraints on how it can be used are very
strict since it needs to "consume" a suffix and be called before the
major-mode).  Maybe we could extend auto-mode-alist to allow things like
("regexp" (major-mode minor-mode-1 minor-mode-2 ...)).

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

To the extent that postscript would always use image-minor-mode anyway,
I think the way it's done now is at least as clean if not cleaner than
relying on image-mode-maybe (image-mode-maybe is fairly hackish).

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

I don't think that would be an improvement.  This is in contrast to XPM
and SVG formats where they get combined with major-modes that are also
used for non-image documents, so it wouldn't make sense for nxml-mode or
c-mode to always enable image-minor-mode.


        Stefan

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

[Lennart Borgman]

On Sun, Nov 29, 2009 at 7:33 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> IIRC auto-mode-alist can point to minor-modes (the auto-mode-alist entry
> needs to look something like ("regexp" minor-mode t)), but I don't think
> it can be used here (the constraints on how it can be used are very
> strict since it needs to "consume" a suffix and be called before the
> major-mode).  Maybe we could extend auto-mode-alist to allow things like
> ("regexp" (major-mode minor-mode-1 minor-mode-2 ...)).


Or just define a derived mode that does major-mode + minor-mode and
use that in auto-mode-alist?

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

[Juri Linkov]

>> IIRC auto-mode-alist can point to minor-modes (the auto-mode-alist entry
>> needs to look something like ("regexp" minor-mode t)), but I don't think
>> it can be used here (the constraints on how it can be used are very
>> strict since it needs to "consume" a suffix and be called before the
>> major-mode).  Maybe we could extend auto-mode-alist to allow things like
>> ("regexp" (major-mode minor-mode-1 minor-mode-2 ...)).
>
> Or just define a derived mode that does major-mode + minor-mode and
> use that in auto-mode-alist?

That's what I did in the previous patch where `image-mode-maybe' is
a combination of a non-image major mode (`normal-mode' with image-mode
entries removed from `auto-mode-alist') and `image-minor-mode'.

Or did you mean a joint mode like `c-mode-and-image-minor-mode',
`nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
Wouldn't this be too clumsy?

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

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

[Lennart Borgman]

On Sun, Nov 29, 2009 at 11:08 PM, Juri Linkov <juri@jurta.org> wrote:
>>
>> Or just define a derived mode that does major-mode + minor-mode and
>> use that in auto-mode-alist?
>
> That's what I did in the previous patch where `image-mode-maybe' is
> a combination of a non-image major mode (`normal-mode' with image-mode
> entries removed from `auto-mode-alist') and `image-minor-mode'.
>
> Or did you mean a joint mode like `c-mode-and-image-minor-mode',
> `nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
> Wouldn't this be too clumsy?

Yes, why would it be too clumsy?

A more flexibel way might be to use define-globalized-minor-mode. The
turn on function there could make any check. It could for example look
in a list similar to auto-mode-alist, but for minor modes.

But maybe that would take too long time?

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

[Juri Linkov]

>> Or did you mean a joint mode like `c-mode-and-image-minor-mode',
>> `nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
>> Wouldn't this be too clumsy?
>
> Yes, why would it be too clumsy?
>
> A more flexibel way might be to use define-globalized-minor-mode. The
> turn on function there could make any check. It could for example look
> in a list similar to auto-mode-alist, but for minor modes.
>
> But maybe that would take too long time?

I think Stefan's idea of allowing auto-mode-alist to have entries like
("regexp" (major-mode minor-mode-1 minor-mode-2 ...)) is more universal.

Two basic ways to specify modes are:

1. With Local Variables you can put in the first line

  -*- mode: major-mode; mode: minor-mode-1; mode: minor-mode-2; ... -*-

or the same to the Local Variables list, or to the
Directory Local Variables.

2. With `auto-mode-alist' you can bind filename patterns to major modes,
but not to minor modes.  (major-mode minor-mode-1 minor-mode-2 ...)
would allow to do the same that Local Variables already allows to do, e.g.
("\\.xpm\\'" (c-mode image-minor-mode)),
("\\.svg\\'" (nxml-mode image-minor-mode)),
("\\.ps\\'" (ps-mode doc-view-minor-mode)),
etc.

PS: this feature currently is not too necessary, because in the patch I sent
`image-mode-maybe' was replaced with `image-mode' in `auto-mode-alist'
to display a file as an image by default (the default behavior in 23.1).
And only if someone wants to change this default to display a file as
text initially, this requires either adding `image-mode-maybe' to
`auto-mode-alist' or a combination of normal-mode + image-minor-mode
that can be implemented as a new feature.

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

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

[Lennart Borgman]

On Thu, Dec 3, 2009 at 1:59 AM, Juri Linkov <juri@jurta.org> wrote:
>>> Or did you mean a joint mode like `c-mode-and-image-minor-mode',
>>> `nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
>>> Wouldn't this be too clumsy?
>>
>> Yes, why would it be too clumsy?
>>
>> A more flexibel way might be to use define-globalized-minor-mode. The
>> turn on function there could make any check. It could for example look
>> in a list similar to auto-mode-alist, but for minor modes.
>>
>> But maybe that would take too long time?
>
> I think Stefan's idea of allowing auto-mode-alist to have entries like
> ("regexp" (major-mode minor-mode-1 minor-mode-2 ...)) is more universal.


Why not allow a form there then:

   ("regexp" '(progn (major-mode) (mino-mode-1 1) (minor-mode-2 1) ...))

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

[Kevin Rodgers]

Lennart Borgman wrote:
> On Thu, Dec 3, 2009 at 1:59 AM, Juri Linkov <juri@jurta.org> wrote:
>>>> Or did you mean a joint mode like `c-mode-and-image-minor-mode',
>>>> `nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
>>>> Wouldn't this be too clumsy?
>>> Yes, why would it be too clumsy?
>>>
>>> A more flexibel way might be to use define-globalized-minor-mode. The
>>> turn on function there could make any check. It could for example look
>>> in a list similar to auto-mode-alist, but for minor modes.
>>>
>>> But maybe that would take too long time?
>> I think Stefan's idea of allowing auto-mode-alist to have entries like
>> ("regexp" (major-mode minor-mode-1 minor-mode-2 ...)) is more universal.
> 
> 
> Why not allow a form there then:
> 
>    ("regexp" '(progn (major-mode) (mino-mode-1 1) (minor-mode-2 1) ...))

Because it's no longer a declarative data structure that can be queried and
modified, rather an imperative program.

-- 
Kevin Rodgers
Denver, Colorado, USA

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

[Lennart Borgman]

On Thu, Dec 3, 2009 at 4:08 AM, Kevin Rodgers <kevin.d.rodgers@gmail.com> wrote:
> Lennart Borgman wrote:
>>
>> On Thu, Dec 3, 2009 at 1:59 AM, Juri Linkov <juri@jurta.org> wrote:
>>>>>
>>>>> Or did you mean a joint mode like `c-mode-and-image-minor-mode',
>>>>> `nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
>>>>> Wouldn't this be too clumsy?
>>>>
>>>> Yes, why would it be too clumsy?
>>>>
>>>> A more flexibel way might be to use define-globalized-minor-mode. The
>>>> turn on function there could make any check. It could for example look
>>>> in a list similar to auto-mode-alist, but for minor modes.
>>>>
>>>> But maybe that would take too long time?
>>>
>>> I think Stefan's idea of allowing auto-mode-alist to have entries like
>>> ("regexp" (major-mode minor-mode-1 minor-mode-2 ...)) is more universal.
>>
>>
>> Why not allow a form there then:
>>
>>   ("regexp" '(progn (major-mode) (mino-mode-1 1) (minor-mode-2 1) ...))
>
> Because it's no longer a declarative data structure that can be queried and
> modified, rather an imperative program.


Hm, yes. I use to think it is bad to put a (lambda () ...) in a hook,
because you may want to remove it later. And actually I do modify this
list to in majmodpri.el so you are right. A simple format is better.

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

[Stefan Monnier]

>>>> Or did you mean a joint mode like `c-mode-and-image-minor-mode',
>>>> `nxml-mode-and-image-minor-mode', `ps-mode-and-doc-view-minor-mode'?
>>>> Wouldn't this be too clumsy?
>>> 
>>> Yes, why would it be too clumsy?
>>> 
>>> A more flexibel way might be to use define-globalized-minor-mode. The
>>> turn on function there could make any check. It could for example look
>>> in a list similar to auto-mode-alist, but for minor modes.
>>> 
>>> But maybe that would take too long time?
>> 
>> I think Stefan's idea of allowing auto-mode-alist to have entries like
>> ("regexp" (major-mode minor-mode-1 minor-mode-2 ...)) is more universal.


> Why not allow a form there then:

>    ("regexp" '(progn (major-mode) (mino-mode-1 1) (minor-mode-2 1) ...))

Because you can already do it just fine:

   (defun myfoo () (major-mode) (mino-mode-1 1) (minor-mode-2 1))
   
   [...]
   
      ("regexp" . myfoo)


-- Stefan

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

[Juri Linkov]

>> 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' .
>
> I haven't looked in detail (partly because it doesn't apply to the
> current code because of some other change I installed in the mean time),
> but it looks good.

Since feature freeze is postponed waiting for Alan's code (or maybe bug#5062
counts as a bug fix, but anyway) below is a patch to push to savannah.

It refactors image-mode.el to assign clean roles to the following
modes and functions:

image-mode - major mode that displays a file as an image.
  When there are an error during mode activation (either a display
  does not support images, an invalid image file, etc.), it switches
  to normal-mode + image-minor-mode and displays an error message.
  It uses your latest change to exit more gracefully when the image
  cannot be displayed.

image-minor-mode - provides the `C-c C-c' key binding
  to toggle image display.

image-mode-maybe - finds a non-image mode in `auto-mode-alist' and
  activates it, also activates `image-minor-mode'.  (`image-mode-maybe'
  is not a good name, but it's kept for backward compatibility).

image-toggle-display-text - cleans the current buffer
  from image properties.

image-toggle-display-image - puts image properties to display the image.

image-toggle-display - toggles between `image-mode' and `image-mode-maybe'.

In `auto-mode-alist', `image-mode-maybe' is replaced with `image-mode'
to restore the old 23.1 behavior of displaying the image initially.

If this is ok, I'm ready to update docstrings and commit.

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: juri@jurta.org-20091203005706-u7yip52y4ylskpuc
# target_branch: http://bzr.savannah.gnu.org/r/emacs/trunk
# testament_sha1: 30ead91e8155b5927110d672c076120376776503
# timestamp: 2009-12-03 02:57:15 +0200
# base_revision_id: handa@m17n.org-20091202080102-2jb3nd78wntfodbr
# 
# Begin patch
=== modified file 'lisp/image-mode.el'
--- lisp/image-mode.el	2009-11-28 20:45:19 +0000
+++ lisp/image-mode.el	2009-12-03 00:57:06 +0000
@@ -42,10 +42,10 @@
 ;;;###autoload (push (cons (purecopy "\\.p[bpgn]m\\'") 'image-mode) auto-mode-alist)
 
 ;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'c-mode)     auto-mode-alist)
-;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'image-mode-maybe) auto-mode-alist)
+;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'image-mode) auto-mode-alist)
 
 ;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'xml-mode)   auto-mode-alist)
-;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'image-mode-maybe) auto-mode-alist)
+;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'image-mode) auto-mode-alist)
 
 ;;; Image mode window-info management.
 
@@ -286,6 +286,9 @@
 This variable is used to display the current image type in the mode line.")
 (make-variable-buffer-local 'image-type)
 
+(defvar image-mode-previous-major-mode nil
+  "Internal variable to keep the previous non-image major mode.")
+
 (defvar image-mode-map
   (let ((map (make-sparse-keymap)))
     (suppress-keymap map)
@@ -306,7 +309,7 @@
     map)
   "Major mode keymap for viewing images in Image mode.")
 
-(defvar image-mode-text-map
+(defvar image-minor-mode-map
   (let ((map (make-sparse-keymap)))
     (define-key map "\C-c\C-c" 'image-toggle-display)
     map)
@@ -320,68 +323,58 @@
 You can use \\<image-mode-map>\\[image-toggle-display]
 to toggle between display as an image and display as text."
   (interactive)
-  (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)
-	;; Set next vars when image is already displayed but local
-	;; variables were cleared by kill-all-local-variables
+  (condition-case err
+      (progn
+	(unless (display-images-p)
+	  (error "Display does not support images"))
+
+	(kill-all-local-variables)
+	(setq major-mode 'image-mode)
+
+	(if (not (image-get-display-property))
+	    (progn
+	      (image-toggle-display-image)
+	      ;; If attempt to display image fails.
+	      (if (not (image-get-display-property))
+		  (error "Invalid image file")))
+	  ;; Set next vars when image is already displayed but local
+	  ;; variables were cleared by kill-all-local-variables
+	  (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)
+
 	(use-local-map image-mode-map)
-	(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") "."))))
+	(setq mode-name (if image-type (format "Image[%s]" image-type) "Image"))
+	(add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
+	(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") ".")))
+    (error
+     (image-mode-maybe)
+     (funcall
+      (if (called-interactively-p 'any) 'error 'message)
+      "Cannot display image: %s" (cdr err)))))
 
 ;;;###autoload
 (define-minor-mode image-minor-mode
   "Toggle Image minor mode.
 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
+  nil (:eval (if image-type (format " Image[%s]" image-type) " Image"))
+  image-minor-mode-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))))
+  (if image-minor-mode
+      (add-hook 'change-major-mode-hook (lambda () (image-minor-mode -1)) nil t)))
 
 ;;;###autoload
 (defun image-mode-maybe ()
@@ -394,94 +387,102 @@
 See commands `image-mode' and `image-minor-mode' for more
 information on these modes."
   (interactive)
-  (let* ((mode-alist
-	  (delq nil (mapcar
-		     (lambda (elt)
-		       (unless (memq (or (car-safe (cdr elt)) (cdr elt))
-				     '(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))))
+  ;; image-mode-maybe = normal-mode + image-minor-mode
+  (let ((previous-image-type image-type)) ; preserve `image-type'
+    (if image-mode-previous-major-mode
+	;; Restore previous major mode that was already found by this
+	;; function and cached in `image-mode-previous-major-mode'
+	(funcall image-mode-previous-major-mode)
+      (let ((auto-mode-alist
+	     (delq nil (mapcar
+			(lambda (elt)
+			  (unless (memq (or (car-safe (cdr elt)) (cdr elt))
+					'(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)
+    (image-toggle-display-text)
+    ;; Restore `image-type' after `kill-all-local-variables' in `normal-mode'.
+    (setq image-type previous-image-type)))
 
 (defun image-toggle-display-text ()
   "Showing the text of the image file."
-  (if (image-get-display-property)
-      (image-toggle-display)))
+  (let ((inhibit-read-only t)
+	(buffer-undo-list t)
+	(modified (buffer-modified-p)))
+    (remove-list-of-text-properties (point-min) (point-max)
+				    '(display intangible read-nonsticky
+					      read-only front-sticky))
+    (set-buffer-modified-p modified)
+    (if (called-interactively-p 'any)
+	(message "Repeat this command to go back to displaying the image"))))
 
 (defvar archive-superior-buffer)
 (defvar tar-superior-buffer)
 (declare-function image-refresh "image.c" (spec &optional frame))
 
+(defun image-toggle-display-image ()
+  "Showing the image of the image file."
+  ;; Turn the image data into a real image, but only if the whole file
+  ;; was inserted
+  (let* ((filename (buffer-file-name))
+	 (data-p (not (and filename
+			   (file-readable-p filename)
+			   (not (file-remote-p filename))
+			   (not (buffer-modified-p))
+			   (not (and (boundp 'archive-superior-buffer)
+				     archive-superior-buffer))
+			   (not (and (boundp 'tar-superior-buffer)
+				     tar-superior-buffer)))))
+	 (file-or-data (if data-p
+			   (string-make-unibyte
+			    (buffer-substring-no-properties (point-min) (point-max)))
+			 filename))
+	 (type (image-type file-or-data nil data-p))
+	 (image (create-image file-or-data type data-p))
+	 (props
+	  `(display ,image
+		    intangible ,image
+		    rear-nonsticky (display intangible)
+		    read-only t front-sticky (read-only)))
+	 (inhibit-read-only t)
+	 (buffer-undo-list t)
+	 (modified (buffer-modified-p)))
+    (image-refresh image)
+    (let ((buffer-file-truename nil)) ; avoid changing dir mtime by lock_file
+      (add-text-properties (point-min) (point-max) props)
+      (restore-buffer-modified-p modified))
+    ;; Inhibit the cursor when the buffer contains only an image,
+    ;; because cursors look very strange on top of images.
+    (setq cursor-type nil)
+    ;; This just makes the arrow displayed in the right fringe
+    ;; area look correct when the image is wider than the window.
+    (setq truncate-lines t)
+    ;; Allow navigation of large images
+    (set (make-local-variable 'auto-hscroll-mode) nil)
+    (setq image-type type)
+    (if (eq major-mode 'image-mode)
+	(setq mode-name (format "Image[%s]" type)))
+    (if (called-interactively-p 'any)
+	(message "Repeat this command to go back to displaying the file as text"))))
+
 (defun image-toggle-display ()
   "Start or stop displaying an image file as the actual image.
 This command toggles between showing the text of the image file
 and showing the image as an image."
   (interactive)
   (if (image-get-display-property)
-      (let ((inhibit-read-only t)
-	    (buffer-undo-list t)
-	    (modified (buffer-modified-p)))
-	(remove-list-of-text-properties (point-min) (point-max)
-					'(display intangible read-nonsticky
-						  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")))
-    ;; Turn the image data into a real image, but only if the whole file
-    ;; was inserted
-    (let* ((filename (buffer-file-name))
-	   (data-p (not (and filename
-			     (file-readable-p filename)
-			     (not (file-remote-p filename))
-			     (not (buffer-modified-p))
-			     (not (and (boundp 'archive-superior-buffer)
-				       archive-superior-buffer))
-			     (not (and (boundp 'tar-superior-buffer)
-				       tar-superior-buffer)))))
-	   (file-or-data (if data-p
-			     (string-make-unibyte
-			      (buffer-substring-no-properties (point-min) (point-max)))
-			   filename))
-	   (type (image-type file-or-data nil data-p))
-	   (image (create-image file-or-data type data-p))
-	   (props
-	    `(display ,image
-		      intangible ,image
-		      rear-nonsticky (display intangible)
-		      read-only t front-sticky (read-only)))
-	   (inhibit-read-only t)
-	   (buffer-undo-list t)
-	   (modified (buffer-modified-p)))
-      (image-refresh image)
-      (let ((buffer-file-truename nil)) ; avoid changing dir mtime by lock_file
-	(add-text-properties (point-min) (point-max) props)
-	(restore-buffer-modified-p modified))
-      ;; Inhibit the cursor when the buffer contains only an image,
-      ;; because cursors look very strange on top of images.
-      (setq cursor-type nil)
-      ;; This just makes the arrow displayed in the right fringe
-      ;; area look correct when the image is wider than the window.
-      (setq truncate-lines t)
-      ;; 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)))
-      (if (called-interactively-p 'any)
-	  (message "Repeat this command to go back to displaying the file as text")))))
+      (image-mode-maybe)
+    (image-mode)))
 \f
 ;;; Support for bookmark.el
 (declare-function bookmark-make-record-default "bookmark"

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

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

[Stefan Monnier]

> If this is ok, I'm ready to update docstrings and commit.

Looks good, thank you,


        Stefan

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

[Jason Rumney]

Juri Linkov wrote:
> image-mode-maybe - finds a non-image mode in `auto-mode-alist' and
>   activates it, also activates `image-minor-mode'.  (`image-mode-maybe'
>   is not a good name, but it's kept for backward compatibility).
>   

It seems like you are changing the behavior incompatibly, so why keep 
the name?  IIRC the original intention for image-mode-maybe was to use a 
non-image mode from auto-mode-alist if it existed, otherwise use 
image-mode. It originated when we were detecting images with 
magic-mode-alist, which was higher priority than auto-mode-alist.

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

[Juri Linkov]

>> image-mode-maybe - finds a non-image mode in `auto-mode-alist' and
>>   activates it, also activates `image-minor-mode'.  (`image-mode-maybe'
>>   is not a good name, but it's kept for backward compatibility).
>
> It seems like you are changing the behavior incompatibly, so why keep the
> name?  IIRC the original intention for image-mode-maybe was to use
> a non-image mode from auto-mode-alist if it existed, otherwise use
> image-mode.

Right, so the patch below makes `image-mode-maybe' obsolete as an alias
of `image-mode'.  This makes it 99% backward-compatible except for rare
cases when somehow `auto-mode-alist' doesn't contain a non-image mode -
in this case Fundamental Mode is used.

And a new name for the function that is a combination of `normal-mode' +
`image-minor-mode' is now `image-mode-as-text'.  This name avoids using
the suffix `-mode' because it's not a real mode.

A CVS patch with these changes and updated docstrings:

Index: lisp/image-mode.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/image-mode.el,v
retrieving revision 1.59
diff -c -r1.59 image-mode.el
*** lisp/image-mode.el	28 Nov 2009 20:45:22 -0000	1.59
--- lisp/image-mode.el	4 Dec 2009 00:04:52 -0000
***************
*** 42,51 ****
  ;;;###autoload (push (cons (purecopy "\\.p[bpgn]m\\'") 'image-mode) auto-mode-alist)
  
  ;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'c-mode)     auto-mode-alist)
! ;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'image-mode-maybe) auto-mode-alist)
  
  ;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'xml-mode)   auto-mode-alist)
! ;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'image-mode-maybe) auto-mode-alist)
  
  ;;; Image mode window-info management.
  
--- 42,51 ----
  ;;;###autoload (push (cons (purecopy "\\.p[bpgn]m\\'") 'image-mode) auto-mode-alist)
  
  ;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'c-mode)     auto-mode-alist)
! ;;;###autoload (push (cons (purecopy "\\.x[bp]m\\'")   'image-mode) auto-mode-alist)
  
  ;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'xml-mode)   auto-mode-alist)
! ;;;###autoload (push (cons (purecopy "\\.svgz?\\'")    'image-mode) auto-mode-alist)
  
  ;;; Image mode window-info management.
  
***************
*** 286,291 ****
--- 286,294 ----
  This variable is used to display the current image type in the mode line.")
  (make-variable-buffer-local 'image-type)
  
+ (defvar image-mode-previous-major-mode nil
+   "Internal variable to keep the previous non-image major mode.")
+ 
  (defvar image-mode-map
    (let ((map (make-sparse-keymap)))
      (suppress-keymap map)
***************
*** 306,316 ****
      map)
    "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)
      map)
!   "Major mode keymap for viewing images as text in Image mode.")
  
  (defvar bookmark-make-record-function)
  
--- 309,319 ----
      map)
    "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)
      map)
!   "Minor mode keymap for viewing images as text in Image mode.")
  
  (defvar bookmark-make-record-function)
  
***************
*** 320,487 ****
  You can use \\<image-mode-map>\\[image-toggle-display]
  to toggle between display as an image and display as text."
    (interactive)
!   (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)
! 	;; Set next vars when image is already displayed but local
! 	;; variables were cleared by kill-all-local-variables
  	(use-local-map image-mode-map)
! 	(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
  (define-minor-mode image-minor-mode
    "Toggle Image minor mode.
  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
! (defun image-mode-maybe ()
!   "Set major or minor mode for image files.
! Set Image major mode only when there are no other major modes
! associated with a filename in `auto-mode-alist'.  When an image
! filename matches another major mode in `auto-mode-alist' then
! set that major mode and Image minor mode.
  
! See commands `image-mode' and `image-minor-mode' for more
! information on these modes."
    (interactive)
!   (let* ((mode-alist
! 	  (delq nil (mapcar
! 		     (lambda (elt)
! 		       (unless (memq (or (car-safe (cdr elt)) (cdr elt))
! 				     '(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 ()
!   "Showing the text of the image file."
!   (if (image-get-display-property)
!       (image-toggle-display)))
  
  (defvar archive-superior-buffer)
  (defvar tar-superior-buffer)
  (declare-function image-refresh "image.c" (spec &optional frame))
  
  (defun image-toggle-display ()
    "Start or stop displaying an image file as the actual image.
! This command toggles between showing the text of the image file
! and showing the image as an image."
    (interactive)
    (if (image-get-display-property)
!       (let ((inhibit-read-only t)
! 	    (buffer-undo-list t)
! 	    (modified (buffer-modified-p)))
! 	(remove-list-of-text-properties (point-min) (point-max)
! 					'(display intangible read-nonsticky
! 						  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")))
!     ;; Turn the image data into a real image, but only if the whole file
!     ;; was inserted
!     (let* ((filename (buffer-file-name))
! 	   (data-p (not (and filename
! 			     (file-readable-p filename)
! 			     (not (file-remote-p filename))
! 			     (not (buffer-modified-p))
! 			     (not (and (boundp 'archive-superior-buffer)
! 				       archive-superior-buffer))
! 			     (not (and (boundp 'tar-superior-buffer)
! 				       tar-superior-buffer)))))
! 	   (file-or-data (if data-p
! 			     (string-make-unibyte
! 			      (buffer-substring-no-properties (point-min) (point-max)))
! 			   filename))
! 	   (type (image-type file-or-data nil data-p))
! 	   (image (create-image file-or-data type data-p))
! 	   (props
! 	    `(display ,image
! 		      intangible ,image
! 		      rear-nonsticky (display intangible)
! 		      read-only t front-sticky (read-only)))
! 	   (inhibit-read-only t)
! 	   (buffer-undo-list t)
! 	   (modified (buffer-modified-p)))
!       (image-refresh image)
!       (let ((buffer-file-truename nil)) ; avoid changing dir mtime by lock_file
! 	(add-text-properties (point-min) (point-max) props)
! 	(restore-buffer-modified-p modified))
!       ;; Inhibit the cursor when the buffer contains only an image,
!       ;; because cursors look very strange on top of images.
!       (setq cursor-type nil)
!       ;; This just makes the arrow displayed in the right fringe
!       ;; area look correct when the image is wider than the window.
!       (setq truncate-lines t)
!       ;; 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)))
!       (if (called-interactively-p 'any)
! 	  (message "Repeat this command to go back to displaying the file as text")))))
  \f
  ;;; Support for bookmark.el
  (declare-function bookmark-make-record-default "bookmark"
--- 323,504 ----
  You can use \\<image-mode-map>\\[image-toggle-display]
  to toggle between display as an image and display as text."
    (interactive)
!   (condition-case err
!       (progn
! 	(unless (display-images-p)
! 	  (error "Display does not support images"))
! 
! 	(kill-all-local-variables)
! 	(setq major-mode 'image-mode)
! 
! 	(if (not (image-get-display-property))
! 	    (progn
! 	      (image-toggle-display-image)
! 	      ;; If attempt to display the image fails.
! 	      (if (not (image-get-display-property))
! 		  (error "Invalid image")))
! 	  ;; Set next vars when image is already displayed but local
! 	  ;; variables were cleared by kill-all-local-variables
! 	  (setq cursor-type nil truncate-lines t
! 		image-type (plist-get (cdr (image-get-display-property)) :type)))
! 
! 	(setq mode-name (if image-type (format "Image[%s]" image-type) "Image"))
  	(use-local-map image-mode-map)
! 
! 	;; 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)
! 	(run-mode-hooks 'image-mode-hook)
! 	(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-mode-as-text)
!      (funcall
!       (if (called-interactively-p 'any) 'error 'message)
!       "Cannot display image: %s" (cdr err)))))
  
  ;;;###autoload
  (define-minor-mode image-minor-mode
    "Toggle Image minor mode.
  With arg, turn Image minor mode on if arg is positive, off otherwise.
! It provides the key \\<image-mode-map>\\[image-toggle-display] \
! to switch back to `image-mode'
! to display an image file as the actual image."
!   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)))
  
  ;;;###autoload
! (defun image-mode-as-text ()
!   "Set a non-image mode as major mode in combination with image minor mode.
! A non-image major mode found from `auto-mode-alist' or Fundamental mode
! displays an image file as text.  `image-minor-mode' provides the key
! \\<image-mode-map>\\[image-toggle-display] to switch back to `image-mode'
! to display an image file as the actual image.
! 
! You can use `image-mode-as-text' in `auto-mode-alist' when you want
! to display an image file as text inititally.
  
! See commands `image-mode' and `image-minor-mode' for more information
! on these modes."
    (interactive)
!   ;; image-mode-as-text = normal-mode + image-minor-mode
!   (let ((previous-image-type image-type)) ; preserve `image-type'
!     (if image-mode-previous-major-mode
! 	;; Restore previous major mode that was already found by this
! 	;; function and cached in `image-mode-previous-major-mode'
! 	(funcall image-mode-previous-major-mode)
!       (let ((auto-mode-alist
! 	     (delq nil (mapcar
! 			(lambda (elt)
! 			  (unless (memq (or (car-safe (cdr elt)) (cdr elt))
! 					'(image-mode image-mode-maybe image-mode-as-text))
! 			    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 image-mode-as-text))
! 			    elt))
! 			magic-fallback-mode-alist))))
! 	(normal-mode)
! 	(set (make-local-variable 'image-mode-previous-major-mode) major-mode)))
!     ;; Restore `image-type' after `kill-all-local-variables' in `normal-mode'.
!     (setq image-type previous-image-type)
!     ;; Enable image minor mode with `C-c C-c'.
!     (image-minor-mode 1)
!     ;; Show the image file as text.
!     (image-toggle-display-text)
!     (message "%s" (concat
! 		   (substitute-command-keys
! 		    "Type \\[image-toggle-display] to view the image as ")
! 		   (if (image-get-display-property)
! 		       "text" "an image") "."))))
! 
! (define-obsolete-function-alias 'image-mode-maybe 'image-mode "23.2")
  
  (defun image-toggle-display-text ()
!   "Show the image file as text.
! Remove text properties that display the image."
!   (let ((inhibit-read-only t)
! 	(buffer-undo-list t)
! 	(modified (buffer-modified-p)))
!     (remove-list-of-text-properties (point-min) (point-max)
! 				    '(display intangible read-nonsticky
! 					      read-only front-sticky))
!     (set-buffer-modified-p modified)
!     (if (called-interactively-p 'any)
! 	(message "Repeat this command to go back to displaying the image"))))
  
  (defvar archive-superior-buffer)
  (defvar tar-superior-buffer)
  (declare-function image-refresh "image.c" (spec &optional frame))
  
+ (defun image-toggle-display-image ()
+   "Show the image of the image file.
+ Turn the image data into a real image, but only if the whole file
+ was inserted."
+   (let* ((filename (buffer-file-name))
+ 	 (data-p (not (and filename
+ 			   (file-readable-p filename)
+ 			   (not (file-remote-p filename))
+ 			   (not (buffer-modified-p))
+ 			   (not (and (boundp 'archive-superior-buffer)
+ 				     archive-superior-buffer))
+ 			   (not (and (boundp 'tar-superior-buffer)
+ 				     tar-superior-buffer)))))
+ 	 (file-or-data (if data-p
+ 			   (string-make-unibyte
+ 			    (buffer-substring-no-properties (point-min) (point-max)))
+ 			 filename))
+ 	 (type (image-type file-or-data nil data-p))
+ 	 (image (create-image file-or-data type data-p))
+ 	 (props
+ 	  `(display ,image
+ 		    intangible ,image
+ 		    rear-nonsticky (display intangible)
+ 		    read-only t front-sticky (read-only)))
+ 	 (inhibit-read-only t)
+ 	 (buffer-undo-list t)
+ 	 (modified (buffer-modified-p)))
+     (image-refresh image)
+     (let ((buffer-file-truename nil)) ; avoid changing dir mtime by lock_file
+       (add-text-properties (point-min) (point-max) props)
+       (restore-buffer-modified-p modified))
+     ;; Inhibit the cursor when the buffer contains only an image,
+     ;; because cursors look very strange on top of images.
+     (setq cursor-type nil)
+     ;; This just makes the arrow displayed in the right fringe
+     ;; area look correct when the image is wider than the window.
+     (setq truncate-lines t)
+     ;; Allow navigation of large images
+     (set (make-local-variable 'auto-hscroll-mode) nil)
+     (setq image-type type)
+     (if (eq major-mode 'image-mode)
+ 	(setq mode-name (format "Image[%s]" type)))
+     (if (called-interactively-p 'any)
+ 	(message "Repeat this command to go back to displaying the file as text"))))
+ 
  (defun image-toggle-display ()
    "Start or stop displaying an image file as the actual image.
! This command toggles between `image-mode-as-text' showing the text of
! the image file and `image-mode' showing the image as an image."
    (interactive)
    (if (image-get-display-property)
!       (image-mode-as-text)
!     (image-mode)))
  \f
  ;;; Support for bookmark.el
  (declare-function bookmark-make-record-default "bookmark"

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

bug#5062: marked as done (23.1.50; image-toggle-display overwrites nxml-mode local key map)

[Emacs bug Tracking System]

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

Your message dated Fri, 04 Dec 2009 23:47:30 +0200
with message-id <878wdixvgt.fsf@mail.jurta.org>
and subject line Re: bug#5062: 23.1.50; image-toggle-display overwrites nxml-mode local key map
has caused the Emacs bug report #5062,
regarding 23.1.50; image-toggle-display overwrites nxml-mode local key map
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
5062: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=5062
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 9742 bytes --]

From: Brent Goodrick <bgoodr@gmail.com>
To: emacs-pretest-bug@gnu.org
Subject: 23.1.50; image-toggle-display overwrites nxml-mode local key map
Date: Fri, 27 Nov 2009 16:44:30 -0800
Message-ID: <e38bce640911271644l148dbc9g1fb378e6b4dbc248@mail.gmail.com>

Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the emacs-pretest-bug@gnu.org mailing list.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug.  If you can, give
a recipe starting from `emacs -Q':

Insure that you have Emacs 23 built on Debian Squeeze Linux with
librsvg2 library (or similar OS's and libraries), just to get the
image-mode to interact with nxml-mode in its keybindings for a .svg
file which is an XML derivative for SVG files. Then proceed as
follows:

1. Store the following XML content into a file called /tmp/test.svg

--- cut below this line ---
<?xml version="1.0" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg width="400px" height="400px" viewBox="0 0 4000 4000"
     xmlns="http://www.w3.org/2000/svg" version="1.1">
  <title>Sample Title</title>
  <desc>Sample Description</desc>
  <g transform="translate(200,200)">
    <rect x="0" y="0" width="400" height="400"
          fill="none" stroke="blue" stroke-width="10px"/>
    <g transform="translate(50,25)">
      <text x="0" y="0" fill="white" stroke="none">10px</text>
    </g>
  </g>
  <g transform="translate(200,1000)">
    <rect x="0" y="0" width="400" height="400"
          fill="none" stroke="green" stroke-width="10px"/>
    <g transform="translate(50,25)">
      <text x="0" y="0" fill="white" stroke="none">1px</text>
    </g>
  </g>
</svg>
--- cut above this line ---

2. Run emacs -Q and wait for it to load and map into the display.

3. Type C-x C-f /tmp/test.svg and see that the image of the file is
displayed.

4. Type C-c C-c and note that the XML is shown. All correct behavior
so far.

5. Type C-h k C-M-n and notice that the key for C-M-n is bound to
`forward-list' which is not correct because the .svg file is a xml
file, that, by default, should be bound to `nxml-forward-element' by
the defvar for nxml-mode-map inside
share/emacs/23.1.50/lisp/nxml/nxml-mode.el.gz. Notice also that
nxml-mode applies its own local map with this call inside the
`nxml-mode' function:

  (use-local-map nxml-mode-map)

But compare that with `image-toggle-display' where it either calls:

  (use-local-map image-mode-text-map)

or

  (use-local-map image-mode-map)

Neither of which respects the nxml-mode's bindings.

For validation, you can inject a "trampoline" function on
`use-local-map' to show who overwrites the local map, as follows:

(progn
  (defun xx-new-use-local-map (&rest args)
    (message "xx-new-use-local-map called from:\n")
    (backtrace)
    (apply xx-orig-use-local-map args))
  (when (not (boundp 'xx-orig-use-local-map))
    (setq xx-orig-use-local-map (symbol-function 'use-local-map))
    (fset 'use-local-map 'xx-new-use-local-map)))

where the "xx-*" functions were arbitrary for this bug report. Once
xx-new-use-local-map is injected as given above, then use C-h C-e to
see what happens when you first execute C-x C-f /tmp/test.svg, and
then see it again when hitting C-c C-c while in the test.svg
buffer. You will see that image-mode function obliterates the local
map set by the `nxml-mode' function.

I don't know what the correct solution here should be, and many
questions arise:

a. Should image-mode be a minor mode of nxml-mode, or,
b. Should `image-mode' stash a copy of the current buffers
(current-local-map), and restore it afterwards, but also applying the
special binding for C-c C-c, or,
c. Should `image-mode' make the other modes local map be the parent of
its own local map?

I'm thinking (c)?

Thanks to whomever added SVG capabilities to Emacs!

bgoodr

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
For information about debugging Emacs, please read the file
/home/brentg/install/Linux.x86_64/share/emacs/23.1.50/etc/DEBUG.

Emacs did not crash.

In GNU Emacs 23.1.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.18.2)
 of 2009-10-31 on hungover
Windowing system distributor `The X.Org Foundation', version 11.0.10605000
configured using `configure  '--with-x-toolkit' '--with-xft'
'--prefix=/home/brentg/install/Linux.x86_64''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: nXML

Minor modes in effect:
  image-minor-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-x C-f C-v / t m p / C-x h <backspace> C-b C-b C-b
C-g C-x C-f C-SPC M-b C-b <backspace> / t m p C-d C-d
C-d C-d C-d / t e s t . s v g <return> C-v <help-echo>
<help-echo> <help-echo> <down-mouse-2> <mouse-2> C-x
C-s C-x C-v <return> C-c C-c C-h k C-M-n M-x r e p
o r t - e m <tab> <return>

Recent messages:
File mode specification error: (error "Cannot determine image type")
call-interactively: End of buffer
Mark set
Saving file /tmp/test.svg...
Wrote /tmp/test.svg
Using vacuous schema
Type C-c C-c to view the image as text.
Repeat this command to go back to displaying the image
Type C-x 1 to delete the help window.

Load-path shadows:
None found.

Features:
(shadow mail-extr message ecomplete rfc822 mml mml-sec password-cache
mm-decode mm-bodies mm-encode mailcap mail-parse rfc2231 rfc2047 rfc2045
qp ietf-drums mailabbrev nnheader gnus-util netrc time-date mm-util
mail-prsvr gmm-utils wid-edit mailheader canlock sha1 hex-util hashcash
mail-utils emacsbug sendmail help-fns help-mode view nxml-uchnm rng-xsd
xsd-regexp rng-cmpct regexp-opt rng-nxml rng-valid rng-loc rng-uri
rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn nxml-ns
easymenu nxml-mode nxml-outln nxml-rap nxml-util nxml-glyph nxml-enc
xmltok cl cl-19 jka-compr image-mode tooltip ediff-hook vc-hooks
lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image fringe
lisp-mode register page menu-bar rfn-eshadow timer select scroll-bar
mldrag mouse jit-lock font-lock syntax facemenu font-core frame cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev loaddefs button minibuffer faces cus-face text-properties overlay
md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind gtk
x-toolkit x multi-tty emacs)


[-- Attachment #3: Type: message/rfc822, Size: 2078 bytes --]

From: Juri Linkov <juri@jurta.org>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: 5062-done@emacsbugs.donarmstrong.com, Brent Goodrick <bgoodr@gmail.com>
Subject: Re: bug#5062: 23.1.50; image-toggle-display overwrites nxml-mode local key map
Date: Fri, 04 Dec 2009 23:47:30 +0200
Message-ID: <878wdixvgt.fsf@mail.jurta.org>

>> If this is ok, I'm ready to update docstrings and commit.
>
> Looks good, thank you,

Done.

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