corfu-popupinfo: Ensure position x has a minimum value of 1

[galeo]

The minimum value of x should be 1, not negative.

[minad]

Shouldn't the minimum be 0, not 1?

[galeo]

Shouldn't the minimum be 0, not 1?

Isn't the minimum x value of the corfu popup 1?

[minad]

Indeed, but I think this was an oversight or a result of some refactoring which happened a long time ago. Please check.

[galeo]

Indeed, but I think this was an oversight or a result of some refactoring which happened a long time ago. Please check.

If borders are allowed to overlap, that's fine.

https://github.com/minad/corfu/blob/5df80e26b1d365fa0bb9250186d8d1ec5ab46cdf/extensions/corfu-popupinfo.el#L263-L264

The 1 in these two lines of code should also be removed.

[minad]

What do you mean by by overlapping borders? Before removing the 1 in corfu.el the popup didn't touch the parent frame edge, there was a thin 1 pixel distance.

[galeo]

What do you mean by by overlapping borders?

The border of the popup is in line with the border of the parent frame.

[minad]

The border of the popup is in line with the border of the parent frame.

Yes, that's okay I guess? Before we also didn't take the parent frame border width into account.

[galeo]

@minad The maintainer gave a way to create a child frame with a height smaller than the default line height by specifying the font attribute of the frame. See bug 59547#17.

We need to add the font attribute to corfu--frame-parameters. You can test it with the code

`(font . ,(font-spec :size (* (font-get (face-attribute 'default :font) :size) 0.8)))

[minad]

@galeo This doesn't sound like an appropriate way to fix the issue. Why is the font the problem here? Also, the issue is that the child frame height is measured in multiples of the default face height, which is just nonsensical.

[galeo]

minad wrote:

Why is the font the problem here?

martin rudalics gave the following explanation:

The immediate cause of the problem are these two lines in window.c's resize_frame_windows code:

new_pixel_size = max (horflag ? size : size - mini_height, unit); new_size = new_pixel_size / unit;

where unit is defined as

int unit = horflag ? FRAME_COLUMN_WIDTH (f) : FRAME_LINE_HEIGHT (f);

As a consequence, 'compute_line_metrics' in xdisp.c will calculate

  max_y = WINDOW_BOX_HEIGHT_NO_MODE_LINE (it->w);

  if (row->y + row->height > max_y)

row->visible_height -= row->y + row->height - max_y;

from the value of 'unit' above, the text is not clipped and whether you see the lower border or the lower part of the buffer text reduces to the question of who succeeds in drawing later into that area - the code displaying the buffer text or the one filling the border rectangle.

If you replace these two lines with say

new_pixel_size = max (horflag ? size : size - mini_height, 1); new_size = max (new_pixel_size / unit, unit);

you should get the bottom border immediately.

I think the calculation of the frame line height should be affected by the frame font.

minad wrote:

Also, the issue is that the child frame height is measured in multiples of the default face height, which is just nonsensical.

With the default font width and line height, characters may be cut off due to a different font size of the popup. I'm wondering if we should calculate the popup size based on the font size in it. I'm also wondering if it's possible to use the font-size of the different parts for calculations and use :align-to to solve #149.

Anyway, I think the current way is ok. Thanks.

[minad]

I must admit I don't understand what Martin Rudalics is explaining there, but there is also no point in trying to. He talks about internals. If we request a certain reasonable frame size based on the face height of our popup height, but get a different one, it is a bug.

With the default font width and line height, characters may be cut off due to a different font size of the popup. I'm wondering if we should calculate the popup size based on the font size in it.

We already do for the info popup. corfu-popupinfo--size takes the face of the info popup into account. For the candidate popup we don't do that yet, but I have a patch around somewhere. I also didn't add this because of the minimum height issue we are talking about here.