emacs-helm / helm

Emacs incremental completion and selection narrowing framework
https://emacs-helm.github.io/helm/
GNU General Public License v3.0
3.37k stars 390 forks source link

helm-echo-input-in-header-line UI bug. #2638

Closed karbiv closed 7 months ago

karbiv commented 8 months ago

What happened?

helm-echo-input-in-header-line when set to t clips bottom line. Changing font size doesn't effect it(doesn't fix).

I could try to fix it but just unsetting helm-echo-input-in-header-line seems ok for now, usual minibuffer at the bottom.

Screenshot from 2024-03-01

How to reproduce?

Setting helm-echo-input-in-header-line to t and opening helm window.

Emacs 29.2

Helm Version

Melpa or NonGnuElpa

Emacs Version

Emacs-29.1

OS

GNU/Linux

Relevant backtrace (if possible)

No response

Minimal configuration

karbiv commented 8 months ago

sh ./emacs-helm.sh

emacs-helm-sh

thierryvolpiatto commented 8 months ago

Alexander Karbivnichiy @.***> writes:

  1. ( ) text/plain (*) text/html

What happened?

helm-echo-input-in-header-line when set to t clips bottom line. Changing font size doesn't effect it.

Not sure what's wrong here, do you mean the header-line contents displayed in minibuffer "C-j:[...] (keeping session)" ?

I see nothing wrong here either with my main Emacs and emacs-helm.sh (in terminal and on graphical display).

I could try to fix it but just unsetting helm-echo-input-in-header-line seems ok for now, usual minibuffer at the bottom.

You can exchange minibuffer and header line with "C-c %".

Screenshot.from.2024-03-01.png (view on web)

How to reproduce?

Setting helm-echo-input-in-header-line to t and opening helm window.

Emacs 29.2

Helm Version

Melpa or NonGnuElpa

Emacs Version

Emacs-29.1

OS

GNU/Linux

Relevant backtrace (if possible)

No response

Minimal configuration

• [*] I agree using a minimal configuration

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.*Message ID: @.***>

-- Thierry

karbiv commented 8 months ago

do you mean the header-line contents displayed in minibuffer "C-j:[...] (keeping session)" ?

Yes, that original minibuffer line is clipped, letters are not shown in full. That window is resizable by mouse actually. I'm looking for the code in Helm that creates that UI, maybe there's a setting to make for it.

So the culprit is resize-mini-windows customization var for minibuffer. When it's "off" this bug disappears. My setting was "fit", I reset it to Emacs' default value which is "grow-only". With "grow-only" this bug is not present only at the very first opening of Helm, then it appears in all subsequent Helm windows.

With "off" setting for resize-mini-windows the bug disappeared completely. So setting resize-mini-windows to "off" fixes it. But default value in Emacs is "grow-only".

... resize-mini-windows var is used in helm-read-from-minibuffer function: https://github.com/emacs-helm/helm/blob/17c7bd015280b472eda80b90908233a97ba63fca/helm-core.el#L3869-L3889

karbiv commented 8 months ago

In helm-read-from-minibuffer this code is the only source of the bug, it can be removed:

(resize-mini-windows (and (null helm-echo-input-in-header-line)
                           resize-mini-windows))

Tested function without this lines, recompiling bytecode, everything works correctly.

thierryvolpiatto commented 8 months ago

Alexander Karbivnichiy @.***> writes:

  1. ( ) text/plain (*) text/html

    do you mean the header-line contents displayed in minibuffer "C-j:[...] (keeping session)" ?

Yes, that original minibuffer line is clipped, letters are not shown in full.

Ok thanks.

That window is resizable by mouse actually. I'm looking for the code in Helm that creates that UI, maybe there's a setting to make for it.

AFAIR no, there is nothing for this.

