bug#16204: eww does not respect shr-width customization

bug#16204: eww does not respect shr-width customization

[Ivan Shmakov]

Package:  emacs
Severity: wishlist

	As currently implemented, eww-display-html simply resets
	shr-width to nil, disrespecting any user’s customization
	thereof, and providing no easy way to specify the HTML rendering
	width to use in EWW buffers.

-- 
FSF associate member #7257

bug#16204: eww does not respect shr-width customization

[Kenjiro NAKAYAMA]

> 	As currently implemented, eww-display-html simply resets
> 	shr-width to nil, disrespecting any user’s customization
> 	thereof, and providing no easy way to specify the HTML rendering
> 	width to use in EWW buffers.

Yes, I think so too.
I wrote the patch to cusomize rendering-width(shr-width) easily by users.

Signed-off-by: Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com>

        * net/eww.el(eww-rendering-width): New value to set rendering width.
        (eww-mode-map): New key map to eww-set-rendering-width.
        (easy-menu-define): New easy menu of eww-set-rendering-width.
        (eww-set-rendering-width): New function to set rendering width.
        (eww-display-html): Change to enable shr-width to set by users.
        Suggested by Ivan Shmakov <ivan@siamics.net>

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 2263f81..43adb41 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -129,6 +129,7 @@ See also `eww-form-checkbox-selected-symbol'."
 (defvar eww-home-url nil)
 (defvar eww-start-url nil)
 (defvar eww-contents-url nil)
+(defvar eww-rendering-width nil)

 (defvar eww-local-regex "localhost"
   "When this regex is found in the URL, it's not a keyword but an address.")
@@ -255,7 +256,7 @@ word(s) will be searched for via `eww-search-prefix'."
     (setq eww-current-dom document)
     (let ((inhibit-read-only t)
 	  (after-change-functions nil)
-	  (shr-width nil)
+	  (shr-width eww-rendering-width)
 	  (shr-target-id (url-target (url-generic-parse-url url)))
 	  (shr-external-rendering-functions
 	   '((title . eww-tag-title)
@@ -419,6 +420,7 @@ word(s) will be searched for via `eww-search-prefix'."
     (define-key map "B" 'eww-list-bookmarks)
     (define-key map [(meta n)] 'eww-next-bookmark)
     (define-key map [(meta p)] 'eww-previous-bookmark)
+    (define-key map [(meta s)] 'eww-set-rendering-width)

     (easy-menu-define nil map ""
       '("Eww"
@@ -435,7 +437,8 @@ word(s) will be searched for via `eww-search-prefix'."
 	["Copy page URL" eww-copy-page-url t]
 	["Add bookmark" eww-add-bookmark t]
 	["List bookmarks" eww-list-bookmarks t]
-	["List cookies" url-cookie-list t]))
+	["List cookies" url-cookie-list t]
+	["Set rendering width" eww-set-rendering-width t]))
     map))

 (define-derived-mode eww-mode nil "eww"
@@ -543,6 +546,15 @@ appears in a <link> or <a> tag."
   (url-retrieve eww-current-url 'eww-render
 		(list eww-current-url (point))))

+(defun eww-set-rendering-width (width)
+  "Set the redering width."
+  (interactive "nSet new redering width (0: window-width) :")
+  (if (zerop width)
+      (setq eww-rendering-width nil)
+    (if (wholenump width)
+	(setq eww-rendering-width width)
+      (message "Set Number to rendering width"))))
+
 ;; Form support.

 (defvar eww-form nil)
---

Kenjiro NAKAYAMA

ivan@siamics.net writes:

> Package:  emacs
> Severity: wishlist
>
> 	As currently implemented, eww-display-html simply resets
> 	shr-width to nil, disrespecting any user’s customization
> 	thereof, and providing no easy way to specify the HTML rendering
> 	width to use in EWW buffers.

bug#16204: eww does not respect shr-width customization

[Ivan Shmakov]

