Re: feature/icomplete-vertical

[João Távora]

Ergus <spacibba@aol.com> writes:

> On Mon, Oct 05, 2020 at 12:47:39AM +0100, Jo�o T�vora wrote:
>
> Hi Joao:
>
> Please look at the previous messages in this same thread to see the
> problems with your approach.
>
> So far there is:
>
> 1) The problem when using different font in minibuffer reported by
> Jixiuf 2) the long line issue from Gregory (but also mentioned before)
> 3) the ellipsis issue mentioned by Eli
> 4) The prompt issue (which seems is not going to be solved as it is more
> a design choice or a feature than a real issue)
> 5) Respect the user customs max-mini-window-height and
> resize-mini-windows so we shouldn't call enlarge-window and as an
> core package we must not conflict with such customs.
> 6) The removal of {} in vertical mode because in that case they are
> redundant.
> 7) The compatibility with packages like maple-minibuffer reported by
> Dimitry.
> 8) The case when using multiline separator.
> 9) Move the first candidate to a newline instead of keeping it inline.
>
> The patch I propose is not complex at all considering all the above
> conditions. The only complexity comes from the height calculation in
> pixels which is something that will need to be done in any case either
> for 1, 2, 3 or 4.

Thanks for listing these again, for my benefit.  I've not been following
this very closely, and needed a summary to give me context after trying
your patch again, as you requested.

To be clear: I'm not "against" this functionality or even against it
becoming a part of Emacs or Emacs core.  I think solving the problems
you list above is fine (though I haven't been by them, at least not yet)

I'm not too much concerned about the complexity of implementation (such
as the height calculation).  I am concerned the changes to the user
interface, of which your patch seems to bring many, or at least more
than I would have expected.  It seems to be that your solution mixes
bugfixes and new features, which is what I object to.  I think your
solution could be trimmed down so that it solves _only_ the technical
problems you list above without adding new user interfaces.

In other words, don't see why the points above, which sound like
basically bugs, need any new "defcustom".  This is why I asked you to
(re)list them.  Then I can start with my naive implementation and fix
them one by one, without adding new interfaces.  Of course, then I will
be repeating some work you've already done, so this is why I'm arguing
for this separation in _your_ work.

Could you structure your work so that one part fixes the above problems,
and another part adds the new features, like customizing bracket types
and such, and is should be discussed as a separate matter.

After my sig, some comments that illustrate my points.

I took this diff with git diff master...feature/icomplete-vertical

João

> -(defcustom icomplete-separator " | "
> -  "String used by Icomplete to separate alternatives in the minibuffer."
> -  :type 'string
> -  :version "24.4")
> +(defcustom icomplete-vertical nil
> +  "Enable `icomplete' vertical mode."
> +  :type 'bool
> +  :version "28.1")

I would think it more elegant to keep `icomplete-separator` as the entry
point to verticality.  Certainly it should be possible to analyse the
string and decide whether it implies verticality and needs special
worries.

  (defcustom icomplete-hide-common-prefix t
   "When non-nil, hide common prefix from completion candidates.
@@ -97,6 +97,12 @@ icomplete-first-match
   "Face used by Icomplete for highlighting first match."
   :version "24.4")
 
> +(defface icomplete-common-match '((t :inherit 'highlight
> +                                     :underline t
> +                                     :weight bold))
> +  "Face used by Icomplete for highlighting common completion."
> +  :version "28.1")


What is a "common match"?  Is this exclusive to icomplete verticalness?
Doesn't seem so, is this not a side feature?

> -(defcustom icomplete-minibuffer-setup-hook nil
> +(defvar icomplete-ellipsis nil)
> +
> +(defcustom icomplete--minibuffer-setup-hook nil
>    "Icomplete-specific customization of minibuffer setup.

Why has this minibuffer hook been made an internal variable?

> -    (define-key map (kbd "<right>") 'icomplete-forward-completions)
> -    (define-key map (kbd "<left>") 'icomplete-backward-completions)

Why have these been removed from icomplete-fido-mode-map?  Also, I have
C-r and C-r in that map, will it work in the verticality?  I would hope
it would, as it does not in my naive verticality implementation.

> -;;;_ > icomplete-minibuffer-setup ()
> -(defun icomplete-minibuffer-setup ()
> +;; Vertical functions
> +
> +(defcustom icomplete-separator-vertical " \n"
> +  "String used by Icomplete to separate alternatives in the minibuffer."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-list-indicators-vertical (cons "" "")
> +  "Indicator bounds to list alternatives in the minibuffer."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-require-indicators-vertical (cons "" "")
> +  "Indicator bounds for match in the minibuffer when require-match."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-not-require-indicators-vertical (cons "" "")
> +  "Indicator bounds for match in the minibuffer when not require-match."
> +  :type 'string
> +  :version "28.1")

These are examples of "interface bloat" that do not seem to directly
address the 9 problems you listed in the beginning of the message
(again, I am not against these features per se, mind you)

> +(defvar icomplete--vertical-mode-map

If this keymap is "internal" (as indicated by the two "--") how am I
supposed to customize the keybindings?

> +(defcustom icomplete-list-indicators-horizontal (cons "{" "}")
> +  "Indicator bounds to list alternatives in the minibuffer."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-require-indicators-horizontal (cons "(" ")")
> +  "Indicator bounds for match in the minibuffer when require-match."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-not-require-indicators-horizontal (cons "[" "]")
> +  "Indicator bounds for match in the minibuffer when not require-match."
> +  :type 'string
> +  :version "28.1")

Again, these seem like improvements that are not directly related to
verticality.  (Maybe they are very good improvements, but still...

João