So the culprit is resize-mini-windows customization var for minibuffer. When it's "off" or "grow-only" this bug disappears. My setting was "fit", I reset it to Emacs' default value which is "grow-only". With "grow-only" this is not present only at the very first opening of Helm, then it appears in all subsequent Helm windows.

With "off" setting for resize-mini-windows the bug disappeared completely. So setting resize-mini-windows to "off" fixes it. But default value in Emacs is "grow-only".

So IIUC we could let bind it to nil when helm-echo-input-in-header-line is non nil and to t when using a normal minibuffer i.e. helm-echo-input-in-header-line == nil, it that correct?

-- Thierry

thierryvolpiatto commented 8 months ago

Alexander Karbivnichiy @.***> writes:

  1. ( ) text/plain (*) text/html

In helm-read-from-minibuffer this code is the only source of the bug, it can be removed:

(resize-mini-windows (and (null helm-echo-input-in-header-line) resize-mini-windows))

Tested function without it, recompiling bytecode, everything works correctly.

Hmm, what I don't understand is that in your previous mail you said that with the default "grow-only" it is working fine at first call and after the line is clipped; So if we remove the let-binding Emacs will use default setting, right?

Anyway A PR would be welcome as I can't reproduce the bug here (LinuxMint).

Thanks.

-- Thierry

karbiv commented 8 months ago

Anyway A PR would be welcome as I can't reproduce the bug here (LinuxMint).

I explored more how to reproduce it in emacs-helm.sh; The bug doesn't show up if "C-c %" binding is used.

1) sh emacs-helm.sh 2) Enable helm-echo-input-in-header-line in Customize buffer. 3) M-x

And the bug is visible, as on screenshots above. If it's still not reproducible on other machine, then I'll think of something else, no point to do PR that would hang indefinitely.

So IIUC we could let bind it to nil when helm-echo-input-in-header-line is non nil and to t when using a normal minibuffer i.e. helm-echo-input-in-header-line == nil, it that correct?

For now I would answer that while testing I couldn't find any conflicts between helm-echo-input-in-header-line and resize-mini-windows that would require that let binding. It just works without that binding. But first the bug should be reproduced somehow, if lucky.

thierryvolpiatto commented 8 months ago

I still can't reproduce from emacs-helm.sh.

Capture d’écran_2024-03-01_22-24-16

thierryvolpiatto commented 8 months ago

Previous screenshot is on Emacs-30, just tried now from Emacs-29 and it is the same:

Capture d’écran_2024-03-01_22-33-40

karbiv commented 8 months ago

After stepping in Edebug I detected the exact lines of code that cause height clipping. It's helm-hide-minibuffer-maybe function in helm-core.el; This snippet puts a string into overlay and causes the bug:

(overlay-put ov 'display
             (truncate-string-to-width
               (substitute-command-keys
                 (concat "\\<helm-map>\\[helm-execute-persistent-action]: "
                         (format "%s (keeping session)" it)))
               (- (window-width) 1)))

Not clear why on other machines font rendering isn't prone to this bug. I guess I'll install Linux Mint to test it. I use Manjaro Gnome(Arch).

thierryvolpiatto commented 8 months ago

Alexander Karbivnichiy @.***> writes:

  1. ( ) text/plain (*) text/html

After stepping in Edebug I detected the exact lines of code that do that height clipping. It's helm-hide-minibuffer-maybe function in helm-core.el; This snippet puts a string into overlay and causes the bug:

(overlay-put ov 'display (truncate-string-to-width (substitute-command-keys (concat "\\[helm-execute-persistent-action]: " (format "%s (keeping session)" it))) (- (window-width) 1)))

Not clear why on other machines font rendering isn't prone to this bug.

Also why when using "C-c %" the header-line is represented correctly in minibuffer?

I guess I gonna install Linux Mint to test it. I use Manjaro Gnome(Arch).

If you examine my screenshot and yours, you will see the fonts are not the same (emacs-helm.sh uses the Emacs default font).

Why on your system emacs -Q uses a different font than the default Emacs one?

Are you compiling your Emacs yourself or are you installing the version provided by Manjaro (if so perhaps Manjaro setup Emacs in a particular way even when using -Q) ?

Also note that if you called emacs-helm.sh with -q option, emacs uses the .Xdefaults or .Xresources settings...

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: @.***>

-- Thierry

karbiv commented 8 months ago

Also why when using "C-c %" the header-line is represented correctly in minibuffer?

Only overlay is affected when it's in the bottom. Emacs can't detect face height or something in latest Gnome Shell versions. This article of December 2023 on www .phoronix. com/news/GNOME-Shell-Better-Text-Scaling :

updated fontsize function to convert fonts in pt to em; replace/update instances of labels not using fontsize()

This might be the reason. emacs -nw, terminal UI has no bug of course. Manjaro uses latest versions of packages hence this bug is visible. There's no need to compile Emacs because latest version 29.2 is available in packages.

substitute-command-keys call in helm-hide-minibuffer-maybe function returns propertized string with "help-key-binding" face that breaks somehow the overlay. If string is without properties - the bug disappears.

So some unknown state influences rendering of this face on different machines. I'll try to find that.

thierryvolpiatto commented 8 months ago

Alexander Karbivnichiy @.***> writes:

  1. ( ) text/plain (*) text/html

Manjaro uses latest versions of packages hence this bug is visible. There's no need to compile Emacs because latest version 29.2 is available in packages.

Yes but even if it is the same version 29.2, a distro may do some extra configuration for their users that may affect even Emacs -Q, debian does so.

-- Thierry

karbiv commented 8 months ago

Yes but even if it is the same version 29.2, a distro may do some extra configuration for their users that may affect even Emacs -Q, debian does so.

Just a default Emacs build, "./configure". I tested builds with different options and only disabling font renderers fixes the bug:

./configure --without-xft --without-cairo

Even with Lucid widgets build the bug is present if font renderers are not disabled(--without-xft --without-cairo). I think it will appear in other distros when they will upgrade packages.

karbiv commented 8 months ago

I'm going to prepare a PR, even though it's just removing one LET binding for resize-mini-windows customization var in helm-read-from-minibuffer function. It does not break anything, nothing changes, only fixes the bug. The main part of it is demonstrating the bug if it can't be easily replicated on some machines with older system packages. It could be even a link to a screen capture video.

thierryvolpiatto commented 8 months ago

I can't remember why we used this i.e. resize-mini-windows, @michael-heerdegen may be you remember why?

karbiv commented 8 months ago

I can't remember why we used this

commit https://github.com/emacs-helm/helm/commit/f3b26da849e0bcf96f453f1a49fc800cc16f1b33 Author: Thierry Volpiatto _@_ Date: Sat Jun 27 10:17:34 2015 +0200

On hovering mouse over commit number above github shows popup with commit's info. "(helm-internal): Remove resize-mini-windows binding."

Now there's only one last such binding of resize-mini-windows in Helm. In helm-read-from-minibuffer which causes this bug.

michael-heerdegen commented 8 months ago

Thierry Volpiatto @.***> writes:

I can't remember why we used this i.e. resize-mini-windows, @michael-heerdegen may be you remember why?

The problem when removing this binding is: when you insert a multi-line string at the prompt with helm-echo-input-in-header-line enabled, the mini-window will vertically grow because of the invisible multi-line text in the minibuffer. Helm makes the text invisible to fake the prompt in the header line, but it is still there and causes the mini window to grow. You can just try yourself, remove the binding and yank a multiline string at the header-line prompt.

michael-heerdegen commented 8 months ago

BTW, this doesn't look like something you can fix in Helm. It's a display thing, so you might better ask Eli.

I sometimes experience similar issues, independently from Helm. Turning on and off a header-line is a risk, Emacs sometimes doesn't get that right.

More important: A problem might be the image I see in the mode-line of the OP: if there is an image in the mode-line or mini-window, Emacs geometry calculations might not take that into account as expected: The number of pixels that an image is higher than the current font will sometimes be cut off in the mini-window.

Maybe @karbiv could check whether removing the Emacs logo from the mode-line prevents the issue. If this is the case this would really be a bug report for Gnu Emacs, not Helm.

thierryvolpiatto commented 8 months ago

Michael Heerdegen @.***> writes:

  1. ( ) text/plain (*) text/html

Thierry Volpiatto @.***> writes:

I can't remember why we used this i.e. resize-mini-windows, @michael-heerdegen may be you remember why?

The problem when removing this binding is: when you insert a multi-line string at the prompt with helm-echo-input-in-header-line enabled, the mini-window will vertically grow because of the invisible multi-line text in the minibuffer. Helm makes the text invisible to fake the prompt in the header line, but it is still there and causes the mini window to grow. You can just try yourself, remove the binding and yank a multiline string at the header-line prompt.

Thanks Michael for explanations.

For this same reason I recall now I had disabled helm-echo-input-in-header-line in helm-eval, unfortunately later I have added helm-exchange-minibuffer-and-header-line without remembering about helm-eval, I have to fix this now :-(

-- Thierry

thierryvolpiatto commented 8 months ago

Michael Heerdegen @.***> writes:

  1. ( ) text/plain (*) text/html

BTW, this doesn't look like something you can fix in Helm. It's a display thing, so you might better ask Eli.

I sometimes experience similar issues, independently from Helm. Turning on and off a header-line is a risk, Emacs sometimes doesn't get that right.

More important: A problem might be the image I see in the mode-line of the OP: if there is an image in the mode-line or mini-window, Emacs geometry calculations might not take that into account as expected: The number of pixels that an image is higher than the current font will sometimes be cut off in the mini-window.

It seems it appears also with emacs-helm.sh, so from emacs -Q without logo, though this is not clear according to the bugreport. I suspect some Emacs default settings done by Manjaro somewhere that affect Emacs even with -Q. When looking at the screenshot of emacs-helm.sh, it seems the font is different than the one Emacs uses by default.

@karbiv did you try to build emacs yourself?

Maybe @karbiv could check whether removing the Emacs logo from the mode-line prevents the issue. If this is the case this would really be a bug report for Gnu Emacs, not Helm.

So yes perhaps the best would be an Emacs bug report.

Thanks.

-- Thierry

karbiv commented 8 months ago

You can just try yourself, remove the binding and yank a multiline string at the header-line prompt.

Nothing breaks, Emacs replaces newlines with "^J" characters. I'm using dev setup for Helm in Emacs init that is forked and pull request with it created, after a thorough debugging.

BTW, this doesn't look like something you can fix in Helm.

I tested it with emacs-helm.sh, the bug is present. Tried all possibilities with rewriting overlay code in helm-hide-minibuffer-maybe. Installed Linux Mint to see why it's not reproduced there and explore what's there may be different besides old packages in repositories. Compiled default Emacs, recompiled with different options and found that the bug is caused only by font rendering that changes line height(as it should), but Helm disables automatic minibuffer adjustment by masking resize-mini-windows var. Searched in all git log to find commits related to resize-mini-windows.

I suspect some Emacs default settings done by Manjaro somewhere that affect Emacs even with -Q.

C-h v system-configuration-options :

_"--sysconfdir=/etc --prefix=/usr --libexecdir=/usr/lib --with-tree-sitter --localstatedir=/var --with-cairo --disable-build-details --with-harfbuzz --with-libsystemd --with-modules --with-x-toolkit=gtk3 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFYSOURCE=2 -Wformat -Werror=format-security -fstack-clash-protection -fcf-protection -g -ffile-prefix-map=/build/emacs/src=/usr/src/debug/emacs -flto=auto' 'LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -flto=auto'"

But since default build of Emacs with ./configure.sh still has this bug, these options do not matter.

I haven't found any example or recipe that would break Helm when that binding is removed. So that I could realize "now that's why it's there, ok then". I may be missing something of course.

So yes perhaps the best would be an Emacs bug report.

I think that resize-mini-windows in Emacs works exactly as was intended. If its nil value prevents minibuffer line from autoresizing that in turn would fit the text face, then what's to complain about.

The bug can't be easily reproduced on some machines in current circumstances, so nothing else left to do. This issue can be closed if there are no other suggestions.

thierryvolpiatto commented 8 months ago

Alexander Karbivnichiy @.***> writes:

You can just try yourself, remove the binding and yank a multiline string at the header-line prompt.

Nothing breaks, Emacs replaces newlines with "^J" characters. I'm using dev setup for Helm in Emacs init that is forked and pull request with it created, after a thorough debugging.

Indeed, just tried. However this is with helm-display-header-line enabled (t), when we disable it (nil) and yank text in the header-line (the now fake minibuffer) the minibuffer (at bottom) IS resized to that many lines (bad). However this is true with and without resize-mini-windows let-bounded, IOW the resize-mini-windows let-binding have no effect. Alexander, I suggest we try for some time your PR and see if we have complaints, if so we will have a real life case where resize-mini-windows is indeed needed and we will be able to provide a fix.

-- Thierry

michael-heerdegen commented 8 months ago

Thierry Volpiatto @.***> writes:

Alexander, I suggest we try for some time your PR and see if we have complaints, if so we will have a real life case where resize-mini-windows is indeed needed and we will be able to provide a fix.

Can't say whether this is a good idea, but this only covers the symptoms of a problem we don't understand, and only for those users who have resize-mini-windows enabled. Alexander's screenshots show that the problem already happens with a one-line miniwindow. It should be displayed correctly right away without resizing. Please let's at least ask on emacs-dev.

thierryvolpiatto commented 8 months ago

Michael Heerdegen @.***> writes:

  1. ( ) text/plain (*) text/html

Thierry Volpiatto @.***> writes:

Alexander, I suggest we try for some time your PR and see if we have complaints, if so we will have a real life case where resize-mini-windows is indeed needed and we will be able to provide a fix.

Can't say whether this is a good idea, but this only covers the symptoms of a problem we don't understand, and only for those users who have resize-mini-windows enabled. Alexander's screenshots show that the problem already happens with a one-line miniwindow. It should be displayed correctly right away without resizing. Please let's at least ask on emacs-dev.

I agree but for now I don't know what to ask exactly on emacs-dev, I need some more time to think at it, if you know exacly what to ask please do.

Thanks.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: @.***>

-- Thierry

karbiv commented 8 months ago

Can't say whether this is a good idea, but this only covers the symptoms of a problem we don't understand, and only for those users who have resize-mini-windows enabled.

resize-mini-windows is enabled by default in Emacs, it's set to "grow only".

Alexander's screenshots show that the problem already happens with a one-line miniwindow. It should be displayed correctly right away without resizing. Please let's at least ask on emacs-dev.

I can setup this line to jump at Emacs C source functions from elisp by "M-.", to explore how minibuffer resizing works: (setq find-function-C-source-directory "~/path/to/emacs/src")

This would take time and I'm not sure that something is wrong with it. In general terms I know what could be the problem in Emacs. Probably minibuffer's code does not take into account overlays that display propertized strings when resize-mini-windows is disabled(nil). I guess Emacs devs first response would be "don't disable resize-mini-windows". This fix works without breaking anything so far, hopefully.

karbiv commented 7 months ago

Alexander, I suggest we try for some time your PR and see if we have complaints, if so we will have a real life case where resize-mini-windows is indeed needed and we will be able to provide a fix.

Closing for it not to hang in issues list. It can be reopened if need be.