>>>>> Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com> writes:
>>>>> ivan@siamics.net writes:

 >> Package: emacs Severity: wishlist

 >> As currently implemented, eww-display-html simply resets shr-width
 >> to nil, disrespecting any user’s customization thereof, and
 >> providing no easy way to specify the HTML rendering width to use in
 >> EWW buffers.

 > Yes, I think so too.  I wrote the patch to cusomize rendering-width
 > (shr-width) easily by users.

	ACK, thanks!

[…]

 > --- a/lisp/net/eww.el
 > +++ b/lisp/net/eww.el
 > @@ -129,6 +129,7 @@ See also `eww-form-checkbox-selected-symbol'."
 >  (defvar eww-home-url nil)
 >  (defvar eww-start-url nil)
 >  (defvar eww-contents-url nil)
 > +(defvar eww-rendering-width nil)

	Shouldn’t it rather be defcustom?

 >  (defvar eww-local-regex "localhost"
 >    "When this regex is found in the URL, it's not a keyword but an address.")
 > @@ -255,7 +256,7 @@ word(s) will be searched for via `eww-search-prefix'."
 >      (setq eww-current-dom document)
 >      (let ((inhibit-read-only t)
 >  	  (after-change-functions nil)
 > -	  (shr-width nil)
 > +	  (shr-width eww-rendering-width)

	I’d also ask for a separate “do not override” value.  That is:

   (shr-width (if (DO-NOT-SET-P eww-rendering-width)
                  shr-width
                eww-rendering-width))

	(And, similarly, defcustom’s :type above being a ‘choice’.)

 >  	  (shr-target-id (url-target (url-generic-parse-url url)))
 >  	  (shr-external-rendering-functions
 >  	   '((title . eww-tag-title)

[…]

 > @@ -543,6 +546,15 @@ appears in a <link> or <a> tag."
 >    (url-retrieve eww-current-url 'eww-render
 >  		(list eww-current-url (point))))

 > +(defun eww-set-rendering-width (width)
 > +  "Set the redering width."
 > +  (interactive "nSet new redering width (0: window-width) :")
 > +  (if (zerop width)
 > +      (setq eww-rendering-width nil)
 > +    (if (wholenump width)
 > +	(setq eww-rendering-width width)
 > +      (message "Set Number to rendering width"))))

	Shouldn’t this rather be ‘error’?

 > +
 >  ;; Form support.

 >  (defvar eww-form nil)

-- 
FSF associate member #7257

bug#16204: eww does not respect shr-width customization

[Kenjiro NAKAYAMA]

Thank you for your review. But I have some comments. I'm sorry if I
misunderstood.

>     I’d also ask for a separate “do not override” value.  That is:
>
>    (shr-width (if (DO-NOT-SET-P eww-rendering-width)
>                   shr-width
>                 eww-rendering-width))

I'm not sure how to work well with this logic.
I think shr-width should be overridden by nil or new rendering-width
every time, since previous windows-width will be set to shr-width next rendering.

When users change their window-width, the shr-width should be current
window-width, not previous width.

>  > +(defvar eww-rendering-width nil)
>
> 	Shouldn’t it rather be defcustom?

For the above reason, I think defvar is ok.

> 	Shouldn’t this rather be ‘error’?

Yes, it is my mistake. Sorry.

Kenjiro Nakayama


ivan@siamics.net writes:

>>>>>> Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com> writes:
>>>>>> ivan@siamics.net writes:
>
>  >> Package: emacs Severity: wishlist
>
>  >> As currently implemented, eww-display-html simply resets shr-width
>  >> to nil, disrespecting any user’s customization thereof, and
>  >> providing no easy way to specify the HTML rendering width to use in
>  >> EWW buffers.
>
>  > Yes, I think so too.  I wrote the patch to cusomize rendering-width
>  > (shr-width) easily by users.
>
> 	ACK, thanks!
>
> […]
>
>  > --- a/lisp/net/eww.el
>  > +++ b/lisp/net/eww.el
>  > @@ -129,6 +129,7 @@ See also `eww-form-checkbox-selected-symbol'."
>  >  (defvar eww-home-url nil)
>  >  (defvar eww-start-url nil)
>  >  (defvar eww-contents-url nil)
>  > +(defvar eww-rendering-width nil)
>
> 	Shouldn’t it rather be defcustom?
>
>  >  (defvar eww-local-regex "localhost"
>  >    "When this regex is found in the URL, it's not a keyword but an address.")
>  > @@ -255,7 +256,7 @@ word(s) will be searched for via `eww-search-prefix'."
>  >      (setq eww-current-dom document)
>  >      (let ((inhibit-read-only t)
>  >  	  (after-change-functions nil)
>  > -	  (shr-width nil)
>  > +	  (shr-width eww-rendering-width)
>
> 	I’d also ask for a separate “do not override” value.  That is:
>
>    (shr-width (if (DO-NOT-SET-P eww-rendering-width)
>                   shr-width
>                 eww-rendering-width))
>
> 	(And, similarly, defcustom’s :type above being a ‘choice’.)
>
>  >  	  (shr-target-id (url-target (url-generic-parse-url url)))
>  >  	  (shr-external-rendering-functions
>  >  	   '((title . eww-tag-title)
>
> […]
>
>  > @@ -543,6 +546,15 @@ appears in a <link> or <a> tag."
>  >    (url-retrieve eww-current-url 'eww-render
>  >  		(list eww-current-url (point))))
>
>  > +(defun eww-set-rendering-width (width)
>  > +  "Set the redering width."
>  > +  (interactive "nSet new redering width (0: window-width) :")
>  > +  (if (zerop width)
>  > +      (setq eww-rendering-width nil)
>  > +    (if (wholenump width)
>  > +	(setq eww-rendering-width width)
>  > +      (message "Set Number to rendering width"))))
>
> 	Shouldn’t this rather be ‘error’?
>
>  > +
>  >  ;; Form support.
>
>  >  (defvar eww-form nil)

bug#16204: eww does not respect shr-width customization

[Ivan Shmakov]

>>>>> Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com> writes:
>>>>> ivan@siamics.net writes:

[…]

 >> I’d also ask for a separate “do not override” value.  That is:

 >>    (shr-width (if (DO-NOT-SET-P eww-rendering-width)
 >>                   shr-width
 >>                 eww-rendering-width))

 > I'm not sure how to work well with this logic.  I think shr-width
 > should be overridden by nil

	Which is what the code already does.  And which is what I find
	inconvenient.

	Alternatively (or perhaps complementarily), I’d be satisfied if
	the rendering width is set as to maintain /constant margin/ at
	the right (as in: if shr-width is negative, it’s /added/ to
	window-width, rather than used as-is.)

 > or new rendering-width every time, since previous windows-width will
 > be set to shr-width next rendering.

 > When users change their window-width, the shr-width should be current
 > window-width, not previous width.

	The whole point of non-nil shr-width is that it stays the same,
	irrespective of any window-width changes whatsoever.

	Which is the behavior I also request from EWW.

[…]

-- 
FSF associate member #7257

bug#16204: eww does not respect shr-width customization, bug#16204: eww does not respect shr-width customization

[Ted Zlatanov]

I think Lars is the best judge of how `shr-width' and eww should
interact, since he wrote both.  FWIW I think the simpler, the better.

Ted

bug#16204: eww does not respect shr-width customization

[Lars Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> 	As currently implemented, eww-display-html simply resets
> 	shr-width to nil, disrespecting any user’s customization
> 	thereof, and providing no easy way to specify the HTML rendering
> 	width to use in EWW buffers.

The problem here is really that `shr-width' defaults to `fill-column',
which is fine and dandy for shr in Gnus, but isn't good in eww.  Gnus
wants a width that makes responding to the HTML nice (i.e.,
`fill-column'), while eww wants to render the HTML for reading (i.e.,
use the full window width).

I think the right solution here is to make Gnus customize `shr-width' to
`fill-column', and eww should leave the variable alone.

However, that sounds like a new feature, and we're in a feature freeze.

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

bug#16204: proposed patch for 24.4

[Ivan Kanis]

Hi Lars,

This bug has been driving me nuts for a while. I have set shr-width many
times and it would just not work. I have then wasted time trying to find
out why it didn't work.

I would like to spare that time to users of 24.4 when it gets released.

I propose the following patch in the branch emacs-24 on bzr:

=== modified file 'lisp/net/eww.el'
--- lisp/net/eww.el     2014-03-05 18:44:58 +0000
+++ lisp/net/eww.el     2014-07-25 09:32:01 +0000
@@ -264,7 +264,6 @@
     (setq eww-current-dom document)
     (let ((inhibit-read-only t)
          (after-change-functions nil)
-         (shr-width nil)
          (shr-target-id (url-target (url-generic-parse-url url)))
          (shr-external-rendering-functions
           '((title . eww-tag-title)

=== modified file 'lisp/net/shr.el'
--- lisp/net/shr.el     2014-04-14 22:01:54 +0000
+++ lisp/net/shr.el     2014-07-25 09:25:15 +0000
@@ -196,8 +196,7 @@
   (let ((start (point))
        (shr-state nil)
        (shr-start nil)
-       (shr-base nil)
-       (shr-width (or shr-width (1- (window-width)))))
+       (shr-base nil))
     (shr-descend (shr-transform-dom dom))
     (shr-remove-trailing-whitespace start (point))))

I have tested with shr-width equal to nil and 70 and it does the
right thing ie. the former wraps at the window width and the latter at
column 70.

I can commit it if everyone is OK with this.

Ivan

-- 
The monitor needs another box of pixels.
    -- BOFH excuse #60

bug#16204: proposed patch for 24.4

[Lars Magne Ingebrigtsen]

Ivan Kanis <ivan@kanis.fr> writes:

> I have tested with shr-width equal to nil and 70 and it does the
> right thing ie. the former wraps at the window width and the latter at
> column 70.
>
> I can commit it if everyone is OK with this.

Yes, looks good to me.  Please apply.

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

bug#16204: proposed patch for 24.4

[Ivan Kanis]

August, 05 at 19:31 Lars Magne Ingebrigtsen wrote:

> Ivan Kanis <ivan@kanis.fr> writes:
>
>> I have tested with shr-width equal to nil and 70 and it does the
>> right thing ie. the former wraps at the window width and the latter at
>> column 70.
>>
>> I can commit it if everyone is OK with this.
>
> Yes, looks good to me.  Please apply.

Things have drastically changed. I can't even apply my patch by hand. I
am compiling the latest emacs-24 and let you know how it behaves.

Ivan
-- 
The recent proliferation of nuclear testing.
    -- BOFH excuse #15

bug#16204: proposed patch for 24.4

[Ivan Kanis]

August, 06 at 22:14 Ivan wrote:

> August, 06 at 19:34 Ivan wrote:
>
>> August, 05 at 19:31 Lars Magne Ingebrigtsen wrote:
>>
>>> Ivan Kanis <ivan@kanis.fr> writes:
>>>
>>>> I can commit it if everyone is OK with this.
>>>
>>> Yes, looks good to me.  Please apply.
>>
>> Things have drastically changed. I can't even apply my patch by hand. I
>> am compiling the latest emacs-24 and let you know how it behaves.

Hi Lars,

I could not apply my patch because it was already applied. Duh.

Since I wrote the patch shr-width is used throughout the code. If it is
set to nil it will fail with comparison < <=.

I think the easiest way to work around this is to create a new variable
shr-internal-width that holds the internal value of the web page without
changing the user's customized shr-width.

I have attached the patch below.

I can think of three other ways to fix it:

1) create a function
2) create a macro
3) use (let ((shr-internal-width everywhere shr-width is used

They all seem overkill.

Let me know what you think.

Ivan

=== modified file 'lisp/net/eww.el'
--- lisp/net/eww.el	2014-06-27 00:48:34 +0000
+++ lisp/net/eww.el	2014-08-06 17:22:14 +0000
@@ -264,7 +264,6 @@
     (setq eww-current-dom document)
     (let ((inhibit-read-only t)
 	  (after-change-functions nil)
-	  (shr-width nil)
 	  (shr-target-id (url-target (url-generic-parse-url url)))
 	  (shr-external-rendering-functions
 	   '((title . eww-tag-title)

=== modified file 'lisp/net/shr.el'
--- lisp/net/shr.el	2014-04-14 22:01:54 +0000
+++ lisp/net/shr.el	2014-08-07 14:10:57 +0000
@@ -130,6 +130,7 @@
 (defvar shr-start nil)
 (defvar shr-indentation 0)
 (defvar shr-inhibit-images nil)
+(defvar shr-internal-width (or shr-width (1- (window-width))))
 (defvar shr-list-mode nil)
 (defvar shr-content-cache nil)
 (defvar shr-kinsoku-shorten nil)
@@ -197,7 +198,7 @@
 	(shr-state nil)
 	(shr-start nil)
 	(shr-base nil)
-	(shr-width (or shr-width (1- (window-width)))))
+	(shr-internal-width (or shr-width (1- (window-width)))))
     (shr-descend (shr-transform-dom dom))
     (shr-remove-trailing-whitespace start (point))))
 
@@ -467,8 +468,8 @@
       (insert elem)
       (setq shr-state nil)
       (let (found)
-	(while (and (> (current-column) shr-width)
-		    (> shr-width 0)
+	(while (and (> (current-column) shr-internal-width)
+		    (> shr-internal-width 0)
 		    (progn
 		      (setq found (shr-find-fill-point))
 		      (not (eolp))))
@@ -482,10 +483,10 @@
 	  (when (> shr-indentation 0)
 	    (shr-indent))
 	  (end-of-line))
-	(if (<= (current-column) shr-width)
+	(if (<= (current-column) shr-internal-width)
 	    (insert " ")
 	  ;; In case we couldn't get a valid break point (because of a
-	  ;; word that's longer than `shr-width'), just break anyway.
+	  ;; word that's longer than `shr-internal-width'), just break anyway.
 	  (insert "\n")
 	  (when (> shr-indentation 0)
 	    (shr-indent)))))
@@ -493,7 +494,7 @@
       (delete-char -1)))))
 
 (defun shr-find-fill-point ()
-  (when (> (move-to-column shr-width) shr-width)
+  (when (> (move-to-column shr-internal-width) shr-internal-width)
     (backward-char 1))
   (let ((bp (point))
 	failed)
@@ -533,7 +534,7 @@
 	   ;; so we look for the second best position.
 	   (while (and (progn
 			 (forward-char 1)
-			 (<= (current-column) shr-width))
+			 (<= (current-column) shr-internal-width))
 		       (progn
 			 (setq bp (point))
 			 (shr-char-kinsoku-eol-p (following-char)))))
@@ -1336,7 +1337,7 @@
 
 (defun shr-tag-hr (_cont)
   (shr-ensure-newline)
-  (insert (make-string shr-width shr-hr-line) "\n"))
+  (insert (make-string shr-internal-width shr-hr-line) "\n"))
 
 (defun shr-tag-title (cont)
   (shr-heading cont 'bold 'underline))
@@ -1629,7 +1630,7 @@
 	(setq style (nconc (list (cons 'color fgcolor)) style)))
       (when style
 	(setq shr-stylesheet (append style shr-stylesheet)))
-      (let ((shr-width width)
+      (let ((shr-internal-width width)
 	    (shr-indentation 0))
 	(shr-descend (cons 'td cont)))
       ;; Delete padding at the bottom of the TDs.
@@ -1701,7 +1702,8 @@
     (dotimes (i (length columns))
       (aset widths i (max (truncate (* (aref columns i)
 				       total-percentage
-				       (- shr-width (1+ (length columns)))))
+				       (- shr-internal-width
+                                          (1+ (length columns)))))
 			  10)))
     widths))


-- 
No race can prosper till it learns that there is as much dignity in
tilling a field as in writing a poem.
    -- Booker T. Washington

bug#16204: proposed patch for 24.4

[Lars Magne Ingebrigtsen]

Ivan Kanis <ivan@kanis.fr> writes:

> I think the easiest way to work around this is to create a new variable
> shr-internal-width that holds the internal value of the web page without
> changing the user's customized shr-width.
>
> I have attached the patch below.

Thanks; applied.

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

bug#16204: proposed patch for 24.4

[Ivan Kanis]

September, 18 at 21:19 Lars Magne Ingebrigtsen wrote:

> Ivan Kanis <ivan@kanis.fr> writes:
>
>> I think the easiest way to work around this is to create a new variable
>> shr-internal-width that holds the internal value of the web page without
>> changing the user's customized shr-width.
>>
>> I have attached the patch below.
>
> Thanks; applied.

It should go in emacs-24, don't you think?
-- 
Mix a little foolishness with your prudence. It's good to be silly at
the right moment.
    -- Horace

bug#16204: proposed patch for 24.4

[Lars Magne Ingebrigtsen]

Ivan Kanis <ivan@kanis.fr> writes:

> September, 18 at 21:19 Lars Magne Ingebrigtsen wrote:
>
>> Ivan Kanis <ivan@kanis.fr> writes:
>>
>>> I think the easiest way to work around this is to create a new variable
>>> shr-internal-width that holds the internal value of the web page without
>>> changing the user's customized shr-width.
>>>
>>> I have attached the patch below.
>>
>> Thanks; applied.
>
> It should go in emacs-24, don't you think?

Uhm, yeah, it probably should.  Does anybody know how to make that
happen?  I haven't checked out an emacs-24...

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

bug#16204: proposed patch for 24.4

[Ivan Kanis]

September, 21 at 14:27 Lars Magne Ingebrigtsen wrote:

> Ivan Kanis <ivan@kanis.fr> writes:
>
>> September, 18 at 21:19 Lars Magne Ingebrigtsen wrote:
>>
>>> Ivan Kanis <ivan@kanis.fr> writes:
>>>
>>>> I think the easiest way to work around this is to create a new variable
>>>> shr-internal-width that holds the internal value of the web page without
>>>> changing the user's customized shr-width.
>>>>
>>>> I have attached the patch below.
>>>
>>> Thanks; applied.
>>
>> It should go in emacs-24, don't you think?
>
> Uhm, yeah, it probably should.  Does anybody know how to make that
> happen?  I haven't checked out an emacs-24...

I have an emacs-24 with the patch ready to go. Can I go ahead and commit
or would it cause conflicts?

Ivan
-- 
Browser's cookie is corrupted. Someone's been nibbling on it.
    -- BOFH excuse #68

bug#16204: proposed patch for 24.4

[Lars Magne Ingebrigtsen]

Ivan Kanis <ivan@kanis.fr> writes:

> I have an emacs-24 with the patch ready to go. Can I go ahead and commit
> or would it cause conflicts?

Oops; I dropped the ball on this one.  Did you apply it to emacs-24?

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

bug#16204: proposed patch for 24.4

[Ivan Kanis]

October, 20 at 0:36 Lars Magne Ingebrigtsen wrote:

> Ivan Kanis <ivan@kanis.fr> writes:
>
>> I have an emacs-24 with the patch ready to go. Can I go ahead and commit
>> or would it cause conflicts?
>
> Oops; I dropped the ball on this one.  Did you apply it to emacs-24?

No and it seems a little too late now...