Image autodetection

Image autodetection

[Chong Yidong]

I have tried to digest the long and confusing thread on this subject.
I propose the following:

1. Make a new variable, auto-detect-image-type-p.  If non-nil, the
   function image-type-auto-detected-p (called from magic-mode-alist)
   performs content detection.  The default is nil.

2. Do not display images as images automatically in image-mode.
   Instead, the user sees "Type C-c C-c to view the image as an
   image".  This additional step should not be a major inconvenience.

The other changes proposed in that thread, such as using
image-minor-mode by default, are probably unnecessary.

Re: Image autodetection

[Juanma Barranquero]

On 1/29/07, Chong Yidong <cyd@stupidchicken.com> wrote:

> The other changes proposed in that thread, such as using
> image-minor-mode by default, are probably unnecessary.

The only advantage of the image-minor-mode method is that users would
receive a security warning when they first run image-toggle-display
(because it would be disabled).

But at this moment, I think any of the proposed fixes is good, as long
as we adopt one and can go forward with the release.

                    /L/e/k/t/u

Re: Image autodetection

[Richard Stallman]

    1. Make a new variable, auto-detect-image-type-p.  If non-nil, the
       function image-type-auto-detected-p (called from magic-mode-alist)
       performs content detection.  The default is nil.

    2. Do not display images as images automatically in image-mode.
       Instead, the user sees "Type C-c C-c to view the image as an
       image".  This additional step should not be a major inconvenience.

If we do #2, I see no need for #1.

Re: Image autodetection

[Miles Bader]

Chong Yidong <cyd@stupidchicken.com> writes:
> 1. Make a new variable, auto-detect-image-type-p.  If non-nil, the
>    function image-type-auto-detected-p (called from magic-mode-alist)
>    performs content detection.  The default is nil.

Don't use a "-p" suffix on variables.

-Miles

-- 
What the fuck do white people have to be blue about!?  Banana Republic ran
out of Khakis?  The Espresso Machine is jammed?  Hootie and The Blowfish are
breaking up??!  Shit, white people oughtta understand, their job is to GIVE
people the blues, not to get them!    -- George Carlin

Re: Image autodetection

[Jason Rumney]

Juanma Barranquero wrote:
> The only advantage of the image-minor-mode method is that users would
> receive a security warning when they first run image-toggle-display
> (because it would be disabled).

There's no reason that image-mode could not do the same.

Re: Image autodetection

[Juanma Barranquero]

On 1/30/07, Jason Rumney <jasonr@gnu.org> wrote:

> There's no reason that image-mode could not do the same.

You're right.

With image-mode, image-minor-mode, image-mode-maybe,
image-type-auto-detected-p, image-type-auto-detectable,
image-type-file-name-regexps, image-type-header-regexps,
image-file-name-extensions, image-file-name-regexps,
etc. we already have too much confusion regarding how and when to set
image-mode; so I like Richard's idea of bypassing #1 of Chong's
proposals (no more options, please!) and just implementing #2 (with
image-toggle-display disabled, as you note).

Either that, or we start scrapping all the above and rethink all this
stuff... which I think would be great, were not for the pretest.
(Though, to be honest, the "confusion" stems from the fact that we
started to teach Emacs to better auto-detect images, and then had to
switch tracks when the security issues came up, so we've finished
conflating two very different issues.)

                    /L/e/k/t/u

Re: Image autodetection

[Juanma Barranquero]

On 1/30/07, Juanma Barranquero <lekktu@gmail.com> wrote:

> so I like Richard's idea of bypassing #1 of Chong's
> proposals (no more options, please!) and just implementing #2 (with
> image-toggle-display disabled, as you note).

Something like the attached patch, which couldn't be simpler.

                    /L/e/k/t/u


