GNU bug report logs - #71666
30.0.50; [PATCH] Fix zooming images with SHR

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 71666 in the body.
You can then email your comments to 71666 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Wed, 19 Jun 2024 21:47:26 -0700

[Message part 1 (text/plain, inline)]

To reproduce this, start from "emacs -Q -f eww" and then go to fsf.org. 
Move point over one of the larger images on the page, like the ones that 
say "Featured" above them, and press Z ('shr-zoom-image'). The result is 
that the image is duplicated, and the first slice gets pushed up onto 
the previous line.

Attached is a patch to fix this, with regression tests.

[0001-Fix-zooming-images-in-SHR.patch (text/plain, attachment)]

Message #8 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Sat, 22 Jun 2024 12:11:45 +0300

> Date: Wed, 19 Jun 2024 21:47:26 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> To reproduce this, start from "emacs -Q -f eww" and then go to fsf.org. 
> Move point over one of the larger images on the page, like the ones that 
> say "Featured" above them, and press Z ('shr-zoom-image'). The result is 
> that the image is duplicated, and the first slice gets pushed up onto 
> the previous line.
> 
> Attached is a patch to fix this, with regression tests.

I tried the recipe and the patch.  While the patches shr behaves
better than the unpatched, I don't see the image being zoomed, it
stays at its original size.  Do I need some optional library or
external program for the zoom to happen?

Thanks.

Message #11 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Sat, 22 Jun 2024 13:21:53 -0700

On 6/22/2024 2:11 AM, Eli Zaretskii wrote:
> I tried the recipe and the patch.  While the patches shr behaves
> better than the unpatched, I don't see the image being zoomed, it
> stays at its original size.  Do I need some optional library or
> external program for the zoom to happen?

Ah right, that would have been helpful for me to mention in the original 
message: by default, SHR scales down images that are more than 70% of 
the width (or height) of the window. So if you narrow your window to 40 
columns or so and reload the page in EWW, the larger images should be 
scaled down. Then putting point on one and pressing "z" should scale it 
up to the original size. Pressing "z" again should scale the image back 
down.

As an aside, there are actually *three* states for image scaling in SHR: 
default, original, and full, and 'shr-zoom-image' cycles through them in 
that order. As far as I can tell from the code, "default" and "full" are 
the same though. I haven't figured out whether that's a bug (and if so, 
what the bug is), or whether I'm just misunderstanding something. I'll 
try to fix that later (if only by adding documentation), once I know 
what's going on.

Message #14 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Sat, 22 Jun 2024 16:04:51 -0700

[Message part 1 (text/plain, inline)]

Here's an improved patch with a better regression test.

[0001-Fix-zooming-images-in-SHR.patch (text/plain, attachment)]

Message #17 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Sun, 23 Jun 2024 07:44:09 +0300

> Date: Sat, 22 Jun 2024 13:21:53 -0700
> Cc: 71666 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 6/22/2024 2:11 AM, Eli Zaretskii wrote:
> > I tried the recipe and the patch.  While the patches shr behaves
> > better than the unpatched, I don't see the image being zoomed, it
> > stays at its original size.  Do I need some optional library or
> > external program for the zoom to happen?
> 
> Ah right, that would have been helpful for me to mention in the original 
> message: by default, SHR scales down images that are more than 70% of 
> the width (or height) of the window. So if you narrow your window to 40 
> columns or so and reload the page in EWW, the larger images should be 
> scaled down. Then putting point on one and pressing "z" should scale it 
> up to the original size. Pressing "z" again should scale the image back 
> down.

OK, then feel free to install, and thanks.

> As an aside, there are actually *three* states for image scaling in SHR: 
> default, original, and full, and 'shr-zoom-image' cycles through them in 
> that order. As far as I can tell from the code, "default" and "full" are 
> the same though. I haven't figured out whether that's a bug (and if so, 
> what the bug is), or whether I'm just misunderstanding something. I'll 
> try to fix that later (if only by adding documentation), once I know 
> what's going on.

I must say this kind of dwim-ish operation that looks like no-op in
too many "normal" cases looks very strange to me.  How does it make
sense to have features that only appear to do something in rare cases?
Doesn't that cause user bewilderment (like I was surprised above), and
cause users to think we have a bug?  Should we perhaps adjust the
heuristics and its parameters to make this not no-op in more cases?
Or at least show a message in echo area explaining why the image was
not zoomed-in when we decide not to?  For example, in the case above,
why would it not make sense to enlarge the image when the user presses
'z' even if it was not scaled down initially?  Doesn't principle of
least surprise count anymore?

Message #20 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Sat, 22 Jun 2024 23:14:16 -0700

On 6/22/2024 9:44 PM, Eli Zaretskii wrote:
> OK, then feel free to install, and thanks.

Thanks. Merged to the master branch as 5f9b5803bea.

I made one small improvement to the code to use overlays for preventing 
image slices from being underlined. That makes the code a bit cleaner, 
and should make future improvements in this area easier.

> I must say this kind of dwim-ish operation that looks like no-op in
> too many "normal" cases looks very strange to me.  How does it make
> sense to have features that only appear to do something in rare cases?
> Doesn't that cause user bewilderment (like I was surprised above), and
> cause users to think we have a bug?  Should we perhaps adjust the
> heuristics and its parameters to make this not no-op in more cases?
> Or at least show a message in echo area explaining why the image was
> not zoomed-in when we decide not to?  For example, in the case above,
> why would it not make sense to enlarge the image when the user presses
> 'z' even if it was not scaled down initially?  Doesn't principle of
> least surprise count anymore?

I agree completely, and I'm working on a patch to that effect. :)

