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: Mon, 29 Aug 2022 18:45:31 -0500 Message-ID: <87ilma397o.fsf@red-bean.com> References: <8735dt5si6.fsf@red-bean.com> 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="28015"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: Drew Adams , "emacs-devel@gnu.org" To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Aug 30 01:49:23 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 1oSoVO-00075B-8J for ged-emacs-devel@m.gmane-mx.org; Tue, 30 Aug 2022 01:49:22 +0200 Original-Received: from localhost ([::1]:50758 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oSoVM-00079W-SJ for ged-emacs-devel@m.gmane-mx.org; Mon, 29 Aug 2022 19:49:20 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:48956) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oSoRt-0006Hv-H9 for emacs-devel@gnu.org; Mon, 29 Aug 2022 19:45:45 -0400 Original-Received: from sanpietro.red-bean.com ([45.79.25.59]:60788) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oSoRr-000499-Bi for emacs-devel@gnu.org; Mon, 29 Aug 2022 19:45:45 -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=/dZ9AMAxNlH9llJdTqZxm+SZrKGncF9TyjtG3r/vanE=; t=1661816739; x=1663026339; b=DCGwoOmlElQCAUJRBU35OU7/BoMTCRL9RHdKAQS+f6KbArfDKJC75mm5i7zNggXafNHEZVVu2er 6p2sg4jOD2C0RN0uYyiRaPsTu+odwLTp7CJKTnFry9faocMH1kqooOSXkyEdZm5q5yVufAWgWKLVG lWKXcEmIE/vUO9ziIJgAbjri4HA7TnHZf6uEow651+Z0iyD9UceIp6TmASfSjHIZAKwljSgMjKCJW uGn7wazCvhIoCApLVrmWAwzLSHeReDoPeuxmSFoOl6U2riApjnVSqzNyfSuXlkSIMK/3Fmf/s279l rIUaB9U3WQfLMHrxTh37V5+tZl4Hxw6o2ppA==; Original-Received: from [107.119.44.1] (port=22143 helo=klen) 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 1oSoRg-000GTo-Pm; Mon, 29 Aug 2022 23:45:32 +0000 In-Reply-To: (Stefan Monnier's message of "Mon, 29 Aug 2022 11:28:37 -0400") 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:294318 Archived-At: On 29 Aug 2022, Stefan Monnier wrote: >> 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. > > Indeed :-( > >> 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? > > Hmm... I must be missing something. Why can't > `bookmark--jump-via` > let-bind `bookmark-jump-display-function` around the call to > `bookmark-handle-bookmark`? Of course -- obvious, now that you point it out. Thank you for reviewing, Stefan. Drew, does that sound good to you? > Regarding the patch, here are some comments: > >> + To display the destination, HANDLER can call the function >> that's the >> + value of variable `bookmark-jump-display-function', which is >> set by >> + `bookmark-jump' to automatically accommodate other-window >> etc. >> + displaying that depends on the jump command. For example: >> + >> + (funcall bookmark-jump-display-function (current-buffer)) >> + >> + Or HANDLER can directly call another display function. For >> example: >> + >> + (switch-to-buffer-other-tab BUFFER) > > I skipped most of the rest of the docstring's massaging, which > I'm not > sure is an improvement but with which I guess I can live. But > the above > goes significantly beyond what one usually expects from a > docstring. > It would fit much better in a Texinfo manual instead. > >> + Or HANDLER can invoke `bookmark-default-handler'. That >> displays the >> + destination. It then moves to the recorded buffer position, >> POS, >> + repositioning point, if necessary, to match the recorded >> context >> + strings STR-BEFORE-POS and STR-AFTER-POS. For instance, the >> Info >> + handler, `Info-bookmark-jump', does this at its end: > > And this is (or should be) redundant with the docstring of > `bookmark-default-handler` so better keep the link and the the > readers > jump to it when they need/want to know what that function does. Agreed. >> +(defvar bookmark-jump-display-function nil >> + "Function used currently to display a bookmark, or nil if no >> function.") > > Please give it a function as default value (e.g. `ignore`). I think I understand: by taking your advice, we don't have to check the function's existence -- we can just call it unconditionally, and if it happens to do nothing (i.e., is `ignore'), that's fine. A specific package may still override the default handler and do whatever display magic is called for; otherwise, bookmark.el's default handler will Do The Right Thing for displaying. But all this raises the question you raise below... >> @@ -1350,6 +1391,9 @@ >> ((and buf (get-buffer buf))) >> (t ;; If not, raise error. >> (signal 'bookmark-error-no-filename (list 'stringp >> file))))) >> + (when bookmark-jump-display-function >> + (save-current-buffer (funcall >> bookmark-jump-display-function >> + (current-buffer)))) > > Why `save-current-buffer`? > >> (if place (goto-char place)) >> ;; Go searching forward first. Then, if forward-str >> exists and >> ;; was found in the file, we can search backward for >> behind-str. > > Hmm... `bookmark-default-handler` is also called via things like > `bookmark-insert`, so I think `bookmark-insert` should > explicitly bind > `bookmark-jump-display-function` to `ignore`, and of course that > begs > the question of what to do for handlers which don't obey > `bookmark-jump-display-function`. > > I think it's good to provide more control to the bookmark's > handler, but > there seems to be a need for the caller to control the display > as well > to some extent. ...which is that we now seem to have two possibly-redundant ways to control displaying: write a custom handler, *or* bind `bookmark-jump-display-function' to something? Or perhaps I'm misunderstanding something. Drew, if you post the next revision of the patch incorporating Stefan's points above, I will review that in detail, when fully alert, and should be able to distill any remaining questions -- I think we'd be pretty close at that point. > BTW, your generalization of bookmarks to "anything" makes it all > the > more obvious to me that these should be unified with "registers" > (not > necessarily at the UI level, but a register should be able to > hold > anything that can stored as a bookmark, and vice versa). I agree that that makes sense as a long term goal. Best regards, -Karl