From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#17453: Isearch doesn't work properly with Follow Mode. Date: Sun, 11 May 2014 18:18:32 +0000 Message-ID: <20140511181832.GA2759@acm.acm> References: <20140509224458.GA4205@acm.acm> <20140511125842.GA11781@acm.acm> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1399832611 9515 80.91.229.3 (11 May 2014 18:23:31 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 11 May 2014 18:23:31 +0000 (UTC) Cc: 17453@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun May 11 20:23:22 2014 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1WjYPB-00041f-TR for geb-bug-gnu-emacs@m.gmane.org; Sun, 11 May 2014 20:23:22 +0200 Original-Received: from localhost ([::1]:33945 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WjYPB-0004sE-HT for geb-bug-gnu-emacs@m.gmane.org; Sun, 11 May 2014 14:23:21 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:56677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WjYP1-0004ls-7r for bug-gnu-emacs@gnu.org; Sun, 11 May 2014 14:23:18 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WjYOt-0007Xb-2P for bug-gnu-emacs@gnu.org; Sun, 11 May 2014 14:23:11 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:42208) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WjYOs-0007XX-Ve for bug-gnu-emacs@gnu.org; Sun, 11 May 2014 14:23:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1WjYOs-0002zh-Gq for bug-gnu-emacs@gnu.org; Sun, 11 May 2014 14:23:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 11 May 2014 18:23:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 17453 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 17453-submit@debbugs.gnu.org id=B17453.139983256311464 (code B ref 17453); Sun, 11 May 2014 18:23:02 +0000 Original-Received: (at 17453) by debbugs.gnu.org; 11 May 2014 18:22:43 +0000 Original-Received: from localhost ([127.0.0.1]:59558 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WjYOY-0002yp-NQ for submit@debbugs.gnu.org; Sun, 11 May 2014 14:22:43 -0400 Original-Received: from colin.muc.de ([193.149.48.1]:61411 helo=mail.muc.de) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1WjYOV-0002yf-H8 for 17453@debbugs.gnu.org; Sun, 11 May 2014 14:22:40 -0400 Original-Received: (qmail 98238 invoked by uid 3782); 11 May 2014 18:22:38 -0000 Original-Received: from acm.muc.de (pD951AB04.dip0.t-ipconnect.de [217.81.171.4]) by colin.muc.de (tmda-ofmipd) with ESMTP; Sun, 11 May 2014 20:22:37 +0200 Original-Received: (qmail 3373 invoked by uid 1000); 11 May 2014 18:18:32 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Delivery-Agent: TMDA/1.1.12 (Macallan) X-Primary-Address: acm@muc.de X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:88920 Archived-At: Hello, Stefan. On Sun, May 11, 2014 at 12:09:38PM -0400, Stefan Monnier wrote: > > Follow Mode knows nothing of Isearch. > follow-mode.el doesn't, indeed, but since you're moving some follow mode > code to isearch.el, .... Am I? No code that currently is in follow-mode.el will cease to be in follow-mode.el. > .... that means that follow mode (which is now spread > over follow-mode.el and isearch.el) knows something about Isearch. No, Follow Mode will know nothing about Isearch. Isearch will know about follow-mode's internal structures, namely its list of windows, and what their sequencing means. In that sense, the two libraries are coupled, which isn't, other things being equal, good, but I can't see how to avoid it. Can you see a way of avoiding this coupling? > > The problem the other way round is in Lisp files that themselves play > > with redisplay (including calling `sit-for') and `set-window-start', > > and so on. Between these invocations and the actual redisplay, Follow > > Mode must get a chance to make its adjustments. > There's no doubt that follow mode's task is a tricky one, and I'm > willing to add special support for it. I just want this special support > to be a bit more generic than what you provided. I'm having trouble discerning WHAT, specifically, should be made more generic (with the exception of the two functions discussed below). Could you talk more in specifics, please. > It shouldn't be too difficult. It's just a matter of refactoring: > change your patch so that on isearch.el's side it only adds some hooks, > which are then set follow-mode.el. Do you mean "set to a function in follow-mode.el". I think you're suggesting writing more functions in follow-mode.el. Do you mean something like a Follow Mode equivalent of `window-start'? > >> IOW we should try harder to come up with more general hooks. > > For what? > So that the same hooks can be used by other code than Isearch, for example. I know the motivation for the change. For what functionalities should code be come up with that will be incorporated into these hooks? What will the hooks do, specifically? > > Maybe a useful hook here would be `redisplay-hook' where Follow Mode > > could do its stuff more effectively than on `post-command-hook'. > There's pre-redisplay-function, which I think it does get us closer but > is not sufficient yet. And of course, it won't help with things > like "bring-region-into-sight". I didn't know about pre-redisplay-function. It looks new. As an aside, its documentation is unclear (what is a "set" of windows, for example, and what sort of things are allowed/not allowed in a hook function (deleting windows, for example)?) Is it for "bring-region-into-sight" and friends that you envisage adding hooks? > >> > @@ -2207,10 +2239,12 @@ > >> > together with as much of the search string as will fit; the symbol > >> > `above' if we need to scroll the text downwards; the symbol `below', > >> > if upwards." > >> > - (let ((w-start (window-start)) > >> > - (w-end (window-end nil t)) > >> > - (w-L1 (save-excursion (move-to-window-line 1) (point))) > >> > - (w-L-1 (save-excursion (move-to-window-line -1) (point))) > >> > + (let ((w-start (isearch-windows-start)) > >> > + (w-end (isearch-windows-end nil t)) > >> > + (w-L1 (with-selected-window (car isearch-windows) > >> > + (save-excursion (move-to-window-line 1) (point)))) > >> > + (w-L-1 (with-selected-window (car (last isearch-windows)) > >> > + (save-excursion (move-to-window-line -1) (point)))) > >> This isearch-string-out-of-window+isearch-back-into-window business, for > >> example should be generalized to a function along the lines of > >> "bring-region-into-sight". It's not useful only for isearch. > > This seems to be a different bug to the one I reported, and orthogonal > > to it. > What bug? I'm just pointing out that the code you're modifying is more > generally useful and that this generality is a good guide to help us > decide where to put needed hooks. I meant that forming a more general subroutine out of isearch-back-into-window is not properly part of bug #17453. It is a separate exercise, which can be done independently of #17453. I'm not sure how well a hook would work for this functionality, since in the "plain" hook function you'd want to pass it a window, whereas in the "follow" hook function you'd want to pass it a list of windows. > >> E.g. ediff-next-hunk would also benefit from it. > ^^^^^ > I meant "diff", sorry. Tho it would also be useful to ediff and smerge-mode > as well, indeed. OK. So you're thinking of somewhere like subr.el, with something like (defun scroll-region-into-window/s (start end window/s above opposite-important) ....) where WINDOW/S is either a single window or a list of them, START END bound the region, OPPOSITE-IMPORTANT is t when the region boundary "nearest the window" is to be preferred for display when the region's too big. Or something like that. Maybe we could add a parameter for desired margin at the top/bottom of the window. > > Not necessarily. isearch-back-into-window, in addition to doing > > "bring-region-into-sight" ensures that it's the isearch-point end of it > > which is visible when it is too big for the window. This may be > > problematic for other uses, like in ediff. > I doubt this will be problematic. It seems quite natural for > "bring-region-into-sight" to take as arguments not just the region but > also some indication of the part to favor in case it can't all > be displayed. OK. Formulating a good description of that parameter is tricky, though. > > Question is, what about the main bug? The patch I wrote fixes a real > > bug, and works (modulo any remaining bugs). Do you have any concrete > > suggestions as to how to improve the fix? > Hmm... I'm sorry I got lost along the way. I thought the whole patch is > the bug fix (and the bring-region-into-sight part does seem like > a bug-fix as well, tho maybe of top priority, admittedly). > Could you show which part of the patch addresses the main bug? The whole patch addresses the main bug; forming generally useful subroutines out of isearch-string-out-of-window and friends is the other bug, which I'm suggesting be dealt with separately. > Stefan -- Alan Mackenzie (Nuremberg, Germany).