bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Jimmy Yuen Ho Wong]

Fix for #38457, commit 3b0938c0420de2b845e7e8f8fbbb57ddc61718f2, seems
to have broken ido-vertical-mode and perhaps other minor modes that uses
similar techniques to render a custom ido completion list. Is there
another fix possible, or better yet, introduce a new overridable
function that controls how the prompt is displayed?

https://github.com/creichert/ido-vertical-mode.el/issues/48


In GNU Emacs 27.0.60 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G2022))
 of 2020-01-31 built on MobileCat.local
Repository revision: cdf8c31844263312aaf08527ce251edb32c5819c
Repository branch: HEAD
Windowing system distributor 'Apple', version 10.3.1671
System Description:  Mac OS X 10.14.6

Recent messages:
Can’t guess python-indent-offset, using defaults: 4
Importmagic and/or epc not found. importmagic.el will not be working.
Can’t guess python-indent-offset, using defaults: 4
Importmagic and/or epc not found. importmagic.el will not be working. [6 times]
ls does not support --dired; see ‘dired-use-ls-dired’ for more details.
Wrote /Users/wyuenho/.emacs.d/.emacs.desktop.lock
Desktop: 1 frame, 24 buffers restored.
Turning on magit-auto-revert-mode...done (1.118s, 74 buffers checked)
For information about GNU Emacs and the GNU system, type C-h C-a.
Package cl is deprecated

Configured using:
 'configure --prefix=/opt/local --without-dbus --without-gconf
 --without-libotf --without-m17n-flt --without-gpm --with-gnutls
 --with-xml2 --with-modules --infodir /opt/local/share/info/emacs
 --with-json --without-harfbuzz --with-ns --with-lcms2
 --with-imagemagick --with-rsvg 'CFLAGS=-pipe -Os
 -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk -arch
 x86_64' 'CPPFLAGS=-I/opt/local/include
 -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk'
 'LDFLAGS=-L/opt/local/lib -Wl,-headerpad_max_install_names -Wl,-no_pie
 -Wl,-syslibroot,/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk
 -arch x86_64''

Configured features:
RSVG IMAGEMAGICK GLIB NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS XIM NS MODULES THREADS JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: 

Minor modes in effect:
  flycheck-pos-tip-mode: t
  projectile-rails-global-mode: t
  projectile-mode: t
  company-quickhelp-mode: t
  company-quickhelp-local-mode: t
  company-box-mode: t
  rainbow-mode: t
  elisp-def-mode: t
  display-line-numbers-mode: t
  subword-mode: t
  form-feed-mode: t
  purpose-mode: t
  imenu-list-minor-mode: t
  diff-hl-flydiff-mode: t
  company-flx-mode: t
  yas-minor-mode: t
  override-global-mode: t
  winner-mode: t
  which-key-mode: t
  smooth-scrolling-mode: t
  show-smartparens-global-mode: t
  show-smartparens-mode: t
  smartparens-global-mode: t
  smartparens-mode: t
  show-paren-mode: t
  savehist-mode: t
  save-place-mode: t
  rxt-global-mode: t
  rxt-mode: t
  recentf-mode: t
  ido-vertical-mode: t
  ido-ubiquitous-mode: t
  global-whitespace-cleanup-mode: t
  whitespace-cleanup-mode: t
  global-origami-mode: t
  origami-mode: t
  global-move-dup-mode: t
  move-dup-mode: t
  global-magit-file-mode: t
  which-function-mode: t
  magit-auto-revert-mode: t
  global-auto-revert-mode: t
  global-git-commit-mode: t
  shell-dirtrack-mode: t
  server-mode: t
  global-hl-line-mode: t
  global-flycheck-mode: t
  global-diff-hl-mode: t
  diff-hl-mode: t
  flx-ido-mode: t
  ido-everywhere: t
  editorconfig-mode: t
  desktop-save-mode: t
  delete-selection-mode: t
  company-statistics-mode: t
  global-company-mode: t
  company-mode: t
  auto-compile-on-save-mode: t
  auto-compile-mode: t
  async-bytecomp-package-mode: t
  amx-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  temp-buffer-resize-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  visual-line-mode: t
  transient-mark-mode: t

