From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Karl Fogel Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Let bookmark handlers do everything, including display the destination Date: Thu, 18 Aug 2022 13:14:09 -0500 Message-ID: <8735dt5si6.fsf@red-bean.com> References: Reply-To: Karl Fogel Mime-Version: 1.0 Content-Type: text/plain; format=flowed Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="15555"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: "emacs-devel@gnu.org" To: Drew Adams Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Thu Aug 18 20:16:24 2022 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 1oOk46-0003sp-Hs for ged-emacs-devel@m.gmane-mx.org; Thu, 18 Aug 2022 20:16:22 +0200 Original-Received: from localhost ([::1]:53858 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oOk45-0004o7-3A for ged-emacs-devel@m.gmane-mx.org; Thu, 18 Aug 2022 14:16:21 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:41168) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oOk26-0003Iv-04 for emacs-devel@gnu.org; Thu, 18 Aug 2022 14:14:18 -0400 Original-Received: from sanpietro.red-bean.com ([45.79.25.59]:40376) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oOk23-0007bm-IG for emacs-devel@gnu.org; Thu, 18 Aug 2022 14:14:17 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=red-bean.com; s=202005newsp; h=Content-Type:MIME-Version:Message-ID:Date: Reply-To:References:In-Reply-To:Subject:Cc:To:From:Sender: Content-Transfer-Encoding:Content-ID:Content-Description; bh=GqPmbOrVqNuZJStvzCwCG5HP5TQqmRNs+5lHSx3ICxo=; t=1660846453; x=1662056053; b=T8Bkx2zo6DVn0j1YVVx8Dr3HgsQA5kS1kEiF+2E2dqiEs4vbQpmsOyTDEBLbMbyQUfcPh6sCphC PJMLcwtxAQfcuXBvfN06OZtLBvG85Nw3dELn/8KKePrjWcJh7mcseaTFJC3dcYejn6Z/OSSve0hIu W2T1ey4np567ldnrlcQcqGWHAkkBegSYBFxgXNR/D9noLJ+hCIuab267EitYuZZ2Bypij1JGP/rKL m3lK+kWsbNH7cUUpcDCAw+TYo8WlvsJ85qNGi92ZIVBrebisuFzT3RQ86YCST/GXyitC8dLkdRU0I ttMHknZapigop6SDnkk9LLlpgnOHDy6bbmqg==; Original-Received: from [2600:1700:5654:2110:3818:6b31:20b:2bed] (port=33476 helo=tcd) by sanpietro.red-bean.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1oOk1x-00DGeo-V8; Thu, 18 Aug 2022 18:14:10 +0000 In-Reply-To: (Drew Adams's message of "Sat, 13 Aug 2022 20:59:20 +0000") Received-SPF: pass client-ip=45.79.25.59; envelope-from=kfogel@red-bean.com; helo=sanpietro.red-bean.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, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 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:293617 Archived-At: On 13 Aug 2022, Drew Adams wrote: >tl;dr: > >Let's change the handling of a bookmark - including >displaying the bookmark destination - to be the >responsibility of its handler. In general, +1 to this idea. (My ability to be involved in actually making it happen may be limited right now, due to some family responsibilities. I'll try to at least kibbitz constructively though.) >Attached is a patch that does that. It moves the displaying >of a bookmark's "destination" from `bookmark-jump' to the >bookmark itself - specifically, to its handler. > >Currently `bookmark-jump', rather than a bookmark's handler, >displays the bookmark destination. And the behavior's >hard-coded: a bookmark always displays a destination, and >that's always the last thing done before the after-jump >hook. > >That's too rigid. It means that a handler function can't do >something more after the display, unless it finagles to put >that after-display processing on `bookmark-after-jump-hook' >(temporarily). And it means that a handler can't choose its >own display function - the DISPLAY-FUNCTION passed to >`bookmark-jump' is always used. And it means that a handler >can't dispense with display altogether. > >By moving the responsibility for display to the handler, the >patch gets rid of this rigidity. > >Specifically, instead of `bookmark--jump-via' systematically >invoking its DISPLAY-FUNCTION argument, it saves the >argument value in variable `bookmark-jump-display-function'. >And then the default handler, `bookmark-default-handler' >invokes that function. *nod* There's some parallelism-unsafety going on here, obviously. I'd love a way to wrap all this in a clean closure, instead of setting a global variable which then retains that setting until the next time we happen to set it. But I don't really see a way to do tihngs more cleanly than with the carry-out variable you propose. If there's some creative solution for connecting `bookmark--jump-via' to the "corresponding" handler call that happens later, I don't see it, lexically or dynamically. Maybe is just how Emacs usually handles these kinds of situations? >Most bookmarks don't have their own handler; they use the >default handler. So their behavior doesn't change at all >with this approach. Most bookmarks use `bookmark-jump's >DISPLAY FUNCTION as the last thing before running >`bookmark-after-jump-hook' (and most should do that). > >The same applies to bookmarks that do have their own >handler, if the handler invokes the default handler at the >end. For example, the Info bookmark handler, >`Info-bookmark-jump' sets up what it needs, and ends by >invoking the default handler: > > (bookmark-default-handler > `("" (buffer . ,buf) > . ,(bookmark-get-bookmark-record bookmark))) > >What changes with the proposed approach is only what can >happen for a bookmark that has its own handler, if that >handler either (1) invokes the default handler at some point >other than at the end - so it does something after >displaying, or (2) it doesn't invoke the default handler. > >If a handler doesn't invoke the default handler, it can call >whatever display function it likes, whenever it likes. Or >it can forego display altogether. > >This approach gives the handler more control - it can decide >whether, when, and how to display the destination. The >entire behavior of a bookmark is then up to its handler. > >And this allows for more kinds of bookmarks, including >bookmarks that don't display a destination. > >[...some example usages from Bookmark+...] This is all very well-explained; thank you, Drew. >[...] > >But really, the complications `pdf-view-bookmark-handler' >goes through - creating a closure on the fly and adding it >to the after-jump hook (and subsequently removing it) - are >a workaround for the fact that vanilla `bookmark.el' doesn't >let a handler display the destination when it wants to. > >PDF Tools wants to set up the buffer, display it, and then >go to the right PDF page etc. So it sets up the buffer, >ends the handler, counts on `bookmark-jump' to display the >buffer, and then has a (temporary) after-jump hook do the >rest of the handling: go to the right page etc. (The hook >function captures some info from the bookmark, so it needs >to be a closure.) A clever workaround. > >With the approach provided by the patch, the handler can do >all of that itself: set up the buffer, display it, then go >to the page etc. No after-jump-hook workaround. The code >is quite a bit simpler. Yup; that makes sense to me. >Between setting up the buffer and doing the additional stuff >that was relegated to a temporary after-jump hook, the >handler can either (1) just invoke the default handler or >(2) just call a display function. Code for each of those >handler definitions is also attached, for comparison with >the above code. > >Those solutions are examples #5 and #6, respectively, in >attachment `pdf-tools-handler-fix.el'. > >I mention the PDF Tools handler as an example. I'm not >aware of other handlers that depend on `bookmark-jump' >displaying some buffer and selecting its window, and then >depend on the after-jump hook for additional handling in >that buffer, but there may be some out there. > >This is a general problem for Bookmark+, even if it seems to >be run into rarely because most handlers invoke the default >handler (which in Bookmark+ displays the destination using >the DISPLAY-FUNCTION passed to `bookmark-jump'). > >But this is not just about getting rid of an incompatibility >with Bookmark+'s bookmark handling. It's also about >improving vanilla `bookmark.el', by letting the handler >handle a bookmark's behavior - all of it. Give handlers the >control over display, and have the default handler invoke >the `bookmark-jump' DISPLAY-FUNCTION arg. My main concern here is just that compatibility issue. I have no idea how many bookmark-using packages will break if we install this patch. You've explained very clearly how to fix them, and how in many cases their code is likely to get cleaner as a result, and I agree. But still, compatibility-breaking changes are a serious thing. I'd like to know what others here think about this. Your suggested guidelines below seem good *if* we decide to make this change: >That makes possible bookmarks that don't necessarily display >a destination - they perform some other action. And it >allows bookmarks to do any kind of displaying whatsoever >(not just what the `bookmark-jump' DISPLAY-FUNCTION >provides). > >Regardless of whether we adopt the proposed behavior, we >might anyway want to post a guideline to let people who >define custom bookmark handlers know that it's >common/typical for a handler to invoke the default handler - >that repositions the bookmark as needed etc. > >But with the new behavior we should also make clear that for >a bookmark destination to be displayed, a custom handler >needs to invoke either `bookmark-default-handler' or a >display function. > >More generally, the thing to make clear is that a handler >completely defines a bookmark's behavior. And in >particular, it decides whether, when, and how to display a >bookmark destination. Yup. >I think the proposed approach (attached patch or similar) >would affect very few people. It would affect only library >authors who define custom bookmark handlers (few do) that >don't invoke the default handler (fewer still), AND whose >code relegates some of the handling to the after-jump hook >and depends on `bookmark-jump' invoking the DISPLAY-FUNCTION >(very few, I expect). I expect you are right. I'm even inclined to risk the breakage and help callers fix it, just to get this long-term structural improvement. But I'm not confident in this opinion, and would like to know what others here, especially the maintainers, think. I reviewed the bookmark.el patch briefly, and overall it had the shape I expected. I might have a few suggestions about doc strings and such; if we decide to go down this road, I'll do a real patch review. Best regards, -Karl