Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

[Eli Zaretskii]

> +(defun Man-columns ()
> +  (let ((width (cond
> +                ((and (integerp Man-width) (> Man-width 0))
> +                 Man-width)
> +                (Man-width
> +                 (let ((window (get-buffer-window nil t)))
> +                   (frame-width (and window (window-frame window)))))
> +                (t
> +                 (window-width (get-buffer-window nil t))))))

Bother: both frame-width and window-width return values in units of
the canonical character width, which will not change if the default
face is remapped.  And you are using the value to set the COLUMNS
environment variable, so you could get too wide lines, which will not
fit within the window.

P.S. And please do not "optimize" the log messages the way you did in
this commit: it will make the generated ChangeLog entry look wrong.
Please only use the ChangeLog-style text in the header line of the log
entry if it is the entire text; otherwise please come up with some
summary there, and leave the ChangeLog-style text in its original
form, without an empty line in between.  TIA.

Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

[Juri Linkov]

>> +(defun Man-columns ()
>> +  (let ((width (cond
>> +                ((and (integerp Man-width) (> Man-width 0))
>> +                 Man-width)
>> +                (Man-width
>> +                 (let ((window (get-buffer-window nil t)))
>> +                   (frame-width (and window (window-frame window)))))
>> +                (t
>> +                 (window-width (get-buffer-window nil t))))))
>
> Bother: both frame-width and window-width return values in units of
> the canonical character width, which will not change if the default
> face is remapped.  And you are using the value to set the COLUMNS
> environment variable, so you could get too wide lines, which will not
> fit within the window.

This code is not new.  It was moved here from another function.
I don't know how to implement support for variable-pitch fonts
in the Man-mode buffers.  Maybe not to set COLUMNS at all, but
then call fill-paragraph on the output.

ChangeLog styles (was: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385))

[Juri Linkov]

> P.S. And please do not "optimize" the log messages the way you did in
> this commit: it will make the generated ChangeLog entry look wrong.
> Please only use the ChangeLog-style text in the header line of the log
> entry if it is the entire text; otherwise please come up with some
> summary there, and leave the ChangeLog-style text in its original
> form, without an empty line in between.  TIA.

This is not optimization.  The first line of this log message is
a complete sentence.  It provides all information about the change,
answering the questions: WHERE and WHAT.

WHERE: * lisp/man.el (Man-width-max)

WHAT: New defcustom

If even adds the references to bug reports: bug#32536, bug#9385

Many log messages don't provide the information about location of the
change in their subject, so it's difficult to guess what files are
affected by the change.

For example, such log subject provides no clue about the affected package:

  Fix documentation of '-position' server command

It's not clear what is a server command it's talking about.
It takes additional efforts to output the full message to see
that actually it meant the package lisp/server.el.
Such subject would be much more clear:

  * lisp/server.el: Fix documentation of '-position' command

Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: emacs-devel@gnu.org
> Date: Sun, 08 Dec 2019 23:41:28 +0200
> 
> > Bother: both frame-width and window-width return values in units of
> > the canonical character width, which will not change if the default
> > face is remapped.  And you are using the value to set the COLUMNS
> > environment variable, so you could get too wide lines, which will not
> > fit within the window.
> 
> This code is not new.  It was moved here from another function.
> I don't know how to implement support for variable-pitch fonts
> in the Man-mode buffers.  Maybe not to set COLUMNS at all, but
> then call fill-paragraph on the output.

I wasn't thinking about variable-pitch fonts, I was thinking about the
default face being remapped, which can be easily tested.

Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

[martin rudalics]

 > I was thinking about the
 > default face being remapped, which can be easily tested.

For some value of "easily".  Emacs 28 should give 'window-body-height'
a special interpretation of the PIXELWISE argument which will let you
get the body width in terms of the remapped face.  Hopefully.

martin

Re: ChangeLog styles (was: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385))

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: emacs-devel@gnu.org
> Date: Sun, 08 Dec 2019 23:45:09 +0200
> 
> > P.S. And please do not "optimize" the log messages the way you did in
> > this commit: it will make the generated ChangeLog entry look wrong.
> > Please only use the ChangeLog-style text in the header line of the log
> > entry if it is the entire text; otherwise please come up with some
> > summary there, and leave the ChangeLog-style text in its original
> > form, without an empty line in between.  TIA.
> 
> This is not optimization.  The first line of this log message is
> a complete sentence.  It provides all information about the change,
> answering the questions: WHERE and WHAT.
> 
> WHERE: * lisp/man.el (Man-width-max)
> 
> WHAT: New defcustom
> 
> If even adds the references to bug reports: bug#32536, bug#9385

The ChangeLog entry that is produced from Git log of this commit
should look like this:

    * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)
    (Man-columns): New buffer-local variable.
    (Man-columns): New function.
    (Man-start-calling): Call Man-columns and set buffer-local Man-columns.
    (Man--window-state-change-timer): New internal variable.
    (Man--window-state-change): New internal function.
    (Man-fit-to-window): New function.
    (Man-mode): Add Man--window-state-change to local hook
    window-state-change-functions.

This makes it clear that all the functions and variables you mention
are from man.el.

Your format leaves an empty line after the first one, so the
connection to the file will be lost when we generate a ChangeLog from
Git.

This is the only problem I have with the format you used in this
case.  Everything else is very fine.

> Many log messages don't provide the information about location of the
> change in their subject, so it's difficult to guess what files are
> affected by the change.

There's no requirement in CONTRIBUTE to have all of that information
in the heading line.  In many cases it is simply impossible without
losing too much of other useful information that describes the change.
Putting the file name there, let alone the function/variable name and
the bug number, eats up too much of precious estate, leaving too
little for the description.  I'm not saying that mentioning the
location of the change there is "verboten", I'm saying it isn't
required.

> For example, such log subject provides no clue about the affected package:
> 
>   Fix documentation of '-position' server command
> 
> It's not clear what is a server command it's talking about.
> It takes additional efforts to output the full message to see
> that actually it meant the package lisp/server.el.
> Such subject would be much more clear:
> 
>   * lisp/server.el: Fix documentation of '-position' command

Indeed, you have to read the entire log message to know which file(s)
were modified, and that's expected.  It is so even in your case: the
second file that you modified is not mentioned in the header line (and
shouldn't be).

Thanks.

Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 9 Dec 2019 10:20:38 +0100
> 
>  > I was thinking about the
>  > default face being remapped, which can be easily tested.
> 
> For some value of "easily".

??? face-remapping-alist is a simple alist, and there should be no
difficulty in telling whether 'default' appears in it.  Or what am I
missing?

> Emacs 28 should give 'window-body-height' a special interpretation
> of the PIXELWISE argument which will let you get the body width in
> terms of the remapped face.  Hopefully.

Isn't default-font-width provide the information necessary for man.el
to do this calculation more accurately?

Re: master 7e387c9: * lisp/man.el (Man-width-max): New defcustom (bug#32536, bug#9385)

[martin rudalics]

 >>   > I was thinking about the
 >>   > default face being remapped, which can be easily tested.
 >>
 >> For some value of "easily".
 >
 > ??? face-remapping-alist is a simple alist, and there should be no
 > difficulty in telling whether 'default' appears in it.  Or what am I
 > missing?

It will be up to Juri to decide whether it can be done easily.  And it
probably can be done easily if we ignore the filter mechanism you
consider harmful (for the default face) anyway.  Personally, I'm not
convinced yet.

 > Isn't default-font-width provide the information necessary for man.el
 > to do this calculation more accurately?

For a window returned by 'get-buffer-window'?  If you know the
necessary precautions, yes.  I didn't until a couple of days ago.  And
Štěpán apparently didn't know them either.

martin