Load-path shadows:
/opt/local/share/emacs/site-lisp/cmake-mode hides /Users/wyuenho/.emacs.d/elpa/cmake-mode-20190710.1319/cmake-mode

Features:
(shadow sort mail-extr emacsbug sendmail two-column dired-hide-dotfiles
vc-mtn vc-hg vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs diff-hl-dired
dired-collapse dired-hacks-utils all-the-icons-dired lsp-ui
lsp-ui-flycheck lsp-ui-doc goto-addr lsp-ui-imenu lsp-ui-peek
lsp-ui-sideline importmagic epc ctable concurrent deferred blacken
py-autopep8 python-docstring py-isort smartparens-python python tramp-sh
docker-tramp tramp-cache tramp tramp-loaddefs trampver tramp-integration
files-x tramp-compat ls-lisp flycheck-pos-tip add-node-modules-path
vc-git tabify jka-compr projectile-rails rake inflections inf-ruby
smartparens-ruby ruby-mode smie projectile company-quickhelp pos-tip
company-box company-box-doc company-box-icons company-keywords
company-etags company-gtags company-dabbrev-code company-dabbrev
company-yasnippet company-capf company-emoji company-emoji-list
company-files company-cmake company-xcode company-clang company-semantic
company-eclim company-template rainbow-mode xterm-color elisp-def ert pp
debug backtrace display-line-numbers cap-words superword subword
smartparens-config smartparens-org smartparens-markdown smartparens-text
form-feed solarized-dark-theme solarized-theme solarized solarized-faces
all-the-icons all-the-icons-faces data-material data-weathericons
data-octicons data-fileicons data-faicons data-alltheicons
spaceline-config spaceline-segments spaceline powerline
powerline-separators powerline-themes hideshow window-purpose-x shut-up
window-purpose window-purpose-fixes window-purpose-prefix-overload
window-purpose-switch window-purpose-layout window-purpose-core
window-purpose-configuration eieio-compat window-purpose-utils
imenu-list windmove magit-lfs magit-todos hl-todo org ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-macro org-footnote org-src ob-comint
org-pcomplete org-list org-faces org-entities org-version ob-emacs-lisp
ob-core ob-eval org-table ol org-keys org-compat org-macs org-loaddefs
cal-menu calendar cal-loaddefs forge-list forge-commands forge-semi
forge-bitbucket buck forge-gogs gogs forge-gitea gtea forge-gitlab glab
forge-github ghub-graphql treepy gsexp ghub let-alist forge-notify
forge-revnote forge-pullreq forge-issue forge-topic parse-time iso8601
bug-reference forge-post forge-repo forge forge-core forge-db closql
emacsql-sqlite emacsql emacsql-compiler url-http url-auth url-gw
diff-hl-flydiff dumb-jump popup etags fileloop generator rg rg-info-hack
rg-menu rg-ibuffer rg-result wgrep-rg wgrep rg-history rg-header
ibuf-ext ibuffer ibuffer-loaddefs grep yard-mode poly-markdown polymode
poly-lock polymode-base polymode-weave polymode-export polymode-compat
polymode-methods polymode-core polymode-classes eieio-custom eieio-base
flycheck-objc-clang cl-lib-highlight company-lsp company-flx dap-mode
dap-overlays lsp-clients lsp-eslint lsp-verilog lsp-json url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
mailcap lsp-csharp gnutls lsp-pwsh lsp-terraform lsp-yaml lsp-vhdl
lsp-haxe lsp-erlang lsp-fsharp lsp-metals lsp-elm lsp-dart lsp-clojure
lsp-go lsp-xml lsp-css lsp-intelephense lsp-vetur lsp-html
lsp-solargraph lsp-rust lsp-pyls lsp lsp-mode xref url-util spinner
network-stream nsm markdown-mode color noutline outline lv inline ht f
em-glob esh-util dash-functional bindat flymake-proc flymake compile
warnings project yasnippet-snippets yasnippet pager-default-keybindings
pager browse-kill-ring delight use-package-bind-key use-package-delight
osx-trash bind-key exec-path-from-shell quelpa-use-package
use-package-core quelpa lisp-mnt help-fns radix-tree winner which-key
smooth-scrolling smartparens thingatpt paren savehist saveplace pcre2el
rxt re-builder recentf tree-widget ido-vertical-mode
ido-completing-read+ memoize cus-edit wid-edit minibuf-eldef help-at-pt
whitespace-cleanup-mode whitespace origami origami-parsers cl move-dup
magit-submodule magit-obsolete magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func imenu
magit-diff smerge-mode diff magit-core magit-autorevert autorevert
filenotify magit-margin magit-transient magit-process magit-mode
git-commit transient magit-git magit-section magit-utils crm log-edit
message rmc puny dired dired-loaddefs format-spec rfc822 mml mml-sec epa
derived epg epg-config gnus-util rmail rmail-loaddefs
text-property-search time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev
mail-utils gmm-utils mailheader pcvs-util add-log with-editor cl-extra
shell pcomplete comint ring server hl-line flycheck ansi-color find-func
help-mode dash diff-hl vc-dir ewoc vc vc-dispatcher diff-mode easy-mmode
flx-ido flx ido editorconfig desktop frameset delsel company-statistics
company pcase auto-compile packed async-bytecomp advice async amx s
cus-start cus-load finder-inf edmacro kmacro rx info package easymenu
browse-url url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json subr-x map url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/ns-win
ns-win ucs-normalize mule-util term/common-win tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 692021 292293)
 (symbols 48 46555 2)
 (strings 32 181527 57204)
 (string-bytes 1 5576343)
 (vectors 16 92141)
 (vector-slots 8 1579832 416572)
 (floats 8 1046 1071)
 (intervals 56 4457 340)
 (buffers 1000 79))

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> Date: Fri, 31 Jan 2020 23:53:19 +0000
> 
> Fix for #38457, commit 3b0938c0420de2b845e7e8f8fbbb57ddc61718f2, seems
> to have broken ido-vertical-mode and perhaps other minor modes that uses
> similar techniques to render a custom ido completion list. Is there
> another fix possible, or better yet, introduce a new overridable
> function that controls how the prompt is displayed?
> 
> https://github.com/creichert/ido-vertical-mode.el/issues/48