I think the problem you mention is a combination of some bugs I'm 
working on fixing now, and like you say, 'shr-zoom-image' not providing 
enough feedback about what's happening. It just says it's fetching the 
image (this is also probably a bug; why fetch the image from the web 
when we're just resizing it?).

Since I'm most of the way done with this additional patch, I'll leave 
this bug open for now.

Message #23 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Sun, 23 Jun 2024 15:24:47 -0700

[Message part 1 (text/plain, inline)]

On 6/22/2024 11:14 PM, Jim Porter wrote:
> I agree completely, and I'm working on a patch to that effect. :)
> 
> I think the problem you mention is a combination of some bugs I'm 
> working on fixing now, and like you say, 'shr-zoom-image' not providing 
> enough feedback about what's happening. It just says it's fetching the 
> image (this is also probably a bug; why fetch the image from the web 
> when we're just resizing it?).
> 
> Since I'm most of the way done with this additional patch, I'll leave 
> this bug open for now.

This ended up fairly complex, so I've split the patch into sub-parts to 
(I hope) make the changes easier to follow. There are four distinct, but 
related, improvements here:

1. Previously, SHR sliced images whenever you requested "original" zoom. 
But it would really be useful to slice images based on the size they'll 
be displayed at: a tiny image at "original" zoom doesn't need sliced, 
but a tall image at default zoom would benefit from slicing. So now SHR 
checks the height of the image to determine when to slice (you can also 
turn off slicing entirely, since you don't need it if you use 
'pixel-scroll-precision-mode').

2. When zooming, SHR lost track of the width and height of the image 
specified in the HTML like <img src="..." width="M" height="N">. I fixed 
that, and also cleaned up a bit of the code where we had a list that was 
simultaneously an alist and a plist (I converted it to a plist since 
more code used that form).

3. After much archaeology through old Gnus commits, I think I understand 
what each zoom level does, so I've fixed them. I've also added a new 
zoom level that zooms to the image's default size, ignoring HTML 
attributes. That's how the default and "full" zoom levels worked before 
my fix, so if someone wants the old behavior (I'd probably use it), 
there it is. I also made 'shr-zoom-image' display a message telling 
users the new zoom level.

4. Finally, every time you called 'shr-zoom-image', it would reload the 
image from the web. That shouldn't be necessary since there's a local 
cache. Now we use the cache when possible; easy enough.

[0001-Slice-images-based-on-their-height-in-SHR-not-their-.patch (text/plain, attachment)]

[0002-In-SHR-keep-track-of-image-sizes-as-specified-by-the.patch (text/plain, attachment)]

[0003-Fix-the-different-image-zoom-levels-in-SHR-to-work-a.patch (text/plain, attachment)]

[0004-In-SHR-load-from-URL-cache-if-possible-when-zooming-.patch (text/plain, attachment)]

Message #26 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Fri, 28 Jun 2024 18:41:38 -0700

On 6/23/2024 3:24 PM, Jim Porter wrote:
> This ended up fairly complex, so I've split the patch into sub-parts to 
> (I hope) make the changes easier to follow. There are four distinct, but 
> related, improvements here:

Assuming there are no comments/concerns, I'll merge this to the master 
branch in the next few days.

Message #29 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Fri, 28 Jun 2024 20:08:39 -0700

Jim Porter <jporterbugs <at> gmail.com> writes:

> On 6/23/2024 3:24 PM, Jim Porter wrote:
>> This ended up fairly complex, so I've split the patch into sub-parts to
>> (I hope) make the changes easier to follow. There are four distinct, but
>> related, improvements here:
>
> Assuming there are no comments/concerns, I'll merge this to the master
> branch in the next few days.

This is a regression, right?  So should it not go to emacs-30?

Message #32 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Fri, 28 Jun 2024 20:31:31 -0700

On 6/28/2024 8:08 PM, Stefan Kangas wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> Assuming there are no comments/concerns, I'll merge this to the master
>> branch in the next few days.
> 
> This is a regression, right?  So should it not go to emacs-30?

The initial bug report is fixed on emacs-30, but then based on Eli's 
message (and some further examination of the code on my part), I found 
some more closely-related issues.

One of these issues (#3 - the "full" zoom level didn't zoom to the full 
window height) is a regression, but it regressed over a decade ago as 
far as I can tell, so I'd lean towards keeping the current behavior on 
emacs-30. That way, if I misunderstood what the regression was, we have 
plenty of time in the 31 release cycle to address that. (After 
searching, I now see that this particular regression was filed 
previously as bug#63344.)

The other changes aren't so much regressions as they are long-standing 
bugs or limitations of the image-zooming code.

Of course, if you and Eli prefer that I merge some or all of these 
patches to the release branch, I don't mind. But they seemed a bit risky 
to me.

Message #35 received at 71666 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666 <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Fri, 28 Jun 2024 20:39:24 -0700

Jim Porter <jporterbugs <at> gmail.com> writes:

> On 6/28/2024 8:08 PM, Stefan Kangas wrote:
>
>> This is a regression, right?  So should it not go to emacs-30?
>
> The initial bug report is fixed on emacs-30, but then based on Eli's
> message (and some further examination of the code on my part), I found
> some more closely-related issues.

OK, thanks.  I had missed some necessary context here.  If they're not
regressions from Emacs 29, you're right that they belong on master.

Message #40 received at 71666-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 71666-done <at> debbugs.gnu.org
Subject: Re: bug#71666: 30.0.50; [PATCH] Fix zooming images with SHR
Date: Thu, 4 Jul 2024 12:18:08 -0700

On 6/28/2024 8:39 PM, Stefan Kangas wrote:
> OK, thanks.  I had missed some necessary context here.  If they're not
> regressions from Emacs 29, you're right that they belong on master.

Pushed to the master branch as f91387cce8f, and closing this bug now.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.