GNU bug report logs -
#56102
29.0.50; fit-frame-to-buffer's window-text-pixel-size calculation can be incorrect when only is set to vertically
Previous Next
Reported by: Aaron Jensen <aaronjensen <at> gmail.com>
Date: Mon, 20 Jun 2022 03:04:01 UTC
Severity: normal
Tags: patch
Found in version 29.0.50
Done: Aaron Jensen <aaronjensen <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56102 in the body.
You can then email your comments to 56102 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Mon, 20 Jun 2022 03:04:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Aaron Jensen <aaronjensen <at> gmail.com>
:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org
.
(Mon, 20 Jun 2022 03:04:01 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
To repro, open emacs -Q and resize your frame so that the ";; This
buffer..." text wraps (this repro assumes your monitor has more than
enough space for it to not wrap if the frame was big enough).
Add a few more lines of text and then:
M-: (fit-frame-to-buffer nil nil nil nil nil 'vertically)
You should see that the frame's height is too short and does not contain
all the lines. It contains one fewer line for each wrapped line.
Screenshots:
https://share.cleanshot.com/huexHe
https://share.cleanshot.com/dnhKex
The problem appears to be the lines:
(size
(window-text-pixel-size window from to max-width max-height))
As the max-width will be larger than the current frame (meaning the
height calculation will not take wrapping into account).
One possible fix is to set min/max height/width based on `only' to
(frame-parameter frame 'width) / (frame-parameter frame 'height) but I
do not know if that is the best fix.
If that is done, then it may be possible to remove the rest of the
special handling for `only' that sets width/height to nil and handles that.
In GNU Emacs 29.0.50 (build 1, aarch64-apple-darwin21.5.0, NS appkit-2113.50 Version 12.4 (Build 21F79))
of 2022-05-30 built on aaron-m1.local
Windowing system distributor 'Apple', version 10.3.2113
System Description: macOS 12.4
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Wed, 22 Jun 2022 14:00:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 56102 <at> debbugs.gnu.org (full text, mbox):
> From: Aaron Jensen <aaronjensen <at> gmail.com>
> Date: Sun, 19 Jun 2022 23:03:14 -0400
>
>
> To repro, open emacs -Q and resize your frame so that the ";; This
> buffer..." text wraps (this repro assumes your monitor has more than
> enough space for it to not wrap if the frame was big enough).
>
> Add a few more lines of text and then:
>
> M-: (fit-frame-to-buffer nil nil nil nil nil 'vertically)
>
> You should see that the frame's height is too short and does not contain
> all the lines. It contains one fewer line for each wrapped line.
>
> Screenshots:
>
> https://share.cleanshot.com/huexHe
> https://share.cleanshot.com/dnhKex
>
> The problem appears to be the lines:
>
> (size
> (window-text-pixel-size window from to max-width max-height))
>
> As the max-width will be larger than the current frame (meaning the
> height calculation will not take wrapping into account).
>
> One possible fix is to set min/max height/width based on `only' to
> (frame-parameter frame 'width) / (frame-parameter frame 'height) but I
> do not know if that is the best fix.
>
> If that is done, then it may be possible to remove the rest of the
> special handling for `only' that sets width/height to nil and handles that.
I think you're right, but I'd like to hear if Martin has any comments.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Wed, 22 Jun 2022 14:16:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Wed, Jun 22, 2022 at 9:59 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Aaron Jensen <aaronjensen <at> gmail.com>
> > Date: Sun, 19 Jun 2022 23:03:14 -0400
> >
> >
> > To repro, open emacs -Q and resize your frame so that the ";; This
> > buffer..." text wraps (this repro assumes your monitor has more than
> > enough space for it to not wrap if the frame was big enough).
> >
> > Add a few more lines of text and then:
> >
> > M-: (fit-frame-to-buffer nil nil nil nil nil 'vertically)
> >
> > You should see that the frame's height is too short and does not contain
> > all the lines. It contains one fewer line for each wrapped line.
> >
> > Screenshots:
> >
> > https://share.cleanshot.com/huexHe
> > https://share.cleanshot.com/dnhKex
> >
> > The problem appears to be the lines:
> >
> > (size
> > (window-text-pixel-size window from to max-width max-height))
> >
> > As the max-width will be larger than the current frame (meaning the
> > height calculation will not take wrapping into account).
> >
> > One possible fix is to set min/max height/width based on `only' to
> > (frame-parameter frame 'width) / (frame-parameter frame 'height) but I
> > do not know if that is the best fix.
> >
> > If that is done, then it may be possible to remove the rest of the
> > special handling for `only' that sets width/height to nil and handles that.
>
> I think you're right, but I'd like to hear if Martin has any comments.
Sounds good. The most minimal change I can think of is to use the
current frame's width as max-width in the window-text-pixel-size call
when only is set to vertically. I don't know of any reason we need to
constrain max-height in that call because it doesn't have the same
impact. That can be done either explicitly in that call or by changing
max-width to be set to the current width as I described before. That
would be asymmetrical with max-height though, which would be rather
confusing IMO.
Aaron
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Thu, 23 Jun 2022 07:31:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 56102 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>>> The problem appears to be the lines:
>>>
>>> (size
>>> (window-text-pixel-size window from to max-width max-height))
>>>
>>> As the max-width will be larger than the current frame (meaning the
>>> height calculation will not take wrapping into account).
I think MAX-WIDTH should be nil here.
>>> One possible fix is to set min/max height/width based on `only' to
>>> (frame-parameter frame 'width) / (frame-parameter frame 'height) but I
>>> do not know if that is the best fix.
This would not work. The 'height' frame parameter counts in characters
while 'window-text-pixel-size' wants pixels as X-LIMIT. Also, the sizes
of frame and window decorations would hardly match.
>>> If that is done, then it may be possible to remove the rest of the
>>> special handling for `only' that sets width/height to nil and handles that.
>>
>> I think you're right, but I'd like to hear if Martin has any comments.
>
> Sounds good. The most minimal change I can think of is to use the
> current frame's width as max-width in the window-text-pixel-size call
> when only is set to vertically. I don't know of any reason we need to
> constrain max-height in that call because it doesn't have the same
> impact. That can be done either explicitly in that call or by changing
> max-width to be set to the current width as I described before. That
> would be asymmetrical with max-height though, which would be rather
> confusing IMO.
Please try the attached diff.
Thanks, martin
[fit-frame-to-buffer.diff (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Fri, 24 Jun 2022 02:30:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Thu, Jun 23, 2022 at 3:30 AM martin rudalics <rudalics <at> gmx.at> wrote:
>
> >>> The problem appears to be the lines:
> >>>
> >>> (size
> >>> (window-text-pixel-size window from to max-width max-height))
> >>>
> >>> As the max-width will be larger than the current frame (meaning the
> >>> height calculation will not take wrapping into account).
>
> I think MAX-WIDTH should be nil here.
Ah, I had no idea X-LIMIT could be nil, that's great.
> Please try the attached diff.
That seems to work for me. I do wonder though if the the check for
`only' should be first (i.e. if only is vertically, max-width is nil).
Is there a reason that we should not ignore a specified max-width when
only is set to vertically? I ask because in the package that I had
this issue with I employed a work-around where I set the max-width to
(frame-parameter frame 'width), which seems to work well enough, but
probably not as good as your fix. We may not be able to remove that
workaround for some time, so ignoring max-width if set would probably
work better in our specific case.
Thanks,
Aaron
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Fri, 24 Jun 2022 09:21:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 56102 <at> debbugs.gnu.org (full text, mbox):
> That seems to work for me. I do wonder though if the the check for
> `only' should be first (i.e. if only is vertically, max-width is nil).
> Is there a reason that we should not ignore a specified max-width when
> only is set to vertically?
The idea of 'fit-frame-to-buffer' was that an application should be able
to call it (maybe implicitly via 'temp-buffer-resize-mode') and not care
about where on the display the frame will be shown. Hence, a major
concern of its design was to constrain the frame to some specified area
on the display, to avoid that parts of it move off screen. That's why
'fit-frame-to-buffer-margins' and 'fit-frame-to-buffer-sizes' have been
provided.
Currently, we check for an explicit MAX-WIDTH first and then consult
'fit-frame-to-buffer-sizes' via SIZES as
((numberp max-width) (* max-width char-width))
((numberp (nth 2 sizes)) (* (nth 2 sizes) char-width))
If we were to override MAX-WIDTH by setting ONLY to 'vertically', where
and how would we check SIZES?
> I ask because in the package that I had
> this issue with I employed a work-around where I set the max-width to
> (frame-parameter frame 'width), which seems to work well enough, but
> probably not as good as your fix. We may not be able to remove that
> workaround for some time, so ignoring max-width if set would probably
> work better in our specific case.
Are these issues really related? If your workaround works, it should
continue working regardless of whether we ignore MAX-WIDTH when ONLY is
'vertically' or not. Or am I missing something?
martin
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Fri, 24 Jun 2022 14:29:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Fri, Jun 24, 2022 at 5:20 AM martin rudalics <rudalics <at> gmx.at> wrote:
>
> > That seems to work for me. I do wonder though if the the check for
> > `only' should be first (i.e. if only is vertically, max-width is nil).
> > Is there a reason that we should not ignore a specified max-width when
> > only is set to vertically?
>
> The idea of 'fit-frame-to-buffer' was that an application should be able
> to call it (maybe implicitly via 'temp-buffer-resize-mode') and not care
> about where on the display the frame will be shown. Hence, a major
> concern of its design was to constrain the frame to some specified area
> on the display, to avoid that parts of it move off screen. That's why
> 'fit-frame-to-buffer-margins' and 'fit-frame-to-buffer-sizes' have been
> provided.
>
> Currently, we check for an explicit MAX-WIDTH first and then consult
> 'fit-frame-to-buffer-sizes' via SIZES as
>
> ((numberp max-width) (* max-width char-width))
> ((numberp (nth 2 sizes)) (* (nth 2 sizes) char-width))
>
> If we were to override MAX-WIDTH by setting ONLY to 'vertically', where
> and how would we check SIZES?
I'm definitely not familiar enough with the workings and the intended
use cases. My only thought about it was one of principle of least
surprise. If I say that I only want it to scale vertically, I would
expect no width changes to occur, which means I would expect that
max-width and anything else width related would not be relevant.
There's likely something I'm missing here, so please feel free to
disregard if this is necessary for some reason.
> > I ask because in the package that I had
> > this issue with I employed a work-around where I set the max-width to
> > (frame-parameter frame 'width), which seems to work well enough, but
> > probably not as good as your fix. We may not be able to remove that
> > workaround for some time, so ignoring max-width if set would probably
> > work better in our specific case.
>
> Are these issues really related? If your workaround works, it should
> continue working regardless of whether we ignore MAX-WIDTH when ONLY is
> 'vertically' or not. Or am I missing something?
My concern was this:
> Also, the sizes of frame and window decorations would hardly match.
Effectively, that there would be some subtle difference between using
(frame-parameter frame 'width) as MAX-WIDTH and passing nil to
window-text-pixel-size window.
If there's no difference to be concerned about then my workaround
should be fine with the code as it was in the patch.
Thanks,
Aaron
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Sun, 26 Jun 2022 10:10:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 56102 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> If I say that I only want it to scale vertically, I would
> expect no width changes to occur, which means I would expect that
> max-width and anything else width related would not be relevant.
Sounds reasonable.
> My concern was this:
>
>> Also, the sizes of frame and window decorations would hardly match.
>
> Effectively, that there would be some subtle difference between using
> (frame-parameter frame 'width) as MAX-WIDTH and passing nil to
> window-text-pixel-size window.
I was wrong earlier. The doc-string of 'fit-frame-to-buffer' says that
MAX-HEIGHT, MIN-HEIGHT, MAX-WIDTH and MIN-WIDTH specify bounds on
the new total size of FRAME’s root window.
But actually it is the _body_ size of FRAME's root window the current
code constrains. Incidentally, the default body width of a live root
window equals the value of FRAME's width parameter (the native width of
the frame minus the widths of the internal border, the scroll bar and
the fringes) which makes your fix work. With a non-standard setup, say
after doing
(set-window-margins nil 10 10)
in the root window, your fix will fail with the scenario you provided
earlier.
We could try the following: If the new diff I attach now works for you,
I'll push it to master. If after some period of grace (whose length you
determine) I manage to come up with a reasonable fix, I'll push that to
master too and you and your clients will have to adapt. WDYT?
martin
[fit-frame-to-buffer.diff (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Sun, 26 Jun 2022 13:13:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Sun, Jun 26, 2022 at 6:09 AM martin rudalics <rudalics <at> gmx.at> wrote:
> But actually it is the _body_ size of FRAME's root window the current
> code constrains. Incidentally, the default body width of a live root
> window equals the value of FRAME's width parameter (the native width of
> the frame minus the widths of the internal border, the scroll bar and
> the fringes) which makes your fix work. With a non-standard setup, say
> after doing
>
> (set-window-margins nil 10 10)
>
> in the root window, your fix will fail with the scenario you provided
> earlier.
Ah, I can confirm this. Is there a reasonable way for me to calculate
a max-width that would be based on the root window that would work?
There's other math that happens within fit-frame-to-buffer I don't
fully have my head wrapped around yet. I'm not super worried about
this personally as I don't set window margins on this window.
> We could try the following: If the new diff I attach now works for you,
> I'll push it to master. If after some period of grace (whose length you
> determine) I manage to come up with a reasonable fix, I'll push that to
> master too and you and your clients will have to adapt. WDYT?
The patch works for me and seems good. When you say if you come up
with a reasonable fix, could I ask what is unreasonable about the
patch you attached?
Regardless, if you do end up updating the fix to respect a supplied
max-width even if only vertically is supplied, I could always make an
Emacs version based decision on whether or not to pass the work-around
max-width in, and Emacs 29 is as good a version as any to stop passing
it in (people already on master will have to recompile, but that's
fine imo). So, I would be fine with the new patch going straight to
master, though as I mentioned I don't know what it would do
differently or why.
Thanks,
Aaron
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Mon, 27 Jun 2022 08:25:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 56102 <at> debbugs.gnu.org (full text, mbox):
>> With a non-standard setup, say
>> after doing
>>
>> (set-window-margins nil 10 10)
>>
>> in the root window, your fix will fail with the scenario you provided
>> earlier.
>
> Ah, I can confirm this. Is there a reasonable way for me to calculate
> a max-width that would be based on the root window that would work?
Here
(set-window-margins nil 10 10)
(fit-frame-to-buffer nil nil nil (window-body-width) nil 'vertically)
seems to work.
> There's other math that happens within fit-frame-to-buffer I don't
> fully have my head wrapped around yet. I'm not super worried about
> this personally as I don't set window margins on this window.
Similar problems should happen when the fringe or scroll bar sizes of
the root window or its buffer differ from that of the frame.
> The patch works for me and seems good. When you say if you come up
> with a reasonable fix, could I ask what is unreasonable about the
> patch you attached?
The current code interprets all its -HEIGHT and -WIDTH arguments as if
they were body sizes and not total sizes. This at least contradicts the
doc-strings of 'fit-frame-to-buffer-sizes' and of 'fit-frame-to-buffer'
itself. If I can fix the problem by rewriting the doc-strings to match
what the code does, I'll probably do that. In general, you can't get
these right anyway when you allow specifications in terms of character
sizes and windows/frames may have non-integral sizes wrt these.
> Regardless, if you do end up updating the fix to respect a supplied
> max-width even if only vertically is supplied, I could always make an
> Emacs version based decision on whether or not to pass the work-around
> max-width in, and Emacs 29 is as good a version as any to stop passing
> it in (people already on master will have to recompile, but that's
> fine imo). So, I would be fine with the new patch going straight to
> master, though as I mentioned I don't know what it would do
> differently or why.
I don't know yet either. I'll send you a patch as soon as I have one.
martin
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Mon, 27 Jun 2022 13:25:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Mon, Jun 27, 2022 at 4:24 AM martin rudalics <rudalics <at> gmx.at> wrote:
>
> >> With a non-standard setup, say
> >> after doing
> >>
> >> (set-window-margins nil 10 10)
> >>
> >> in the root window, your fix will fail with the scenario you provided
> >> earlier.
> >
> > Ah, I can confirm this. Is there a reasonable way for me to calculate
> > a max-width that would be based on the root window that would work?
>
> Here
>
> (set-window-margins nil 10 10)
> (fit-frame-to-buffer nil nil nil (window-body-width) nil 'vertically)
>
> seems to work.
Thanks, I'll update the workaround to use this instead.
> I don't know yet either. I'll send you a patch as soon as I have one.
Sounds good, thanks.
Aaron
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Tue, 28 Jun 2022 09:30:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 56102 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>> I don't know yet either. I'll send you a patch as soon as I have one.
>
> Sounds good, thanks.
I suppose we can manage by fixing some trivial silliness in the code and
making the documentation tell what the code does. If you don't see any
problems with the attached diff within a week or so, I'll push it.
Thanks, martin
[fit-frame-to-buffer.diff (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Tue, 28 Jun 2022 14:54:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Tue, Jun 28, 2022 at 5:29 AM martin rudalics <rudalics <at> gmx.at> wrote:
>
> >> I don't know yet either. I'll send you a patch as soon as I have one.
> >
> > Sounds good, thanks.
>
> I suppose we can manage by fixing some trivial silliness in the code and
> making the documentation tell what the code does. If you don't see any
> problems with the attached diff within a week or so, I'll push it.
Great, works on first try. I'll run with it and report back if I have
any issues. Thank you.
Aaron
Added tag(s) patch.
Request was from
Stefan Kangas <stefan <at> marxist.se>
to
control <at> debbugs.gnu.org
.
(Tue, 28 Jun 2022 21:21:03 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Tue, 05 Jul 2022 13:08:02 GMT)
Full text and
rfc822 format available.
Message #46 received at 56102 <at> debbugs.gnu.org (full text, mbox):
On Tue, Jun 28, 2022 at 10:52 AM Aaron Jensen <aaronjensen <at> gmail.com> wrote:
>
> On Tue, Jun 28, 2022 at 5:29 AM martin rudalics <rudalics <at> gmx.at> wrote:
> >
> > >> I don't know yet either. I'll send you a patch as soon as I have one.
> > >
> > > Sounds good, thanks.
> >
> > I suppose we can manage by fixing some trivial silliness in the code and
> > making the documentation tell what the code does. If you don't see any
> > problems with the attached diff within a week or so, I'll push it.
>
> Great, works on first try. I'll run with it and report back if I have
> any issues. Thank you.
No issues to report. Has worked fine for me for the last week.
Thanks,
Aaron
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#56102
; Package
emacs
.
(Wed, 06 Jul 2022 07:38:01 GMT)
Full text and
rfc822 format available.
Message #49 received at 56102 <at> debbugs.gnu.org (full text, mbox):
> No issues to report. Has worked fine for me for the last week.
Thanks. I've pushed these changes to master now. If there are any
problems, please complain. Otherwise, consider marking this bug as
done.
Thanks, martin
Reply sent
to
Aaron Jensen <aaronjensen <at> gmail.com>
:
You have taken responsibility.
(Wed, 06 Jul 2022 13:18:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Aaron Jensen <aaronjensen <at> gmail.com>
:
bug acknowledged by developer.
(Wed, 06 Jul 2022 13:18:02 GMT)
Full text and
rfc822 format available.
Message #54 received at 56102-done <at> debbugs.gnu.org (full text, mbox):
On Wed, Jul 6, 2022 at 3:37 AM martin rudalics <rudalics <at> gmx.at> wrote:
>
> > No issues to report. Has worked fine for me for the last week.
>
> Thanks. I've pushed these changes to master now. If there are any
> problems, please complain. Otherwise, consider marking this bug as
> done.
Great, thank you. Closing.
Aaron
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Thu, 04 Aug 2022 11:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 343 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.