Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[tumashu]

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



[-- Attachment #1.2: Type: text/html, Size: 111 bytes --]

[-- Attachment #2: 0001-lisp-xwidget.el-xwidget-webkit-browse-url-Remove-spa.patch --]
[-- Type: application/octet-stream, Size: 831 bytes --]

From 2f460c83b4e304cb1f6df0f11d54e4cb0da0b223 Mon Sep 17 00:00:00 2001
From: Feng Shu <tumashu@163.com>
Date: Thu, 11 Nov 2021 17:32:53 +0800
Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space
 prefix of url.

---
 lisp/xwidget.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index 2c7b4dd83d..be5a510986 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -114,6 +114,7 @@ xwidget-webkit-browse-url
   (or (featurep 'xwidget-internal)
       (user-error "Your Emacs was not compiled with xwidgets support"))
   (when (stringp url)
+    (setq url (replace-regexp-in-string "^[[:space:]]+" "" url))
     ;; If it's a "naked url", just try adding https: to it.
     (unless (string-match "\\`[A-Za-z]+:" url)
       (setq url (concat "https://" url)))
-- 
2.30.2

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

tumashu <tumashu@163.com> writes:

> From 2f460c83b4e304cb1f6df0f11d54e4cb0da0b223 Mon Sep 17 00:00:00 2001
> From: Feng Shu <tumashu@163.com>
> Date: Thu, 11 Nov 2021 17:32:53 +0800
> Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space
>  prefix of url.
>
> ---
>  lisp/xwidget.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/xwidget.el b/lisp/xwidget.el
> index 2c7b4dd83d..be5a510986 100644
> --- a/lisp/xwidget.el
> +++ b/lisp/xwidget.el
> @@ -114,6 +114,7 @@ xwidget-webkit-browse-url
>    (or (featurep 'xwidget-internal)
>        (user-error "Your Emacs was not compiled with xwidgets support"))
>    (when (stringp url)
> +    (setq url (replace-regexp-in-string "^[[:space:]]+" "" url))
>      ;; If it's a "naked url", just try adding https: to it.
>      (unless (string-match "\\`[A-Za-z]+:" url)
>        (setq url (concat "https://" url)))

Why is this necessary?  I don't think the other `browse-url-function's
do this.

Thanks in advance.

Re:Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[tumashu]

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







--
发自我的网易邮箱手机智能版
<br/><br/><br/>


----- Original Message -----
From: "Po Lu" <luangruo@yahoo.com>
To: tumashu <tumashu@163.com>
Cc: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
Sent: Thu, 11 Nov 2021 17:39:41 +0800
Subject: Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

tumashu <tumashu@163.com> writes:

> From 2f460c83b4e304cb1f6df0f11d54e4cb0da0b223 Mon Sep 17 00:00:00 2001
> From: Feng Shu <tumashu@163.com>
> Date: Thu, 11 Nov 2021 17:32:53 +0800
> Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space
>  prefix of url.
>
> ---
>  lisp/xwidget.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/xwidget.el b/lisp/xwidget.el
> index 2c7b4dd83d..be5a510986 100644
> --- a/lisp/xwidget.el
> +++ b/lisp/xwidget.el
> @@ -114,6 +114,7 @@ xwidget-webkit-browse-url
>    (or (featurep 'xwidget-internal)
>        (user-error "Your Emacs was not compiled with xwidgets support"))
>    (when (stringp url)
> +    (setq url (replace-regexp-in-string "^[[:space:]]+" "" url))
>      ;; If it's a "naked url", just try adding https: to it.
>      (unless (string-match "\\`[A-Za-z]+:" url)
>        (setq url (concat "https://" url)))

Why is this necessary?  I don't think the other `browse-url-function's

do this.





I often encountered in this case, when copy url, space begin url hard notice :)




Thanks in advance.

[-- Attachment #2: Type: text/html, Size: 2682 bytes --]

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

tumashu <tumashu@163.com> writes:

> I often encountered in this case, when copy url, space begin url hard
> notice :)

I don't think a function intended for use as `browse-url-function'
should make those decisions.  And in any case, one should check a URL
before opening it.

On an unrelated note, I think the use of a regexp for stripping
whitespace is unwarranted.  Wouldn't `url-strip-leading-spaces' be a
better candidate for this job?

Thanks.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Why is this necessary?  I don't think the other `browse-url-function's
> do this.

eww basically does the same -- but a lot more.  See
`eww--dwim-expand-url'.  (Perhaps you could just use that, actually.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> eww basically does the same -- but a lot more.  See
> `eww--dwim-expand-url'.  (Perhaps you could just use that, actually.)

Hmm, OK.  I wasn't aware of that.  Would it make sense to move this into
url, or url-util?

Thanks.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> eww basically does the same -- but a lot more.  See
>> `eww--dwim-expand-url'.  (Perhaps you could just use that, actually.)
>
> Hmm, OK.  I wasn't aware of that.  Would it make sense to move this into
> url, or url-util?

I took an actual look at the function now, and it relies on a couple of
eww-specific user options (like eww-search-prefix).  Perhaps those
should be passed in instead, and then the function can be put in
url-util, like

(url-dwim-expand-url url &optional search-prefix local-regexp)

And...  add a couple of new user options (like url-search-prefix)?
Yeah, I think that should do the trick.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I took an actual look at the function now, and it relies on a couple of
> eww-specific user options (like eww-search-prefix).  Perhaps those
> should be passed in instead, and then the function can be put in
> url-util, like
>
> (url-dwim-expand-url url &optional search-prefix local-regexp)
>
> And...  add a couple of new user options (like url-search-prefix)?
> Yeah, I think that should do the trick.

Could you provide a more concrete explanation of what
`eww--dwim-expand-url' currently does, so I can write a doc string?

It would be greatly appreciated, thanks :)

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Could you provide a more concrete explanation of what
> `eww--dwim-expand-url' currently does, so I can write a doc string?
>
> It would be greatly appreciated, thanks :)

Well, it's a DWIM function, so: 😅.  It just tries to do what any
browser does when a user types something into the address bar, and it'll
probably change over time (as users' expectations change).

But basically tries to guess whether a user is typing in a (possibly
incomplete) URL or a search term.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Well, it's a DWIM function, so: 😅.  It just tries to do what any
> browser does when a user types something into the address bar, and it'll
> probably change over time (as users' expectations change).
>
> But basically tries to guess whether a user is typing in a (possibly
> incomplete) URL or a search term.

I'd like to add this:

(defun url-dwim-expand-url (url local-regex search-prefix)
  "Canonicalize URL.
Try to determine if URL is an incomplete URL or a search query, and
return the canonical form of URL.
SEARCH-PREFIX is the prefix to be prepended to URL if it is a search query.
LOCAL-REGEX is a regular expression that URL is matched against.  If the
match is successful, then URL is treated as an address."
  (setq url (string-trim url))
  (cond ((string-match-p "\\`file:/" url))
	;; Don't mangle file: URLs at all.
        ((string-match-p "\\`ftp://" url)
         (user-error "FTP is not supported"))
        (t
	 ;; Anything that starts with something that vaguely looks
	 ;; like a protocol designator is interpreted as a full URL.
         (if (or (string-match "\\`[A-Za-z]+:" url)
		 ;; Also try to match "naked" URLs like
		 ;; en.wikipedia.org/wiki/Free software
		 (string-match "\\`[A-Za-z_]+\\.[A-Za-z._]+/" url)
		 (and (= (length (split-string url)) 1)
		      (or (and (not (string-match-p "\\`[\"'].*[\"']\\'" url))
			       (> (length (split-string url "[.:]")) 1))
			  (string-match local-regex url))))
             (progn
               (unless (string-match-p "\\`[a-zA-Z][-a-zA-Z0-9+.]*://" url)
                 (setq url (concat "http://" url)))
               ;; Some sites do not redirect final /
               (when (string= (url-filename (url-generic-parse-url url)) "")
                 (setq url (concat url "/"))))
           (setq url (concat search-prefix
                             (mapconcat
                              #'url-hexify-string (split-string url) "+"))))))
  url)

to url-util.

Does this look good to you?  Thanks in advance.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: tumashu <tumashu@163.com>,  "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sat, 13 Nov 2021 09:08:50 +0800
> 
> > But basically tries to guess whether a user is typing in a (possibly
> > incomplete) URL or a search term.
> 
> I'd like to add this:

Is this a replacement for eww--dwim-expand-url?  Or is this in
addition to it?

> (defun url-dwim-expand-url (url local-regex search-prefix)
>   "Canonicalize URL.

First line of a doc string should mention all the mandatory
arguments.  (But see below.)

> Try to determine if URL is an incomplete URL or a search query, and
> return the canonical form of URL.
> SEARCH-PREFIX is the prefix to be prepended to URL if it is a search query.

Can the doc string explain what does "search query" mean in this
context?

> LOCAL-REGEX is a regular expression that URL is matched against.  If the
> match is successful, then URL is treated as an address."

This begs the question: and if it doesn't match, then what?  And what
does "address" mean in this context, i.e. what does "treated as an
address" means in practice?

>   (cond ((string-match-p "\\`file:/" url))
> 	;; Don't mangle file: URLs at all.

This comment should be above the line that handles file:// URLs.
Btw, should other URLs be exempt from "mangling"?  AFAIK, there are
many protocols whose syntax we don't really understand in url*.el
code, so shouldn't they all be left alone?

>         ((string-match-p "\\`ftp://" url)
>          (user-error "FTP is not supported"))

I can understand this in EWW, but why should FTP be unsupported in
url-util?

> 	 ;; Anything that starts with something that vaguely looks
> 	 ;; like a protocol designator is interpreted as a full URL.
>          (if (or (string-match "\\`[A-Za-z]+:" url)

This will match Windows-style d:/foo/bar absolute file names.  Is that
what we want?

> 		 (and (= (length (split-string url)) 1)

You are using split-string here to verify that URL has no SPC
characters?  

> 		      (or (and (not (string-match-p "\\`[\"'].*[\"']\\'" url))
> 			       (> (length (split-string url "[.:]")) 1))

It would be good to have a comment here explaining what do these
conditions test.

> 			  (string-match local-regex url))))

This sole use of LOCAL-REGEX hints that maybe it should be an optional
argument.

>              (progn
>                (unless (string-match-p "\\`[a-zA-Z][-a-zA-Z0-9+.]*://" url)
>                  (setq url (concat "http://" url)))

"http", not "https"?  I think the default nowadays is the latter.

>            (setq url (concat search-prefix
>                              (mapconcat
>                               #'url-hexify-string (split-string url) "+"))))))
>   url)

Doesn't this part mean a search query is expected to be in some
specific format?  If so, that format should be documented in the doc
string.

Thanks.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Is this a replacement for eww--dwim-expand-url?  Or is this in
> addition to it?

It's a replacement for `eww--dwim-expand-url', but in url-utils, because
it is too generally useful to warrant staying internal to eww.

>> (defun url-dwim-expand-url (url local-regex search-prefix)
>>   "Canonicalize URL.

> First line of a doc string should mention all the mandatory
> arguments.  (But see below.)

>> Try to determine if URL is an incomplete URL or a search query, and
>> return the canonical form of URL.
>> SEARCH-PREFIX is the prefix to be prepended to URL if it is a search query.

> Can the doc string explain what does "search query" mean in this
> context?

>> match is successful, then URL is treated as an address."

> This begs the question: and if it doesn't match, then what?  And what
> does "address" mean in this context, i.e. what does "treated as an
> address" means in practice?

>>   (cond ((string-match-p "\\`file:/" url))
>> 	;; Don't mangle file: URLs at all.

> This comment should be above the line that handles file:// URLs.
> Btw, should other URLs be exempt from "mangling"?  AFAIK, there are
> many protocols whose syntax we don't really understand in url*.el
> code, so shouldn't they all be left alone?

This is code I extracted from eww.el; Lars should know the reasoning for
that better.

>>         ((string-match-p "\\`ftp://" url)
>>          (user-error "FTP is not supported"))

> I can understand this in EWW, but why should FTP be unsupported in
> url-util?

Hmm, makes sense.  See below.

>> 	 ;; Anything that starts with something that vaguely looks
>> 	 ;; like a protocol designator is interpreted as a full URL.
>>          (if (or (string-match "\\`[A-Za-z]+:" url)

> This will match Windows-style d:/foo/bar absolute file names.  Is that
> what we want?

No, but in this case eww should have to be fixed too.

>> 		 (and (= (length (split-string url)) 1)

> You are using split-string here to verify that URL has no SPC
> characters?  

That's what the original code in eww.el did.  I didn't dare change it,
because I didn't know why it was there.

>> 		      (or (and (not (string-match-p "\\`[\"'].*[\"']\\'" url))
>> 			       (> (length (split-string url "[.:]")) 1))

> It would be good to have a comment here explaining what do these
> conditions test.

Lars, do you know what this test is for?  Thanks.

>> 			  (string-match local-regex url))))

> This sole use of LOCAL-REGEX hints that maybe it should be an optional
> argument.
>
>>              (progn
>>                (unless (string-match-p "\\`[a-zA-Z][-a-zA-Z0-9+.]*://" url)
>>                  (setq url (concat "http://" url)))
>
> "http", not "https"?  I think the default nowadays is the latter.
>
>>            (setq url (concat search-prefix
>>                              (mapconcat
>>                               #'url-hexify-string (split-string url) "+"))))))
>>   url)

> Doesn't this part mean a search query is expected to be in some
> specific format?  If so, that format should be documented in the doc
> string.

I don't know: Lars, do you know why this is here?  Thanks in advance.

> Thanks.

Thanks.  Here's a better docstring, but I don't want to change any of
the code until I know precisely what it does.

Canonicalize URL, with SEARCH-PREFIX if URL seems to be a search query.

Try to determine if URL is an address or a query for a search engine,
and return canonicalized URL, with SEARCH-PREFIX prepended before
cannibalisation if it seems to be such a query.  Optional argument
LOCAL-REGEX is a regular expression that URL is matched against.  If the
match is successful, then URL is treated as an address, and not a search
query.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: tumashu@163.com,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Sat, 13 Nov 2021 17:34:50 +0800
> 
> Thanks.  Here's a better docstring, but I don't want to change any of
> the code until I know precisely what it does.
> 
> Canonicalize URL, with SEARCH-PREFIX if URL seems to be a search query.
> 
> Try to determine if URL is an address or a query for a search engine,
> and return canonicalized URL, with SEARCH-PREFIX prepended before
> cannibalisation if it seems to be such a query.  Optional argument
> LOCAL-REGEX is a regular expression that URL is matched against.  If the
> match is successful, then URL is treated as an address, and not a search
> query.

I'd improve like this:

    "Canonicalize incomplete URL using SEARCH-PREFIX if URL is a search query.

  Canonicalization means:

    - prepend a scheme (a.k.a. \"protocol\") if it's missing
    - append a slash if it's missing
    - if URL is a search query, prepend SEARCH-PREFIX and hexify special
      characters using `url-hexify-string'

  Optional arg LOCAL-REGEX, if non-nil, means don't treat URLs that
  match the regex as queries even if they otherwise look like it."

Given the meaning of LOCAL-REGEX (if it is indeed described correctly
above), I'd ask why its name is _LOCAL_-REGEX, not something like
ADDRESS-REGEXP?  there's nothing "local" about such an address, is
there?

Thanks.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd improve like this:
>
>     "Canonicalize incomplete URL using SEARCH-PREFIX if URL is a search query.
>
>   Canonicalization means:
>
>     - prepend a scheme (a.k.a. \"protocol\") if it's missing
>     - append a slash if it's missing
>     - if URL is a search query, prepend SEARCH-PREFIX and hexify special
>       characters using `url-hexify-string'
>
>   Optional arg LOCAL-REGEX, if non-nil, means don't treat URLs that
>   match the regex as queries even if they otherwise look like it."
                                                                ^^^

I think "one" would make more sense here, thanks.

> Given the meaning of LOCAL-REGEX (if it is indeed described correctly
> above), I'd ask why its name is _LOCAL_-REGEX, not something like
> ADDRESS-REGEXP?  there's nothing "local" about such an address, is
> there?

Thanks, I derived the name of the argument from `eww-local-regex'.  But
`address-regexp' would indeed make more sense.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: tumashu@163.com,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Sat, 13 Nov 2021 18:45:26 +0800
> 
> >   Optional arg LOCAL-REGEX, if non-nil, means don't treat URLs that
> >   match the regex as queries even if they otherwise look like it."
> 
> I think "one" would make more sense here, thanks.

"Queries" is plural, so "one" doesn't match.

Re: Subject: [PATCH] * lisp/xwidget.el (xwidget-webkit-browse-url): Remove space prefix of url.

[Stephen Berman]

On Sat, 13 Nov 2021 15:29:26 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: tumashu@163.com,  larsi@gnus.org,  emacs-devel@gnu.org
>> Date: Sat, 13 Nov 2021 18:45:26 +0800
>>
>> >   Optional arg LOCAL-REGEX, if non-nil, means don't treat URLs that
>> >   match the regex as queries even if they otherwise look like it."
>>
>> I think "one" would make more sense here, thanks.
>
> "Queries" is plural, so "one" doesn't match.

I think it's grammatically and stylistically fine to repeat "queries"
(instead of using "it" or "one"), but it's also fine to simply drop
"even if they otherwise look like it", since that phrase just emphasizes
the intent without qualifying it.

Steve Berman