From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Augusto Stoffel Newsgroups: gmane.emacs.devel Subject: Re: [WIP PATCH] Controlling Isearch from the minibuffer Date: Sun, 09 May 2021 19:58:19 +0200 Message-ID: <878s4n4wn8.fsf@gmail.com> References: <87zgx5cz33.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="19246"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) Cc: emacs-devel@gnu.org To: Alan Mackenzie Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun May 09 19:59:14 2021 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lfnhx-0004tA-US for ged-emacs-devel@m.gmane-mx.org; Sun, 09 May 2021 19:59:14 +0200 Original-Received: from localhost ([::1]:44390 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lfnhw-0006ca-HL for ged-emacs-devel@m.gmane-mx.org; Sun, 09 May 2021 13:59:12 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:39928) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lfnhC-0005wk-ON for emacs-devel@gnu.org; Sun, 09 May 2021 13:58:26 -0400 Original-Received: from mail-ej1-x633.google.com ([2a00:1450:4864:20::633]:40481) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lfnhA-0004Aa-Hc for emacs-devel@gnu.org; Sun, 09 May 2021 13:58:26 -0400 Original-Received: by mail-ej1-x633.google.com with SMTP id n2so21084181ejy.7 for ; Sun, 09 May 2021 10:58:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=KRwaOlzUaQQqZDSep0GUKvaQYkZDxbJ6XvZjfE0EC/U=; b=o06XKcTIfCfb6taDuPBoCNhANEGwpiUXhPCO+QRaIhRFg+NHtqO4jtATFk/pNx+RJc yf6FdGzt2n4MWFX38tNB1rXQecm6MtW8jUoh3DH5XleiYeleep4bnoFTyGevOYY9lyON hBpwlb9Ga7k+WkfNsryhHfG1Kfb8M/ih17oooGVoce+lDGAFHuHOUJFA/acBz5V3khFx bNKLYXNE0XT9/EYP8KS7L7EchM6wNxlqSWTQGxFuj+9gN9+yZnW27Hu2WMMjwSLgiy2q 2BeFCBTENsmINRyuWcVRE5tM0UbECD2ck7TVto2SoIKDZ60bzKJdlDCHip3IN2w1wRWV d+uA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=KRwaOlzUaQQqZDSep0GUKvaQYkZDxbJ6XvZjfE0EC/U=; b=iE5xuowsCfE3/OZCenkbxn1ajIZEtyUdFXJm2jSu//9brwPi0NRPOXJuEmSndNU/n5 EsqYr4SZnIDffKU3IWov6KFEMidiUfxOZae1xtkDEopo3QQ6Gh5UQNfzgWTP4V/1JVnu yijJfS2s2KO7K3NOsfP8dC+/y6YecudZ2r38+/D17euvDzepBgrmxtkS8jE1CbKFcMfZ a/UjYVOVE59tBO/3g1U+GC7KLilzLn1xuxrRewu09eMlS/HvX3z6+PzUnk4Voy6avD6s +S8yttCWCBMTJUTxUQTFvH8mR4vD7GvHB2cpLM/hsmA8E+QN1VpvCT4nvpxR5MbSuYCs 4DMg== X-Gm-Message-State: AOAM530dEWywBosGj/cmUUKhPvKGGi8287AeOB24/pRlR2KU9waEo+D8 3gwdGuM42cl0lZUB/OnDiCvEg/T/Mac= X-Google-Smtp-Source: ABdhPJzh4kFv3BxIzjWDGlltTbUdKHYSEX1nnb7DL/01JAY+KJPLUkCwG/AhD3pJspKE07rccs2rDg== X-Received: by 2002:a17:906:b748:: with SMTP id fx8mr17122531ejb.223.1620583102152; Sun, 09 May 2021 10:58:22 -0700 (PDT) Original-Received: from ars3 ([2a02:908:2211:8540::68a]) by smtp.gmail.com with ESMTPSA id bn5sm2299770ejb.97.2021.05.09.10.58.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 09 May 2021 10:58:21 -0700 (PDT) In-Reply-To: (Alan Mackenzie's message of "Sun, 9 May 2021 13:36:56 +0000") Received-SPF: pass client-ip=2a00:1450:4864:20::633; envelope-from=arstoffel@gmail.com; helo=mail-ej1-x633.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:269084 Archived-At: On Sun, 9 May 2021 at 13:36, Alan Mackenzie wrote: > Hello, Augusto. Hi Alan, > > 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? I wouldn't dare to suggest turning this on by default :-). The *slight* incompatible changes are all in `isearch-edit-string'. This seems like a quite unpolished command anyway, but of course I can leave it untouched. I suggest discussing the details in a future subthread. > >> 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 am sure most of the regulars in this thread share your view. Otherwise, something like this proposal would have surfaced much earlier. But this doesn't mean this feature won't be appreciated by other people. In the minbuffer-controlled Isearch mode, DEL is simply `delete-backward-char', not some kind of undo. It deletes in all 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? My difficulty in understanding Isearch is the very complicated internal state. This patch is just a UI thing and doesn't interact at lot with the search state. As to the duplication of ,@body, sure, once can always use a lambda to avoid this. However, I would rather break up a very long function that requires being wrap in these macros (they will be very few, anyway) into shorter and simpler pieces. > > 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 condition-case complicated, really. The tag in question is thrown at just one place (`with-isearch-window-quitting-edit') and caught at just one place (`isearch-edit-string'). No spaghetti logic here at all. It just passes a continuation, hence the funcall. I admit the after-change function and post-command hook are hairy, because they interact with the Isearch state. > > 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? > I used Isearch for many years and never really understood/cared to figure out why sometimes trying to delete a character jumped around the buffer. Or in which circumstances I had to press C-g once or twice to quit (I still don't, but in the meanwhile I found `isearch-cancel'). I would also inadvertently quit the search all the time when trying to edit the string. This is too complicated to my taste. Only after an interlude using Swiper I realized that Isearch isn't even using the minibuffer, like any other command that reads a string does. This patch is supposed to provide a simple, intuitive mode of operation for Isearch. >> 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. > That's because I didn't reindent anything yet :-) Now seriously, the bulk of the change is to rewrite `isearch-edit-string'. Apart from that, it (1) wraps a bunch of commands in a macro that does nothing when `isearch-from-minibuffer' is nil, and (2) adds a condition at the end of `isearch-mode' to possibly call `isearch-edit-string'. That's about it. Maybe with this short summary the patch becomes easier to digest. > [ patch snipped ].