Please show a recipe to reproduce the problem.  I don't use
ido-vertical-mode, and the issue you point to doesn't provide any
description of the actual problem, either.  So it's unclear what
exactly was broken by that commit and why.

Thanks.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Jimmy Yuen Ho Wong]

On 01/02/2020 08:12, Eli Zaretskii wrote:
>> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
>> Date: Fri, 31 Jan 2020 23:53:19 +0000
>>
>> Fix for #38457, commit 3b0938c0420de2b845e7e8f8fbbb57ddc61718f2, seems
>> to have broken ido-vertical-mode and perhaps other minor modes that uses
>> similar techniques to render a custom ido completion list. Is there
>> another fix possible, or better yet, introduce a new overridable
>> function that controls how the prompt is displayed?
>>
>> https://github.com/creichert/ido-vertical-mode.el/issues/48
> Please show a recipe to reproduce the problem.  I don't use
> ido-vertical-mode, and the issue you point to doesn't provide any
> description of the actual problem, either.  So it's unclear what
> exactly was broken by that commit and why.
>
> Thanks.

Steps for reproduction:

1. $ echo "(ido-mode)(require 'ido-vertical-mode)(ido-vertical-mode)" >
.emacs

2. $ emacs

3. C-x C-f

4. The Find file prompt is missing since that commit.

I've also attached screenshots here.

