[jidanni@jidanni.org: ffap; prompt turns color misleadingly]

[jidanni@jidanni.org: ffap; prompt turns color misleadingly]

[Richard Stallman]

I doubt that Michelangelo Grigni is still involved in ffap.el.
Someone else interested in ffap.el might want to work on this.

------- Start of forwarded message -------
To: Michelangelo Grigni <mic@mathcs.emory.edu>
From: jidanni@jidanni.org
Date: Mon, 31 Dec 2007 07:45:50 +0800
Message-ID: <87ejd3d9w1.fsf@jidanni.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: bug-gnu-emacs@gnu.org
Subject: ffap; prompt turns color misleadingly

$ emacs -Q file_with_url_in_it
Put the cursor on the URL, do M-x ffap.
Now add some slashes at the end of the URL. The preceding part of the URL
changes color, making us think that we are now just referring to the local
file system as the usual shortcut: /wrong//right, but in fact it is all
appended to the URL anyway!

Also please use a third color for the "http:/" and "ftp:/", or use the
same color as the rest of the URL. Currently you use the "discarded
text" color, which is not exactly right, as it is indeed part of the
URL.

Emacs  : GNU Emacs 22.1.1 (i486-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
of 2007-12-15 on noname, modified by Debian
ffap-version "1.9-fsf <97/06/25 13:21:41 mic>"
By the way, the date 97 is worrisome, although the ffap.el says
copyright 2007.
------- End of forwarded message -------

rfn-eshadow (Was: [jidanni@jidanni.org: ffap; prompt turns color misleadingly])

[Juri Linkov]

> I doubt that Michelangelo Grigni is still involved in ffap.el.
> Someone else interested in ffap.el might want to work on this.

ffap.el mentions FSF as a maintainer, but actually this bug has nothing
to do with ffap.

The problem is that even if ffap is disabled, when typing an URL in the
minibuffer, rfn-eshadow.el threats it as a file name, and puts a shadow
over the `http:' part because it contains double slashes //.

I think the right way to fix this problem is the following patch that
prevents handling of URLs by file-name-shadow-mode:

Index: lisp/rfn-eshadow.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/rfn-eshadow.el,v
retrieving revision 1.26
diff -u -w -b -r1.26 rfn-eshadow.el
--- lisp/rfn-eshadow.el	21 Sep 2007 05:24:05 -0000	1.26
+++ lisp/rfn-eshadow.el	3 Jan 2008 21:36:44 -0000
@@ -177,7 +177,10 @@
 `file-name-shadow-mode'; the minibuffer should have already
 been set up by `rfn-eshadow-setup-minibuffer'."
   (condition-case nil
-      (let ((goal (substitute-in-file-name (minibuffer-contents)))
+      (let* ((mc (minibuffer-contents))
+	     (goal (if (string-match-p "\\`\\(https?\\|ftp\\|file\\)://" mc)
+		       mc
+		     (substitute-in-file-name mc)))
             (mid (overlay-end rfn-eshadow-overlay))
             (start (minibuffer-prompt-end))
             (end (point-max)))

> From: jidanni@jidanni.org
> Subject: ffap; prompt turns color misleadingly
> To: Michelangelo Grigni <mic@mathcs.emory.edu>
> Cc: bug-gnu-emacs@gnu.org
> Date: Mon, 31 Dec 2007 07:45:50 +0800
>
> $ emacs -Q file_with_url_in_it
> Put the cursor on the URL, do M-x ffap.
> Now add some slashes at the end of the URL. The preceding part of the URL
> changes color, making us think that we are now just referring to the local
> file system as the usual shortcut: /wrong//right, but in fact it is all
> appended to the URL anyway!
>
> Also please use a third color for the "http:/" and "ftp:/", or use the
> same color as the rest of the URL. Currently you use the "discarded
> text" color, which is not exactly right, as it is indeed part of the URL.

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

Re: rfn-eshadow

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

> The problem is that even if ffap is disabled, when typing an URL in the
> minibuffer, rfn-eshadow.el threats it as a file name, and puts a shadow
> over the `http:' part because it contains double slashes //.
>
> I think the right way to fix this problem is the following patch that
> prevents handling of URLs by file-name-shadow-mode:
>
> Index: lisp/rfn-eshadow.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/rfn-eshadow.el,v
> retrieving revision 1.26
> diff -u -w -b -r1.26 rfn-eshadow.el
> --- lisp/rfn-eshadow.el	21 Sep 2007 05:24:05 -0000	1.26
> +++ lisp/rfn-eshadow.el	3 Jan 2008 21:36:44 -0000
> @@ -177,7 +177,10 @@
>  `file-name-shadow-mode'; the minibuffer should have already
>  been set up by `rfn-eshadow-setup-minibuffer'."
>    (condition-case nil
> -      (let ((goal (substitute-in-file-name (minibuffer-contents)))
> +      (let* ((mc (minibuffer-contents))
> +	     (goal (if (string-match-p "\\`\\(https?\\|ftp\\|file\\)://" mc)
> +		       mc
> +		     (substitute-in-file-name mc)))

