Re: feature/icomplete-vertical

[Ergus <spacibba@aol.com>]

Hi Joao

The changes you show are the old version of the branch where I was
trying some experiments... I should have called it "scratch" to that
first version. I understand your concerns now. But not, this is not the
new version

I cleaned all of it in a recent forced push some weeks ago. Please give
a look at the new version. It is simpler and doesn't give any change in
the interface.

Ergus



On Mon, Oct 05, 2020 at 10:06:22AM +0100, Jo�o T�vora wrote:
>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
>