https://github.com/creichert/ido-vertical-mode.el/issues/48#issuecomment-581033055

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org
> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> Date: Sun, 2 Feb 2020 13:58:36 +0000
> 
> 1. $ echo "(ido-mode)(require 'ido-vertical-mode)(ido-vertical-mode)" >
> .emacs
> 
> 2. $ emacs
> 
> 3. C-x C-f
> 
> 4. The Find file prompt is missing since that commit.

I see, thanks.

However, I'm not sure your proposal is the optimal solution: what will
happen if ido-exhibit uses the old code, and a message is displayed
while the user goes through the completion candidates?  The trick with
the overlay was to make sure the message is displayed without
overwriting the minibuffer prompt.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Date: Sun, 02 Feb 2020 18:13:47 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 39379@debbugs.gnu.org
> 
> > 1. $ echo "(ido-mode)(require 'ido-vertical-mode)(ido-vertical-mode)" >
> > .emacs
> > 
> > 2. $ emacs
> > 
> > 3. C-x C-f
> > 
> > 4. The Find file prompt is missing since that commit.
> 
> I see, thanks.
> 
> However, I'm not sure your proposal is the optimal solution: what will
> happen if ido-exhibit uses the old code, and a message is displayed
> while the user goes through the completion candidates?  The trick with
> the overlay was to make sure the message is displayed without
> overwriting the minibuffer prompt.

I think this could be a bug in the display engine, related to resizing
the mini-window when an after-string that includes many newlines is
put at EOB.  Let me look into it.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Date: Sun, 02 Feb 2020 19:23:46 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 39379@debbugs.gnu.org, dgutov@yandex.ru
> 
> I think this could be a bug in the display engine, related to resizing
> the mini-window when an after-string that includes many newlines is
> put at EOB.  Let me look into it.

Looks like my guess was correct.  I'll try to devise a solution.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Date: Sun, 02 Feb 2020 20:06:16 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 39379@debbugs.gnu.org, dgutov@yandex.ru
> 
> > I think this could be a bug in the display engine, related to resizing
> > the mini-window when an after-string that includes many newlines is
> > put at EOB.  Let me look into it.
> 
> Looks like my guess was correct.  I'll try to devise a solution.

Workaround: set ido-max-prospects to 7 or less.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Jimmy Yuen Ho Wong]

On 02/02/2020 18:38, Eli Zaretskii wrote:
> Workaround: set ido-max-prospects to 7 or less.

That gets the prompt back but now the cursor is at the wrong place lol

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org, dgutov@yandex.ru
> From: Jimmy Yuen Ho Wong <wyuenho@gmail.com>
> Date: Sun, 2 Feb 2020 21:33:22 +0000
> 
> On 02/02/2020 18:38, Eli Zaretskii wrote:
> > Workaround: set ido-max-prospects to 7 or less.
> 
> That gets the prompt back but now the cursor is at the wrong place lol

That's a separate problem that should be easy to fix.  The problem is
that the new code in ido.el tells Emacs to display the cursor on the
first character of the overlay string, but that first character is a
newline in the case of ido-vertical-mode, and Emacs cannot place the
cursor on a newline.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 03.02.2020 0:33, Jimmy Yuen Ho Wong wrote:
> That gets the prompt back but now the cursor is at the wrong place lol

I've suggested a workaround for that in the GitHub issue commends as 
soon as you opened this bug report (2 days ago).

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 3 Feb 2020 16:19:12 +0300
> 
> On 03.02.2020 0:33, Jimmy Yuen Ho Wong wrote:
> > That gets the prompt back but now the cursor is at the wrong place lol
> 
> I've suggested a workaround for that in the GitHub issue commends as 
> soon as you opened this bug report (2 days ago).

That's exactly the solution I had in mind.  I suggest that you push it
to the Emacs Git repository without waiting for the solution of the
larger issue (which will take me some time to design and test).

Btw, the reason you didn't see the problem is that the list of
candidates in your case was very small.  That's what the workaround
with ido-max-prospects <= 7 does when the list is longer.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 03.02.2020 18:40, Eli Zaretskii wrote:

