unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Alan Mackenzie <acm@muc.de>
To: Augusto Stoffel <arstoffel@gmail.com>
Cc: emacs-devel@gnu.org
Subject: Re: [WIP PATCH] Controlling Isearch from the minibuffer
Date: Sun, 9 May 2021 13:36:56 +0000	[thread overview]
Message-ID: <YJfleGwrutzzl13X@ACM> (raw)
In-Reply-To: <87zgx5cz33.fsf@gmail.com>

Hello, Augusto.

On Sat, May 08, 2021 at 12:13:52 +0200, Augusto Stoffel wrote:
> I've attached a draft implementation of a minibuffer-controlled mode for
> Isearch, as described for instance in
> https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
> To enable the feature, set `isearch-from-minibuffer' to t.

> The basic trade-off is that this makes it easier to edit the search
> string, and harder to quit the search.

You mean, the patch makes incompatible changes to isearch, yes?

> It also drops some of the eccentricities of regular Isearch.  For
> instance, DEL deletes and C-g quits.

What to you are eccentricities, are features to other people.  I'm very
satisfied with the current workings of C-g, and would protest vehemently
were they to be changed.  I'm not sure what you mean by "DEL deletes" -
what does it delete, and in which circumstances?

> I'm sharing this preliminary version because two important questions can
> already be answered:

> - Does the approach taken here seem sufficiently robust?  Note in
>   particular the `with-isearch-window' macro, which is now needed around
>   several functions, as well as the somewhat hacky `run-with-idle-timer'
>   call inside the `isearch-mode' function.

> - Are the slightly backwards incompatible keybinding changes in
>   `isearch-edit-string' acceptable?

It depends what you mean by "slightly".  I suspect the answer to the
question is no.  Any changes here which aren't simply additions are
going to disturb somebody's workflow.

> If any of these answers is no, then I would provide a package for the
> same feature.  But I think the feature is interesting enough to be built
> in isearch.el.  Moreover, it would benefit from being official because
> many third-party extensions to Isearch will need to take into account
> the possibility that the search is being controlled remotely from a
> minibuffer.

Skimming through your patch (it is too large for me to take in every
detail at the moment), it seems it could make isearch.el even more
difficult to maintain than it currently is.  For example, the two
defmacros `with-...' are the sort that force somebody debugging (or
trying to read a patch) to look somewhere else to find out what they
mean.  This is irksome and tedious.  There are already macros like this
in isearch.el, and it would seem wise to minimise any further
proliferation.  Each of these macros expands the ,@body twice for every
invocation.  Would it not be possible to rearrange things, just to
expand them once?

In one hunk in the patch, I see a condition-case containing a funcall,
containing a catch, containing a minibuffer-with-setup-hook, which in
its turn contains a lambda function and an unwind-protect.  The lambda
function sets functions onto both after-change-functions and
post-command-hook.  Whew!  Each one of these constructs on its own
causes difficulty in debugging, but they have their justification.  Must
they really all appear together in this fashion?

The biggest question, which may have been answered somewhere in the
thread already, is why?  What is wrong with isearch at the moment that
the patch will fix, or enable to be fixed?  The emacs-devel post you
cite above (from 2020) doesn't seem to motivate this change.  Could you
possibly answer this question, or cite a post which answers it, please?

> Some further remarks:

> - The minibuffer-controlled mode is supposed to depend on the proposed
>   `isearch-buffer-local' feature.  This will make the hack used to
>   deactivate the `overriding-terminal-local-map' unnecessary.

> - It seems necessary to let-bind `inhibit-redisplay' to nil in
>   `with-isearch-window' in order to avoid flicker in the cursor.  This
>   seems related to the recent thread "Temporarily select-window, without
>   updating mode-line face and cursor fill?" in this list.  Any better
>   solutions?

> - I don't like the `with-isearch-window-quitting-edit' macro, but I
>   don't see a different way of achieving the necessary effect.

> - I don't use/know of all Isearch features, so let me know if you spot
>   some incompatibility.

> What do you think?

The patch is ~500 lines long.  This makes me worried.  I'm afraid I can
only react negatively and defensively at the moment.

[ patch snipped ].

-- 
Alan Mackenzie (Nuremberg, Germany).



  reply	other threads:[~2021-05-09 13:36 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-08 10:13 [WIP PATCH] Controlling Isearch from the minibuffer Augusto Stoffel
2021-05-09 13:36 ` Alan Mackenzie [this message]
2021-05-09 17:58   ` Augusto Stoffel
2021-05-10 19:51     ` Alan Mackenzie
2021-05-11  9:00       ` Augusto Stoffel
2021-05-11 15:34         ` [External] : " Drew Adams
2021-05-11 18:31           ` Juri Linkov
2021-05-11 19:38             ` Drew Adams
2021-05-12  6:45           ` Augusto Stoffel
2021-05-12 12:44             ` Stefan Monnier
2021-05-12 15:31               ` Drew Adams
2021-05-12 22:17                 ` Kévin Le Gouguec
2021-05-12 23:07                   ` Drew Adams
2021-05-13 15:12                     ` Kévin Le Gouguec
2021-05-12 21:09               ` Augusto Stoffel
2021-05-12 15:30             ` Drew Adams
2021-05-09 19:09   ` Juri Linkov
2021-05-09 19:05 ` Juri Linkov
2021-05-10 20:24   ` Augusto Stoffel
2021-05-10 21:17     ` Juri Linkov
2021-05-12  6:40       ` Augusto Stoffel
2021-05-12 17:13         ` Juri Linkov
2021-05-12 20:52           ` Augusto Stoffel
2021-05-13 16:31             ` Juri Linkov
2021-05-13 20:12               ` [ELPA?] " Augusto Stoffel
2021-05-14  1:17                 ` Jean Louis
2021-05-14  8:36                   ` Augusto Stoffel
2021-05-14 17:30                 ` Augusto Stoffel
2021-05-14 18:20                   ` Juri Linkov
2021-05-16 11:00                     ` Augusto Stoffel
2021-05-16 18:19                       ` Juri Linkov
2021-05-25 20:50                         ` Juri Linkov
2021-05-29 11:48                           ` Augusto Stoffel
2021-05-14 18:18                 ` Juri Linkov
2021-05-16 18:12                   ` Juri Linkov
2021-05-16 18:49                     ` Augusto Stoffel
2021-05-21  9:09                       ` Augusto Stoffel
2021-05-21 10:25                         ` Eli Zaretskii
2021-05-21 11:56                           ` Augusto Stoffel
2021-05-21 12:31                             ` Eli Zaretskii
2021-05-21 12:49                               ` Augusto Stoffel
2021-05-21 15:05                               ` Stefan Monnier
2021-05-21 15:09                                 ` Eli Zaretskii

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJfleGwrutzzl13X@ACM \
    --to=acm@muc.de \
    --cc=arstoffel@gmail.com \
    --cc=emacs-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).