Index: lisp/image-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/image-mode.el,v
retrieving revision 1.16
diff -u -2 -r1.16 image-mode.el
--- lisp/image-mode.el	21 Jan 2007 03:53:11 -0000	1.16
+++ lisp/image-mode.el	30 Jan 2007 12:03:44 -0000
@@ -61,10 +61,5 @@
   (use-local-map image-mode-map)
   (add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
-  (if (and (display-images-p)
-	   (not (get-text-property (point-min) 'display)))
-      (image-toggle-display)
-    ;; 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))
+  (setq cursor-type nil truncate-lines t)
   (run-mode-hooks 'image-mode-hook)
   (if (display-images-p)
@@ -180,4 +175,10 @@
 	  (message "Repeat this command to go back to displaying the file as
text")))))

+(put 'image-toggle-display 'disabled "\
+
+WARNING: Displaying images can be a security risk.
+Please make sure you're using up-to-date image libraries
+and the images displayed come from a trusted source.")
+
 (provide 'image-mode)

Re: Image autodetection

[Stefan Monnier]

>> The other changes proposed in that thread, such as using
>> image-minor-mode by default, are probably unnecessary.

> The only advantage of the image-minor-mode method is that users would
> receive a security warning when they first run image-toggle-display

Actually, the way I see it the only real advantage of image-minor-mode is
that if a C file gets wrongly detected as a GIF file, it doesn't really
matter because the C mode is still activated and you just get a bunch of
spurious extra commands to display the "image".


        Stefan


>> so I like Richard's idea of bypassing #1 of Chong's
>> proposals (no more options, please!) and just implementing #2 (with
>> image-toggle-display disabled, as you note).

> Something like the attached patch, which couldn't be simpler.

Looks good to me.

> Index: lisp/image-mode.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/image-mode.el,v
> retrieving revision 1.16
> diff -u -2 -r1.16 image-mode.el
> --- lisp/image-mode.el	21 Jan 2007 03:53:11 -0000	1.16
> +++ lisp/image-mode.el	30 Jan 2007 12:03:44 -0000
> @@ -61,10 +61,5 @@
>   (use-local-map image-mode-map)
>   (add-hook 'change-major-mode-hook 'image-toggle-display-text nil t)
> -  (if (and (display-images-p)
> -	   (not (get-text-property (point-min) 'display)))
> -      (image-toggle-display)
> -    ;; 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))
> +  (setq cursor-type nil truncate-lines t)
>   (run-mode-hooks 'image-mode-hook)
>   (if (display-images-p)
> @@ -180,4 +175,10 @@
> 	  (message "Repeat this command to go back to displaying the file as
> text")))))

> +(put 'image-toggle-display 'disabled "\
> +
> +WARNING: Displaying images can be a security risk.
> +Please make sure you're using up-to-date image libraries
> +and the images displayed come from a trusted source.")
> +
> (provide 'image-mode)


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

Re: Image autodetection

[Juanma Barranquero]

On 1/30/07, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Actually, the way I see it the only real advantage of image-minor-mode is
> that if a C file gets wrongly detected as a GIF file, it doesn't really
> matter because the C mode is still activated and you just get a bunch of
> spurious extra commands to display the "image".

That would be a different change. As it is now, if we just change
`image-mode' to `image-minor-mode' in `magic-mode-alist' (as was done
in a previous proposed patch, now rejected), image files would be
opened in fundamental mode + image minor mode, because as soon as a
non-nil mode is returned when searching in `magic-mode-alist' the
detection of major mode is finished (and so `auto-mode-alist' is
bypassed). At least, that's what I remember from a test I did.

> Looks good to me.

Great.

                    /L/e/k/t/u

Re: Image autodetection

[Jason Rumney]

Juanma Barranquero wrote:
> That would be a different change. As it is now, if we just change
> `image-mode' to `image-minor-mode' in `magic-mode-alist' (as was done
> in a previous proposed patch, now rejected), image files would be
> opened in fundamental mode + image minor mode, because as soon as a
> non-nil mode is returned when searching in `magic-mode-alist' the
> detection of major mode is finished (and so `auto-mode-alist' is
> bypassed). At least, that's what I remember from a test I did.

We have image-mode-maybe that uses image-mode for files that are 
unambiguously images, and image-minor-mode for files where there may be 
another appropriate mode. With some minor changes, it could be used in 
magic-mode-alist.

Re: Image autodetection

[Chong Yidong]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On 1/30/07, Juanma Barranquero <lekktu@gmail.com> wrote:
>
>> so I like Richard's idea of bypassing #1 of Chong's
>> proposals (no more options, please!) and just implementing #2 (with
>> image-toggle-display disabled, as you note).
>
> Something like the attached patch, which couldn't be simpler.

> -  (if (and (display-images-p)
> -	   (not (get-text-property (point-min) 'display)))
> -      (image-toggle-display)
> -    ;; 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))
> +  (setq cursor-type nil truncate-lines t)

This last line should not be present; its purpose was to hide the
cursor in case we are already displaying as an image and run
image-mode again.  I've checked in the patch, with this additional
correction.

Re: Image autodetection

[Juanma Barranquero]

On 1/30/07, Jason Rumney <jasonr@gnu.org> wrote:

> With some minor changes, it could be used in magic-mode-alist.

I see `image-mode-maybe' more as a user command, but if you can make
it work and the changes are really minor it could be a good option
(additionally to the changes already installed by Chong).

                    /L/e/k/t/u