>> I've suggested a workaround for that in the GitHub issue commends as
>> soon as you opened this bug report (2 days ago).
> 
> That's exactly the solution I had in mind.  I suggest that you push it
> to the Emacs Git repository without waiting for the solution of the
> larger issue (which will take me some time to design and test).

Not for us to do, it's a change to be done in ido-vertical-mode, not in 
ido.el (they override a built-in function).

> Btw, the reason you didn't see the problem is that the list of
> candidates in your case was very small.  That's what the workaround
> with ido-max-prospects <= 7 does when the list is longer.

Yes, I saw that later. :-(

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org, wyuenho@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 Feb 2020 00:51:49 +0300
> 
> On 03.02.2020 18:40, Eli Zaretskii wrote:
> 
> >> I've suggested a workaround for that in the GitHub issue commends as
> >> soon as you opened this bug report (2 days ago).
> > 
> > That's exactly the solution I had in mind.  I suggest that you push it
> > to the Emacs Git repository without waiting for the solution of the
> > larger issue (which will take me some time to design and test).
> 
> Not for us to do, it's a change to be done in ido-vertical-mode, not in 
> ido.el (they override a built-in function).

I think this is for ido.el to do: it is ido.el that puts the cursor on
the overlay string, so it is up to it to make sure the cursor can be
put where it puts the 'cursor' property.  It's a simple change: if the
text to be displayed starts with a newline, prepend a SPC to it when
defining the after-string.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 04.02.2020 6:27, Eli Zaretskii wrote:

> I think this is for ido.el to do: it is ido.el that puts the cursor on
> the overlay string, so it is up to it to make sure the cursor can be
> put where it puts the 'cursor' property.  It's a simple change: if the
> text to be displayed starts with a newline, prepend a SPC to it when
> defining the after-string.

But... the change in ido-vertical-mode is simpler still: just add an 
extra argument to concat.

If we do that in ido.do, the reason why would be fairly non-obvious from 
that code. After all, we don't provide any customization points in 
there, hooks or otherwise. So we risk accidentally losing that change in 
some future version.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org, wyuenho@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 Feb 2020 16:19:05 +0300
> 
> On 04.02.2020 6:27, Eli Zaretskii wrote:
> 
> > I think this is for ido.el to do: it is ido.el that puts the cursor on
> > the overlay string, so it is up to it to make sure the cursor can be
> > put where it puts the 'cursor' property.  It's a simple change: if the
> > text to be displayed starts with a newline, prepend a SPC to it when
> > defining the after-string.
> 
> But... the change in ido-vertical-mode is simpler still: just add an 
> extra argument to concat.

That's true, but AFAIU the problem is not limited to
ido-vertical-mode, it will happen whenever the string to display
starts with a newline.  Such a string is entirely legitimate, isn't
it?  And the caller cannot possibly know that ido-exhibit will put the
'cursor' property on the first character of that text.  So I think it
isn't entirely reasonable to expect such callers to defend themselves
against internal implementation details of ido-exhibit.

> If we do that in ido.do, the reason why would be fairly non-obvious from 
> that code.

If the test for the leading newline is there, the reason is quite
obvious, and we can have a comment for those who don't know enough
about the 'cursor' property and cursor positioning.  I think the
result will more obvious than a mysterious concatenation of a blank in
ido-vertical-mode, which will need a comment explaining it as well.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 04.02.2020 18:40, Eli Zaretskii wrote:

>> But... the change in ido-vertical-mode is simpler still: just add an
>> extra argument to concat.
> 
> That's true, but AFAIU the problem is not limited to
> ido-vertical-mode, it will happen whenever the string to display
> starts with a newline.  Such a string is entirely legitimate, isn't
> it? And the caller cannot possibly know that ido-exhibit will put the
> 'cursor' property on the first character of that text.  So I think it
> isn't entirely reasonable to expect such callers to defend themselves
> against internal implementation details of ido-exhibit.

Umm, it's ido-exhibit that calls ido-completions. And ido-completions, 
as defined in ido.el, never returns such strings.

Anyway, since you insist, I've pushed that change.

>> If we do that in ido.do, the reason why would be fairly non-obvious from
>> that code.
> 
> If the test for the leading newline is there, the reason is quite
> obvious, and we can have a comment for those who don't know enough
> about the 'cursor' property and cursor positioning.  I think the
> result will more obvious than a mysterious concatenation of a blank in
> ido-vertical-mode, which will need a comment explaining it as well.

Yes, ok. Although our general policy, I think, is that external packages 
that use questionable practices (such as redefining functions, instead 
of using whatever available public customization points there are) are 
generally left to their own devices.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org, wyuenho@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 5 Feb 2020 02:55:00 +0300
> 
> Anyway, since you insist, I've pushed that change.

Thanks.

> > If the test for the leading newline is there, the reason is quite
> > obvious, and we can have a comment for those who don't know enough
> > about the 'cursor' property and cursor positioning.  I think the
> > result will more obvious than a mysterious concatenation of a blank in
> > ido-vertical-mode, which will need a comment explaining it as well.
> 
> Yes, ok. Although our general policy, I think, is that external packages 
> that use questionable practices (such as redefining functions, instead 
> of using whatever available public customization points there are) are 
> generally left to their own devices.

True.  But I think this case is somewhat special, as ido-exhibit
handles the completion list in a way that cannot be easily guessed in
advance.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Date: Sun, 02 Feb 2020 20:06:16 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 39379@debbugs.gnu.org, dgutov@yandex.ru
> 
> > Date: Sun, 02 Feb 2020 19:23:46 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 39379@debbugs.gnu.org, dgutov@yandex.ru
> > 
> > I think this could be a bug in the display engine, related to resizing
> > the mini-window when an after-string that includes many newlines is
> > put at EOB.  Let me look into it.
> 
> Looks like my guess was correct.  I'll try to devise a solution.

Sorry for a relatively long delay.  In my defense, I first tried a
couple of easy solutions, but they didn't work.  And that got me
thinking, since my experience with solving issues like this due to
overlay strings is that the solutions tend to be less than elegant,
and take several attempts to get them right.

So now, after thinking about this for some time, I think I want to
change my mind and ask why do we need to use an overlay with
after-string in ido.el?  Re-reading related discussions, it seems the
answer is "so that the temporary display of messages is not at the end
of the minibuffer, where it could be invisible due to
resize-mini-windows being nil or restrictions imposed by ido.el via
ido-max-window-height".  Is that the correct conclusion?  If so, then
I think we can solve that problem without overlays.  See the proposed
patch below, which basically reverts ido.el to its previous shape, and
uses a special text property to instruct set-minibuffer-message where
to put the overlay (defaulting to EOB).

WDYT?

diff --git a/lisp/ido.el b/lisp/ido.el
index 6707d81407..edc848f52f 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -4728,16 +4728,10 @@ ido-exhibit
 	(let ((inf (ido-completions contents)))
 	  (setq ido-show-confirm-message nil)
 	  (ido-trace "inf" inf)
-          (when ido--overlay
-            (delete-overlay ido--overlay))
-          (let ((o (make-overlay (point-max) (point-max) nil t t)))
-            (when (> (length inf) 0)
-              ;; For hacks that redefine ido-completions function (bug#39379)
-              (when (eq (aref inf 0) ?\n)
-                (setq inf (concat " " inf)))
-              (put-text-property 0 1 'cursor t inf))
-            (overlay-put o 'after-string inf)
-            (setq ido--overlay o)))
+          (let ((pos (point)))
+            (insert inf)
+            (put-text-property pos (1+ pos) 'minibuffer-message t))
+          )
 	))))
 
 (defun ido-completions (name)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 0589211877..767fc8dff8 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -763,6 +763,16 @@ minibuffer-message-clear-timeout
 (defvar minibuffer-message-timer nil)
 (defvar minibuffer-message-overlay nil)
 
+(defun minibuffer--message-overlay-pos ()
+  "Return position where `set-minibuffer-message' shall put message overlay."
+  ;; Starting from point, look for non-nil 'minibuffer-message'
+  ;; property, and return its position.  If none found, return the EOB
+  ;; position.
+  (let* ((pt (point))
+         (propval (get-text-property pt 'minibuffer-message)))
+    (if propval pt
+      (next-single-property-change pt 'minibuffer-message nil (point-max)))))
+
 (defun set-minibuffer-message (message)
   "Temporarily display MESSAGE at the end of the minibuffer.
 The text is displayed for `minibuffer-message-clear-timeout' seconds
@@ -784,8 +794,9 @@ set-minibuffer-message
 
       (clear-minibuffer-message)
 
-      (setq minibuffer-message-overlay
-            (make-overlay (point-max) (point-max) nil t t))
+      (let ((ovpos (minibuffer--message-overlay-pos)))
+        (setq minibuffer-message-overlay
+              (make-overlay ovpos ovpos nil t t)))
       (unless (zerop (length message))
         ;; The current C cursor code doesn't know to use the overlay's
         ;; marker's stickiness to figure out whether to place the cursor

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 10.02.2020 17:44, Eli Zaretskii wrote:
> So now, after thinking about this for some time, I think I want to
> change my mind and ask why do we need to use an overlay with
> after-string in ido.el?  Re-reading related discussions, it seems the
> answer is "so that the temporary display of messages is not at the end
> of the minibuffer, where it could be invisible due to
> resize-mini-windows being nil or restrictions imposed by ido.el via
> ido-max-window-height".  Is that the correct conclusion?  If so, then
> I think we can solve that problem without overlays.  See the proposed
> patch below, which basically reverts ido.el to its previous shape, and
> uses a special text property to instruct set-minibuffer-message where
> to put the overlay (defaulting to EOB).

The patch more or less works, with the exception of the case where POS 
is EOB (e.g. when the sole completion is fully typed out). But that 
should be fixable as well.

The reason I used after-string, though, is that icomplete-mode does 
that. And either it should have the same problems, or ido creates some 
circumstances where the problems show up.

The former seems to be the case, though. Like, if I set 
max-mini-window-height to 1, then icomplete-mode also exhibits bug#39433.

And while there is no xxx-vertical-mode for icomplete-mode, we would 
probably want one to be written someday. Sounds like when that happens, 
icomplete-vertical-mode would trigger this same bug as well unless we 
fix it in the display engine or find some other general way to avoid it.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: wyuenho@gmail.com, 39379@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 12 Feb 2020 01:42:22 +0200
> 
> The patch more or less works, with the exception of the case where POS 
> is EOB (e.g. when the sole completion is fully typed out). But that 
> should be fixable as well.

Right, updated patch below.

> The reason I used after-string, though, is that icomplete-mode does 
> that. And either it should have the same problems, or ido creates some 
> circumstances where the problems show up.
> 
> The former seems to be the case, though. Like, if I set 
> max-mini-window-height to 1, then icomplete-mode also exhibits bug#39433.

OK, but this is an old problem, because I see the same in Emacs 26.3.
It wasn't caused by the new implementation of set-minibuffer-message.

> And while there is no xxx-vertical-mode for icomplete-mode, we would 
> probably want one to be written someday. Sounds like when that happens, 
> icomplete-vertical-mode would trigger this same bug as well unless we 
> fix it in the display engine or find some other general way to avoid it.

Well, we hardly ever fix future bugs.  And limiting mini-windows to
such small height is asking for trouble.

But okay, I'm fine with leaving this bug open, and will try to fix the
problem on master.  I do want to install the patch below, because it
fixes a problem directly caused by set-minibuffer-message, so we
should fix this in Emacs 27.  Any objections?

diff --git a/lisp/ido.el b/lisp/ido.el
index 6707d81407..d74ea69ae3 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -4728,16 +4728,11 @@ ido-exhibit
 	(let ((inf (ido-completions contents)))
 	  (setq ido-show-confirm-message nil)
 	  (ido-trace "inf" inf)
-          (when ido--overlay
-            (delete-overlay ido--overlay))
-          (let ((o (make-overlay (point-max) (point-max) nil t t)))
-            (when (> (length inf) 0)
-              ;; For hacks that redefine ido-completions function (bug#39379)
-              (when (eq (aref inf 0) ?\n)
-                (setq inf (concat " " inf)))
-              (put-text-property 0 1 'cursor t inf))
-            (overlay-put o 'after-string inf)
-            (setq ido--overlay o)))
+          (let ((pos (point)))
+            (insert inf)
+            (if (< pos (point-max))
+                (put-text-property pos (1+ pos) 'minibuffer-message t)))
+          )
 	))))
 
 (defun ido-completions (name)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 0589211877..767fc8dff8 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -763,6 +763,16 @@ minibuffer-message-clear-timeout
 (defvar minibuffer-message-timer nil)
 (defvar minibuffer-message-overlay nil)
 
+(defun minibuffer--message-overlay-pos ()
+  "Return position where `set-minibuffer-message' shall put message overlay."
+  ;; Starting from point, look for non-nil 'minibuffer-message'
+  ;; property, and return its position.  If none found, return the EOB
+  ;; position.
+  (let* ((pt (point))
+         (propval (get-text-property pt 'minibuffer-message)))
+    (if propval pt
+      (next-single-property-change pt 'minibuffer-message nil (point-max)))))
+
 (defun set-minibuffer-message (message)
   "Temporarily display MESSAGE at the end of the minibuffer.
 The text is displayed for `minibuffer-message-clear-timeout' seconds
@@ -784,8 +794,9 @@ set-minibuffer-message
 
       (clear-minibuffer-message)
 
-      (setq minibuffer-message-overlay
-            (make-overlay (point-max) (point-max) nil t t))
+      (let ((ovpos (minibuffer--message-overlay-pos)))
+        (setq minibuffer-message-overlay
+              (make-overlay ovpos ovpos nil t t)))
       (unless (zerop (length message))
         ;; The current C cursor code doesn't know to use the overlay's
         ;; marker's stickiness to figure out whether to place the cursor

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 12.02.2020 20:18, Eli Zaretskii wrote:
> OK, but this is an old problem, because I see the same in Emacs 26.3.
> It wasn't caused by the new implementation of set-minibuffer-message.

True.

> Well, we hardly ever fix future bugs.  And limiting mini-windows to
> such small height is asking for trouble.

I don't know the reason why the user who reported this problem did it, 
but they *did* find a need to set ido-max-window-height to 1, apparently 
(which has the same effect).

TBH, of course, I'm more worried about the "future bug" regarding 
icomplete-vertical-mode. But maybe we'll switch to child frames 
exclusively for this purpose by them.

> But okay, I'm fine with leaving this bug open, and will try to fix the
> problem on master.  I do want to install the patch below, because it
> fixes a problem directly caused by set-minibuffer-message, so we
> should fix this in Emacs 27.  Any objections?

None from me. If that's the simplest fix we can do for the release, 
please go ahead.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Eli Zaretskii]

> Cc: 39379@debbugs.gnu.org, wyuenho@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 12 Feb 2020 20:24:40 +0200
> 
> > But okay, I'm fine with leaving this bug open, and will try to fix the
> > problem on master.  I do want to install the patch below, because it
> > fixes a problem directly caused by set-minibuffer-message, so we
> > should fix this in Emacs 27.  Any objections?
> 
> None from me. If that's the simplest fix we can do for the release, 
> please go ahead.

Done.

bug#39379: 27.0.60; Fix for #38457 broke ido-vertical-mode

[Dmitry Gutov]

On 12.02.2020 20:24, Dmitry Gutov wrote:
> TBH, of course, I'm more worried about the "future bug" regarding 
> icomplete-vertical-mode. But maybe we'll switch to child frames 
> exclusively for this purpose by them.

Speaking of "future bugs",

   (setq icomplete-separator "\n")

effectively enables such "vertical mode" for icomplete.

Unfortunately, it suffers from the same problem. :-(

JFYI.