From: Juri Linkov <juri@jurta.org>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: Tassilo Horn <tassilo@member.fsf.org>,
Brent Goodrick <bgoodr@gmail.com>,
5062@emacsbugs.donarmstrong.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 [thread overview]
Message-ID: <87fx7x5np9.fsf@mail.jurta.org> (raw)
In-Reply-To: <jwvocmlpcnr.fsf-monnier+emacsbugreports@gnu.org> (Stefan Monnier's message of "Sun, 29 Nov 2009 10:36:59 -0500")
>>> 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/
next prev parent reply other threads:[~2009-11-29 16:03 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <878wdixvgt.fsf@mail.jurta.org>
2009-11-28 0:44 ` bug#5062: 23.1.50; image-toggle-display overwrites nxml-mode local key map Brent Goodrick
2009-11-28 2:25 ` Stefan Monnier
2009-11-28 15:26 ` Brent Goodrick
2009-11-28 17:49 ` Juri Linkov
2009-11-28 20:21 ` Stefan Monnier
2009-11-28 22:54 ` Juri Linkov
2009-11-29 15:36 ` Stefan Monnier
2009-11-29 16:03 ` Juri Linkov [this message]
2009-11-29 18:33 ` Stefan Monnier
2009-11-29 22:00 ` Lennart Borgman
2009-11-29 22:08 ` Juri Linkov
2009-11-29 23:16 ` Lennart Borgman
2009-12-03 0:59 ` bug#5062: " Juri Linkov
2009-12-03 1:37 ` Lennart Borgman
2009-12-03 3:08 ` Kevin Rodgers
2009-12-03 3:31 ` Lennart Borgman
2009-12-03 3:30 ` Stefan Monnier
2009-12-03 0:57 ` bug#5062: 23.1.50; " Juri Linkov
2009-12-03 3:28 ` Stefan Monnier
2009-12-03 5:00 ` Jason Rumney
2009-12-04 0:05 ` Juri Linkov
2009-12-04 22:00 ` bug#5062: marked as done (23.1.50; image-toggle-display overwrites nxml-mode local key map) Emacs bug Tracking System
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87fx7x5np9.fsf@mail.jurta.org \
--to=juri@jurta.org \
--cc=5062@emacsbugs.donarmstrong.com \
--cc=bgoodr@gmail.com \
--cc=monnier@iro.umontreal.ca \
--cc=tassilo@member.fsf.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).