I'm not in favour of this solution, because it introduces file name
handling at a place we won't remember in the future. The clean way would
be to add a file name handler, which offers its own implementation of
substitute-in-file-name.

Doesn't exist something like this already in the url package?

Best regards, Michael.

Re: rfn-eshadow

[Juri Linkov]

> I'm not in favour of this solution, because it introduces file name
> handling at a place we won't remember in the future. The clean way would
> be to add a file name handler, which offers its own implementation of
> substitute-in-file-name.

Actually, this is not a special URL handling, but a way to disable
handling of URLs by rfn-eshadow.  Or do you propose adding two
file name handlers to rfn-eshadow: one to handle all file names
(with a handler regexp ".*") and another to handle URLs?

> Doesn't exist something like this already in the url package?

`url-handler-mode' minor mode implements such a handling, but I'm not sure
we should enable it by default.

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

Re: rfn-eshadow

[Stefan Monnier]

> ffap.el mentions FSF as a maintainer, but actually this bug has nothing
> to do with ffap.

On the contrary it has everything to do with ffap.

> The problem is that even if ffap is disabled, when typing an URL in the
> minibuffer, rfn-eshadow.el threats it as a file name, and puts a shadow
> over the `http:' part because it contains double slashes //.

No, it is perfectly right for rfn-eshadow to treat it as a file name,
since that's what rfn-eshadow is all about: to improve file-name entry.
If you want your user to enter something else than a file-name, then
don't set minibuffer-completing-file-name.

> I think the right way to fix this problem is the following patch that
> prevents handling of URLs by file-name-shadow-mode:

No, the right way is for ffap to either not set
minibuffer-completing-file-name (which probably requires more
changes in order to recover the desired completion behavior) or to
explain to rfn-eshadow what kind of file-names are accepted via
a file-name-handler (which can be added only temporarily).

For the record, URL entry works just fine with rfn-eshadow if you enable
url-handler-mode.


        Stefan

Re: rfn-eshadow

[Juri Linkov]

>> ffap.el mentions FSF as a maintainer, but actually this bug has nothing
>> to do with ffap.
>
> On the contrary it has everything to do with ffap.

Do you mean ffap is the only package that reads file names and URLs
in the same minibuffer and thus needs a special handling?

>> I think the right way to fix this problem is the following patch that
>> prevents handling of URLs by file-name-shadow-mode:
>
> No, the right way is for ffap to either not set
> minibuffer-completing-file-name (which probably requires more
> changes in order to recover the desired completion behavior) or to
> explain to rfn-eshadow what kind of file-names are accepted via
> a file-name-handler (which can be added only temporarily).
>
> For the record, URL entry works just fine with rfn-eshadow if you enable
> url-handler-mode.

I don't think url-handler-mode should be enabled by ffap, but maybe a similar
handling of file-name-handler could be added to ffap.

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

Re: rfn-eshadow

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

>> For the record, URL entry works just fine with rfn-eshadow if you enable
>> url-handler-mode.
>
> I don't think url-handler-mode should be enabled by ffap, but maybe a similar
> handling of file-name-handler could be added to ffap.

What's about this patch?

