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: Tue, 11 May 2021 11:00:41 +0200 Message-ID: <87y2clve4m.fsf@gmail.com> References: <87zgx5cz33.fsf@gmail.com> <878s4n4wn8.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="25515"; 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 Tue May 11 11:27:11 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 1lgOfW-0006Q8-Mn for ged-emacs-devel@m.gmane-mx.org; Tue, 11 May 2021 11:27:10 +0200 Original-Received: from localhost ([::1]:35350 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lgOfV-0001C9-FI for ged-emacs-devel@m.gmane-mx.org; Tue, 11 May 2021 05:27:09 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:44370) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lgOG1-0002wN-Sd for emacs-devel@gnu.org; Tue, 11 May 2021 05:00:50 -0400 Original-Received: from mail-ej1-x636.google.com ([2a00:1450:4864:20::636]:45715) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lgOFy-0001T9-Uk for emacs-devel@gnu.org; Tue, 11 May 2021 05:00:49 -0400 Original-Received: by mail-ej1-x636.google.com with SMTP id c22so518656ejd.12 for ; Tue, 11 May 2021 02:00:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:message-id:user-agent :mime-version; bh=xhL3ErIhEpG2qyOATB8uV0j+lccS0EPpeOCxpUk036U=; b=hAFCjllI4ZPu/bBN5i1/kkNZkyGY7fwP0nEblaZKzoRv5yCEcA4cOXx4xifFEnJTEl 6XeDptibL/BUb++3I2Z07hXPJK7bgew5EfRnUGMFGkpeJIy/pm3vQ+Avqo/KHQFaB393 aLt/Fq5Zz8mppP4gUnQhwfzQxu32zqc4r/jwmKaO8Igwbwl1lIW8D85dtByguC972ToN 8jVTf7SFmoUvK5F54zoyXa2y1a93L3y52ZBw5MadEU866Nr0srS7TWcUyjQrqPHbswzJ fFzPPUguxLpNPG4TtcHqVotWWfQEqQhuUVzTfiyKnWkV2VrXmimPeKaz4EPWMzActqWB 8DHQ== 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:message-id :user-agent:mime-version; bh=xhL3ErIhEpG2qyOATB8uV0j+lccS0EPpeOCxpUk036U=; b=b/ruCgMamqsIWfx3TWwCrPIdbhEDcP8ZG7PiEZh6ZYz/cPci6+WqOeUUtLpRzW0GNl 8E+OJRHVyv3YGbBQuKOkOuciRsdLkik9DpL/RJ+9GxWiMURFQfWF67PKlJRhOMIsNOQE QJYqDL/dmdIXrt84taRnvAeB/28VKnXprpkjfgCU46Sf5/6GDONkHkz5j9vj/aeeGpmM 5EZAWi4ASU+m2KinfxSpA+NTriXHLQCx1yF74clpy6UhbKlGQ/Nor3ekFxz2Wi/V7Up1 OGtIfNooNomcmlur0ORiMzUGLbSqwy5TKUYQxcAku6agRlXpTrdjUUjBL9Qcf/4sOnDH pIYw== X-Gm-Message-State: AOAM532V+vDpXZLgaQMs39vrsfaJ1bOgHWig2ncIxRE1K31fmn6n60CR bsYXSOJcQqOGnLzDam5SpXH6j5Hd2Is= X-Google-Smtp-Source: ABdhPJztqI9EYnOjE630xlbNkM8bjcHQXFLAuM9FYeUcfMg5bl827xFxlYlyOZr0hqfGVObgYgxiVw== X-Received: by 2002:a17:906:4d07:: with SMTP id r7mr30454282eju.224.1620723643886; Tue, 11 May 2021 02:00:43 -0700 (PDT) Original-Received: from ars3 ([2a02:908:2211:8540::68a]) by smtp.gmail.com with ESMTPSA id d24sm13934908edp.31.2021.05.11.02.00.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 May 2021 02:00:43 -0700 (PDT) Received-SPF: pass client-ip=2a00:1450:4864:20::636; envelope-from=arstoffel@gmail.com; helo=mail-ej1-x636.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:269151 Archived-At: On Mon, 10 May 2021 at 19:51, Alan Mackenzie wrote: > Hello, Augusto. > > On Sun, May 09, 2021 at 19:58:19 +0200, Augusto Stoffel wrote: >> On Sun, 9 May 2021 at 13:36, Alan Mackenzie wrote: > >> 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. > > I think the commands came together the way they did in early Emacs > history through people trying things out and discarding what didn't work > very well. What we're left with is what worked/works well. > >> But this doesn't mean this feature won't be appreciated by other >> people. > > Neither does it mean they will be. What is the evidence for that? There are alternative search packages out there which are quite popular, even though they lose several interesting features from Isearch. > >> In the minbuffer-controlled Isearch mode, DEL is simply >> `delete-backward-char', not some kind of undo. It deletes in all >> circumstances. > > So, you're taking away one of the core bindings of isearch. What are you > going to put in its place? The ease of pressing DEL to undo the last > subcommand is one of the delights of isearch, from my point of view. I find it easier to press C-r to undo C-s, and vice versa. But that's completely irrelevant. The question is whether it makes sense to provide an alternative UI for Isearch where editing the search string is not a special maneuver. This includes DEL doing what it does everywhere else, but is not limited to that. > > [ .... ] > >> > 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. > > There's nothing "just" about UI things. ;-) > >> As to the duplication of ,@body, sure, once can always use a lambda to >> avoid this. > > That wasn't really what I meant. I was thinking more on the lines of > perhaps assigning the results of the ,@body to a variable, and doing > different things with that variable. Or something like that. > >> 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. > > As would I, provided the pieces are coherent wholes, and not random > fragments. > >> > 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 [isn't] 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. > > That's complicated difficult to follow logic. Could it not be done more > simply with functions calling functions in the standard fashion? The minibuffer starts a recursive edit. There may be ways to simplify my patch, but functions which always return locally won't cut it. > >> I admit the after-change function and post-command hook are hairy, >> because they interact with the Isearch state. > > That looks to me like a source of future bugs. I think you should look at the code before claiming that. The state manipulation I'm adding is very mild compared to what's going on elsewhere in isearch.el. > >> > 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. > > Maybe because DEL does not delete a character, but undoes a subcommand. > >> 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 think one C-g works if the current search string has been found, > otherwise you need two. > >> I would also inadvertently quit the search all the time when trying to >> edit the string. This is too complicated to my taste. > > Personally, I don't see it that way at all. I find isearch delightfully > simple and intuitive. But I can accept you see it differently. > >> 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. > > I didn't realise this till very recently either. But isearch isn't > reading a string - at least not primarily - it's searching through a > buffer. And as Drew pointed out, that buffer might well be the > minibuffer itself. > >> This patch is supposed to provide a simple, intuitive mode of operation >> for Isearch. > > Thanks, but that doesn't really answer my question. You're unhappy about > some of the keybindings in isearch, which is fair enough. But how does > switching to using the minibuffer have anything to do with that? > > My feeling is that if you were to make the (optional) keybinding > changes The patch is about making it easier to edit the search string. Keybindings are easy to customize, and I'm not trying to share my config here. > whilst using the current echo-area manipulation, the patch would be > _much_ less that 500 lines long. The patch has 215 insertions, 57 deletions. A good chunk of that is so one gets lazy highlight and lazy count in the good old M-e. > Why use the minibuffer at all? So that it's natural to edit the search string during a search. > > [ .... ] > >> >> 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. > > Probably. But, again, what has using the minibuffer got to do with the > keybindings a user prefers? As stated in the original message, this feature is all about editing the search string. Having the same keybindings as everywhere is else might be seen as a beneficial side effect by some people, but is incidental.