Package: emacs;
Reported by: Brent Goodrick <bgoodr <at> gmail.com>
Date: Sat, 28 Nov 2009 00:50:04 UTC
Severity: normal
Done: Juri Linkov <juri <at> jurta.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 5062 <at> debbugs.gnu.org, Tassilo Horn <tassilo <at> member.fsf.org>,
Brent Goodrick <bgoodr <at> gmail.com>
Subject: bug#5062: 23.1.50; image-toggle-display overwrites nxml-mode local key map
Date: Sun, 29 Nov 2009 18:03:50 +0200
>>> 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/
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.