*** /usr/local/src/emacs/lisp/ffap.el.~1~	Fri Nov 30 22:34:18 2007
--- /usr/local/src/emacs/lisp/ffap.el	Fri Jan  4 12:58:57 2008
***************
*** 1262,1268 ****
  		    ))
  	  (setq dir (file-name-directory guess))))
      (let ((minibuffer-completing-file-name t)
! 	  (completion-ignore-case read-file-name-completion-ignore-case))
        (setq guess
  	    (completing-read
  	     prompt
--- 1262,1270 ----
  		    ))
  	  (setq dir (file-name-directory guess))))
      (let ((minibuffer-completing-file-name t)
! 	  (completion-ignore-case read-file-name-completion-ignore-case)
! 	  (file-name-handler-alist (copy-tree file-name-handler-alist)))
!       (url-handler-mode t)
        (setq guess
  	    (completing-read
  	     prompt

Re: rfn-eshadow

[Juri Linkov]

>>> For the record, URL entry works just fine with rfn-eshadow if you enable
>>> url-handler-mode.
>>
>> I don't think url-handler-mode should be enabled by ffap, but maybe a similar
>> handling of file-name-handler could be added to ffap.
>
> What's about this patch?
>
> *** /usr/local/src/emacs/lisp/ffap.el.~1~	Fri Nov 30 22:34:18 2007
> --- /usr/local/src/emacs/lisp/ffap.el	Fri Jan  4 12:58:57 2008
> ***************
> *** 1262,1268 ****
>   		    ))
>   	  (setq dir (file-name-directory guess))))
>       (let ((minibuffer-completing-file-name t)
> ! 	  (completion-ignore-case read-file-name-completion-ignore-case))
>         (setq guess
>   	    (completing-read
>   	     prompt
> --- 1262,1270 ----
>   		    ))
>   	  (setq dir (file-name-directory guess))))
>       (let ((minibuffer-completing-file-name t)
> ! 	  (completion-ignore-case read-file-name-completion-ignore-case)
> ! 	  (file-name-handler-alist (copy-tree file-name-handler-alist)))
> !       (url-handler-mode t)
>         (setq guess
>   	    (completing-read
>   	     prompt

Doesn't look like a right solution to me since url-handlers.el causes
loading two additional files from the url package and two files from gnus.

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

Re: rfn-eshadow

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

> Doesn't look like a right solution to me since url-handlers.el causes
> loading two additional files from the url package and two files from gnus.

But most of the url* files are already loaded when applying
`ffap'. Start "emacs -Q", and compare `load-history' before and after
calling `ffap' on a URL-like string.

Best regards, Michael.

Re: rfn-eshadow

[Richard Stallman]

    No, it is perfectly right for rfn-eshadow to treat it as a file name,
    since that's what rfn-eshadow is all about: to improve file-name entry.

I think you are right.  If the supposed URL is going to be interpreted
as a file name, then it is right for rfn-eshadow to treat it as a file
name too.

Re: rfn-eshadow

[Juri Linkov]

>> Doesn't look like a right solution to me since url-handlers.el causes
>> loading two additional files from the url package and two files from gnus.
>
> But most of the url* files are already loaded when applying
> `ffap'. Start "emacs -Q", and compare `load-history' before and after
> calling `ffap' on a URL-like string.

Ah, I didn't notice that ffap already loads url* files via url-normalize-url.
In this case there is no problem with that.  The only problem I see is that
after reading an URL by ffap, the value of the variable `url-handler-mode'
is t, but really this mode is not active since file-name-handler-alist
doesn't keep the corresponding entry.  You can fix this by adding an empty
let-binding in `ffap-read-file-or-url' for the variable `url-handler-mode'
exactly as you did for `file-name-handler-alist' to not keep its new value.
If nobody objects, could you also fix this in the Emacs 22 branch?

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

Re: rfn-eshadow

[Stefan Monnier]

>>> Doesn't look like a right solution to me since url-handlers.el causes
>>> loading two additional files from the url package and two files from gnus.
>> 
>> But most of the url* files are already loaded when applying
>> `ffap'. Start "emacs -Q", and compare `load-history' before and after
>> calling `ffap' on a URL-like string.

> Ah, I didn't notice that ffap already loads url* files via url-normalize-url.
> In this case there is no problem with that.  The only problem I see is that
> after reading an URL by ffap, the value of the variable `url-handler-mode'
> is t, but really this mode is not active since file-name-handler-alist
> doesn't keep the corresponding entry.  You can fix this by adding an empty
> let-binding in `ffap-read-file-or-url' for the variable `url-handler-mode'
> exactly as you did for `file-name-handler-alist' to not keep its new value.
> If nobody objects, could you also fix this in the Emacs 22 branch?

Of course additional let bindings are needed for every variable modified
by the call to url-handle-mode.  So you end up writing code that makes
assumptions about the inner working of url-handler-mode.
Maybe we may as well just add (cons url-handler-regexp
'url-file-handler) to file-name-handler-mode rather than call
url-handler-mode.


        Stefan

Re: rfn-eshadow

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Of course additional let bindings are needed for every variable modified
> by the call to url-handle-mode.  So you end up writing code that makes
> assumptions about the inner working of url-handler-mode.
> Maybe we may as well just add (cons url-handler-regexp
> 'url-file-handler) to file-name-handler-mode rather than call
> url-handler-mode.

I did it this way in EMCS_22_BASE, using `ffap-url-regexp'.  Shall be
propagated to the trunk next days.

>         Stefan

Best regards, Michael.

Re: rfn-eshadow

[Juri Linkov]

> I did it this way in EMCS_22_BASE, using `ffap-url-regexp'.  Shall be
> propagated to the trunk next days.

Thanks, but it now produces weird results, e.g. removing a letter from
the beginning of an URL and retyping it again doesn't remove the file name
shadow.  This is because url-handlers.el is not loaded, and url-file-handler
is undefined.  I see two variants of fixing this: either by explicitly
loading url-handlers.el, or adding an ###autoload cookie to url-file-handler
(I think this is better).

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

Re: rfn-eshadow

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

> Thanks, but it now produces weird results, e.g. removing a letter from
> the beginning of an URL and retyping it again doesn't remove the file name
> shadow.  This is because url-handlers.el is not loaded, and url-file-handler
> is undefined.  I see two variants of fixing this: either by explicitly
> loading url-handlers.el, or adding an ###autoload cookie to url-file-handler
> (I think this is better).

I've done the latter.

Best regards, Michael.

Re: rfn-eshadow

[Juri Linkov]

>> Thanks, but it now produces weird results, e.g. removing a letter from
>> the beginning of an URL and retyping it again doesn't remove the file name
>> shadow.  This is because url-handlers.el is not loaded, and url-file-handler
>> is undefined.  I see two variants of fixing this: either by explicitly
>> loading url-handlers.el, or adding an ###autoload cookie to url-file-handler
>> (I think this is better).
>
> I've done the latter.

After your changes arrived to the trunk I observe a strange behavior:

1. emacs -Q
2. M-x ffap-bindings RET
3. C-x C-f C-a C-k / s u : : / C-g
4. C-h v file-name-handler-alist RET

And `file-name-handler-alist' doesn't contain `tramp-file-name-handler'
anymore.  But in the Emacs 22 branch everything is correct.
Do you see the same?

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

Re: rfn-eshadow

[Stefan Monnier]

>>> Thanks, but it now produces weird results, e.g. removing a letter from
>>> the beginning of an URL and retyping it again doesn't remove the file name
>>> shadow.  This is because url-handlers.el is not loaded, and url-file-handler
>>> is undefined.  I see two variants of fixing this: either by explicitly
>>> loading url-handlers.el, or adding an ###autoload cookie to url-file-handler
>>> (I think this is better).
>> 
>> I've done the latter.

> After your changes arrived to the trunk I observe a strange behavior:

> 1. emacs -Q
> 2. M-x ffap-bindings RET
> 3. C-x C-f C-a C-k / s u : : / C-g
> 4. C-h v file-name-handler-alist RET

> And `file-name-handler-alist' doesn't contain `tramp-file-name-handler'
> anymore.  But in the Emacs 22 branch everything is correct.
> Do you see the same?

Indeed, in the above recipe, Tramp gets loaded while we're inside the
ffap, i.e. while inside the `let' binding, so the
tramp-file-name-handler' gets removed when get out of the let
binding :-(
I.e. we shouldn't use let-binding.  I installed the untested patch below
which should hopefully fix this problem.


        Stefan


--- ffap.el.~1.68.~	2008-01-08 21:08:40.000000000 -0500
+++ ffap.el	2008-01-08 21:13:19.000000000 -0500
@@ -1263,10 +1263,10 @@
 	  (setq dir (file-name-directory guess))))
     (let ((minibuffer-completing-file-name t)
 	  (completion-ignore-case read-file-name-completion-ignore-case)
-	  ;; because of `rfn-eshadow-update-overlay'.
-	  (file-name-handler-alist
-	   (cons (cons ffap-url-regexp 'url-file-handler)
-		 file-name-handler-alist)))
+          (fnh-elem (cons ffap-url-regexp 'url-file-handler)))
+      ;; Explain to `rfn-eshadow' that we can use URLs here.
+      (push fnh-elem file-name-handler-alist)
+      (unwind-protect
       (setq guess
 	    (completing-read
 	     prompt
@@ -1276,7 +1276,12 @@
 	     (if dir (cons guess (length dir)) guess)
 	     (list 'file-name-history)
 	     (and buffer-file-name
-		  (abbreviate-file-name buffer-file-name)))))
+                      (abbreviate-file-name buffer-file-name))))
+        ;; Remove the special handler manually.  We used to just let-bind
+        ;; file-name-handler-alist to preserve its value, but that caused
+        ;; other modifications to be lost (e.g. when Tramp gets loaded
+        ;; during the completing-read call).
+        (setq file-name-handler-alist (delq fnh-elem file-name-handler-alist))))
     ;; Do file substitution like (interactive "F"), suggested by MCOOK.
     (or (ffap-url-p guess) (setq guess (substitute-in-file-name guess)))
     ;; Should not do it on url's, where $ is a common (VMS?) character.

Re: rfn-eshadow

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Indeed, in the above recipe, Tramp gets loaded while we're inside the
> ffap, i.e. while inside the `let' binding, so the
> tramp-file-name-handler' gets removed when get out of the let
> binding :-(
> I.e. we shouldn't use let-binding.  I installed the untested patch below
> which should hopefully fix this problem.

Likely it does (I have no recent checked out version at hand).

But ... wouldn't be the clean solution to simply call url-handler-mode,
and let it be active? Why do we need to remove url-file-handler from
file-name-handler-alist afterwards?

If somebody copies a url as filename into the minibuffer (that's ffap
functionality), and intends to load this file, this seems to be TRT.

And if url-handler-mode hurts, it needs to be fixed there.

>         Stefan

Best regards, Michael.