RE: propose adding Icicles to Emacs

[Drew Adams]

> >> Redefining completing-read is a no-no in my book.  Please try
> >> to rewrite your code so as not to do that (e.g. using
> >> minibuffer-setup-hook).
>
> > I don't see how to do that.  Concrete suggestions welcome.  Please have
> > a look at the existing code.  Likewise for the other, related functions
> > (`read-file-name', etc.).
>
> I'd rather it be the other way around: tell us what you're trying
> to do (and finding you cannot do).

As I said, I don't know how to approach that. What I'm trying to do now
should be clear from the code, but see below, for some concrete examples.

> > I'm not sure why the redefinition within Icicle mode is a
> > no-no.  I'm not arguing, but I don't see the reason for the
> > prohibition.
>
> It's OK for an external package (tho I prefer defadvice), but for
> reasons of maintainability and general principle it's not good
> inside Emacs. We probably do have such things already inside
> Emacs, but we want to reduce such occurrences rather than increase
> them.
>
> > Perhaps if you explain what you mean about using minibuffer-setup-hook,
> > there will be no need.
>
> E.g. Why not do the fiddling of initial-input with icicle-initial-value in
> a minibuffer-setup-hook?

That particular initialization is only appropriate for `completing-read',
not, for example, for `read-file-name' or other functions that cause
`minibuffer-setup-hook' to be invoked. In particular, `read-file-name' has a
very different treatment of the initial value.

I'm pretty sure that, had it been feasible to put the stuff that is done in
`completing-read' onto `minibuffer-setup-hook', that would have been the
first thing that I would have done. I don't recall now the details behind
each of the decisions to do what is done now in `completing-read' (or in
`read-file-name' etc.), and I would be reluctant to simply try to move any
such code to a hook.

I guess I'm asking that you trust, to some extent, that I did what I could
in that regard, but I'm open to concrete suggestions if you have them. I'm
grateful for new pairs of eyes and greater expertise and knowledge of Emacs
code. What I don't want to do is risk breaking things. As I said, there is a
lot going on, and it's not easy or obvious how to test everything after
making a fundamental change.

I really propose that the basic implementation be incorporated as it is now.
That should be OK, since the redefinitions are encapsulated in a minor mode.
We could revisit possible optimizations, cleanup, and other code
improvements later, carefully, and little by little.

For example, we could go through the Icicle-mode version of
`completing-read' piece by piece, trying to see whether such-and-such a
piece could be better done elsewhere or in a different way - just as I just
did above for `icicle-initial-value'. I believe that what is there now is
specific to `completing-read', and would be inappropriate (it wouldn't work)
if it were moved, say, to `minibuffer-setup-hook'.

Each of the pieces of code in the Icicles definition of `completing-read'
has a comment explaining the intention, so I think that it should be
possible for someone to make a suggestion regarding such a piece. For
example:

  ;; Override REQUIRE-MATCH as needed.
  (setq require-match (case icicle-require-match-flag
                        ((nil) require-match)
                        (no-match-required nil)
                        (partial-match-ok t)
                        (full-match-required 'full-match-required))
        icicle-require-match-p require-match)

Here, the intent of the first `setq' pair is to override the REQUIRE-MATCH
argument, based on the value of `icicle-require-match-flag', which is bound
in some commands, but is generally nil. This piece of code is common to both
`completing-read' and `read-file-name'; other pieces are specific to one or
the other (or to other such minibuffer functions, such as
`read-from-minibuffer').

Do you have a suggestion how or where better to do that? It cannot be done
in `minibuffer-setup-hook', because it redefines the value of a
`completing-read' argument.

The intention of the second `setq' pair is not commented, but it is simply
to record the REQUIRE-MATCH argument, so that it can be tested elsewhere.
One place that information is used is in `icicle-narrow-candidates', which
is bound to `M-*' and which you use for progressive completion, to provide a
new match pattern. That command launches a new `completing-read' or a new
`read-file-name' with appropriate arguments according to the current
context, and the appropriate value to pass for REQUIRE-MATCH is the same
value that was used originally. There are currently global variables such as
`minibuffer-completion-table', but AFAIK there is no variable for the
current REQUIRE-MATCH parameter. So, here's a case where, if we added such a
global variable to Emacs, this bit of code in the redefined
`completing-read' would go away.

Again, I agree that it would be worthwhile to examine the code in such
detail, with an eye to seeing what might in fact be needed in Emacs
generally (such as a `minibuffer-require-match' variable, perhaps), and also
with an eye to seeing how I might better do some things elsewhere and
differently than by redefining `completing-read'. I just don't expect that
there is one simple solution for all such stuff, and I don't think we should
try to do that now, except possibly for something easy and obvious.

Similarly, for the other code pieces. We could examine them one by one, but
I'm not sure what that would gain us. In the end, I suspect that there will
be a fair amount of redefinition that is needed, whatever else is tried.

(I didn't mention it, as I thought it was obvious, but the reason I change
the behavior of `completing-read' etc. is so that any code that uses such
functions can automatically take advantage of Icicles features.)

Some of the function redefinitions could perhaps be migrated to the general
Emacs code, as I mentioned earlier. I also mentioned that I think that
should not be attempted now, but after leaving it only in Icicles for a
while.

An example of this would be the Icicles redefinition of `read-face-name'.
The only change Icicles makes here is to display the face names in
*Completions* using their own faces. That is an obvious candidate for
inclusion in vanilla Emacs, IMO. This is the kind of thing that you guys
would generally do with a patch, but from a third-party library perspective,
I needed to do it by redefining (or advising) `read-face-name' temporarily
for the minor mode.