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: [External] : Re: [PATCH] Let bookmark handlers do everything, including display the destination Date: Mon, 26 Sep 2022 14:47:49 -0500 Message-ID: <87zgellxy2.fsf@red-bean.com> References: <8735dt5si6.fsf@red-bean.com> <87ilma397o.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="33506"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: Stefan Monnier , "emacs-devel@gnu.org" To: Drew Adams Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Sep 26 21:50:11 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 1ocu7D-0008P5-BA for ged-emacs-devel@m.gmane-mx.org; Mon, 26 Sep 2022 21:50:07 +0200 Original-Received: from localhost ([::1]:35678 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ocu7B-0007YL-Pi for ged-emacs-devel@m.gmane-mx.org; Mon, 26 Sep 2022 15:50:05 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:40608) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ocu5B-00065M-Hh for emacs-devel@gnu.org; Mon, 26 Sep 2022 15:48:01 -0400 Original-Received: from sanpietro.red-bean.com ([45.79.25.59]:34600) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ocu58-0005GW-FE for emacs-devel@gnu.org; Mon, 26 Sep 2022 15:48:01 -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=N8WPzyPKgCeczIdWACdSZMlA3UnNzt0TkPPiSh5rmDw=; t=1664221674; x=1665431274; b=imYgol2Xi0eP258dbIXnKXGQS3UqVX3S+clWTTIEWJCNs0sfM83bW/+OM6ChWYTzOnd1O2g0Iai Ds8ZU+kTJ+GKeA9+Q/s1lZrfhSOi5wZXCpoieWyKUSrT0Fm0PVky5o1ROTyCKQrgjavn+kqhMkua5 tWi1ILsL4JiLfAkDTaTyv7iJGzG7tmzg93YXC2C2WQV0VdGP8vt7Z9GXOHYBhDQTlLjlFoW4cMC2Z 51BIJsHnW3I0U6dSbIUy2hIenntOfwqxD03BAaFmIjs+Y8QDjY1aBv/R47ThfdYzGbDr2gbnqNJHL IqoAlN1TBfWkl1UJL66pTeWItCe2Gq+6k88A==; Original-Received: from [12.106.183.66] (port=28163 helo=hummy) 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 1ocu4z-0025KK-Tq; Mon, 26 Sep 2022 19:47:49 +0000 In-Reply-To: (Drew Adams's message of "Tue, 6 Sep 2022 00:07:04 +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 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:296307 Archived-At: Drew, below you quite reasonably suggest that I show what I'm proposing by coding it up. I didn't want to just let your message dangle without a reply, so this is just to say that I'm not sure if or when I'll have time to do that. While I can volunteer to review code in a timely manner, I'm much more cautious about volunteering to write code these days. I didn't want you to think my silence meant that I disagreed with your original idea or your followup -- I don't, it's just a matter of limited hacking time. Best regards, -Karl On 06 Sep 2022, Drew Adams 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. > >Retaining that setting till it's reset could >be a feature, not a bug. ;-) > >I imagine I won't have a problem if you bind >it lexically in `bookmark--jump-via', but I'm >not sure. > >Bookmark+ is designed to work also with Emacs >versions that don't have lexical binding. >Maybe using `lexical-let' here'll suffice, >maybe not. > >A `nil' value does matter - it's not the same >as calling a function unconditionally, even >when the function is `ignore'. > >E.g., my version of `bookmark-default-handler' >has this code, which doesn't raise the frame if >there's no display going on (a bookmark need >not display anything): > >(when bmkp-jump-display-function > (save-current-buffer > (funcall bmkp-jump-display-function (current-buffer))) > (raise-frame)) > >Sure, I could change (when XXXX ...) to >(when (eq XXXX 'ignore) ...), but that wouldn't >really improve anything, IMO. > >And I test the display function in some handlers, >for conditional handling/dispatch. E.g., my >handler for Dired bookmarks: > >(defun bmkp-jump-dired (bookmark) > "..." > (let ((dir > (bookmark-prop-get bookmark 'dired-directory)) > (mfiles > (bookmark-prop-get bookmark 'dired-marked)) > (switches > (bookmark-prop-get bookmark 'dired-switches)) > (subdirs > (bookmark-prop-get bookmark 'dired-subdirs)) > (hidden-dirs > (bookmark-prop-get bookmark 'dired-hidden-dirs))) > (case bmkp-jump-display-function > ((nil bmkp--pop-to-buffer-same-window display-buffer) > (dired dir switches)) > ((bmkp-select-buffer-other-window > pop-to-buffer > switch-to-buffer-other-window) > (dired-other-window dir switches)) > (t (dired dir switches))) > (let ((inhibit-read-only t)) > (dired-insert-old-subdirs subdirs) > (dired-mark-remembered > (mapcar (lexical-let ((dd dir)) > (lambda (mf) > (cons (expand-file-name mf dd) 42))) > mfiles)) > (save-excursion > (dolist (dir hidden-dirs) > (when (dired-goto-subdir dir) (dired-hide-subdir 1))))) > (let ((pos (bookmark-get-position bookmark))) > (when pos (goto-char pos))))) > >Obviously testing for functional equality isn't >possible. But testing a function symbol lets a >particular kind of bookmark jump differently >depending on current context/settings. > >> > Indeed :-( > >Not so obvious to me. Fine as a general guideline. >Not clear that it's a real win (no loss) here. > >> > Why can't `bookmark--jump-via` let-bind >> > `bookmark-jump-display-function` around >> > the call to `bookmark-handle-bookmark`? >> >> Of course -- obvious... >> Drew, does that sound good to you? > >See above. And please show your proposed code. > >> > 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) >> > ... >> > the above goes significantly beyond what one usually >> > expects from a docstring. It would fit much better >> > in a Texinfo manual instead. > >Feel free to write a TexInfo manual for bookmarking, >including for writing bookmark handlers, etc. What >exists now, and is as old as `bookmark.el', is just >some code comments and that doc string. > >That doc string documents the data structure for a >bookmark - it's THE place where that's specified. > >The handler part of that is important for people >who create a new bookmark type (always has been). >It's the one bit of bookmark data that really needs >explaining, as it's the one part that provides >behavior (code). > >> >> + 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. > >How is saying that a custom handler can invoke the >default handler, and summarizing what that does, >redundant here? > >Sure, it could just say that your handler can invoke >the default handler, and leave it at that. But that >just begs the question "Huh? Why?". And then, when >you go off to look at the doc of that function, all >you find is this (unmodified): > > Default handler to jump to a particular bookmark location. > BMK-RECORD is a bookmark record, not a bookmark name (i.e., not > a string). > Changes current buffer and point and returns nil, or signals a > `file-error'. > > If BMK-RECORD has a property called `buffer', it should be a > live > buffer object, and this buffer will be selected. > >(Why shouldn't it be able to be a bookmark name, BTW?) > >Does that doc string help you, if you're writing >your own handler? The code helps, but not that >doc string. It doesn't tell you what jumping >_means/does_ in the default case. > >IMO there's little sense in saying only that your >handler _can_ call the default handler. And even >less sense in saying neither that it can nor what >calling it does, i.e., _why_ you might want your >handler to invoke the default handler. > >But TexInfo manual - welcome. Go for it, please. > >> Agreed. > >Please show your proposed code (e.g. doc string). > >It's a deficiency of the existing doc string >that it doesn't really tell you anything useful >about the HANDLER part, IMO. > >But as I said, feel free to put what you like in >any of the doc strings. > >The patch doesn't _require_ any mention of the >possibility or use case of calling the default >handler from a nondefault handler. I think it's >good to mention that, but you needn't do so to >make the code change. > >> >> +(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`). > >See above. I don't think anything's really gained >by doing that here. As a general guideline, sure, >no problem. But let's hear a specific argument >for doing that here, please. > >> 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. > >Not so fine, IMO. And you might still need to test >its existence - with `(eq 'ignore)' instead of just >non-nil. And then there's trying to test for >functions "equivalent" to `ignore'... > >[FWIW, a somewhat related feature, which I didn't >include in my patch (but which could be considered >for another change), wraps most of the code of >`bookmark--jump-via' in a `catch', to which a >handler can `throw' to avoid doing anything else >(e.g. after-jump hook, auto-showing annotations).] > >> 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. > >If "otherwise" means that if a handler doesn't >use a display function then the display of the >default handler is used, then that's wrong. A >handler needs to be able to not display anything. > >> But all this raises the question you raise below... > >What question below? Do you mean the question >"Why `save-current-buffer'?" > >> >> @@ -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`? > >That's already in the code that the patch >replaces. It's in `bookmark--jump-via'. >Displaying is now in the default handler (or >another handler). That's all. > >Take out that ` save-current-buffer', if you >think you don't need it. I'll likely leave it >in my code. > >You can fiddle with the `bookmark--jump-via' >code if you like, to ensure that the behavior >change I described is the only one that's made. >I think that's already the case, but feel free. > >The code right after `(save-current-buffer...)' >expects the buffer that is current not to have >changed. In the unpatched code that right-after >code is in `bookmark--jump-via'. In the patched >code it's in the default handler. > >> >> (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`, > >Not in the code I patched or in the patched result, >it isn't. > >`bookmark-insert' calls `bookmark-handle-bookmark'. >That just invokes the default handler for _some_ >bookmarks. In general it doesn't mean that any >displaying occurs, and it doesn't say what >"displaying" means for any given kind of bookmark. > >> > so I think `bookmark-insert` should explicitly bind >> > `bookmark-jump-display-function` to `ignore`, > >But I don't have a problem if you want to do that. > >The problem you raise comes from `bookmark-insert' >trying to use a handler to insert all of the text >from the buffer that happens to be current after >the bookmark is handled. That assumes quite a lot. > >`bookmark-insert' is old (a vestige). It seems to >assume that you're just jumping to a file (see the >doc string: "text of the file"). Or at least a >text buffer. > >A more reasonable fix might be for `bookmark-insert' >to do nothing or to raise an error if the bookmark >type isn't a file bookmark. (Or maybe a text file. >Try it on a bookmark whose jumping displays an image >file.) > >> > and of course that begs the question of what to do >> > for handlers which don't obey >> > `bookmark-jump-display-function`. > >(Obey? How about use?) See above. It's more than >just a question of whether/how a bookmark displays >something. `bookmark-insert' apparently assumes a >narrow (even if common) sort of bookmark. > >(BTW, do you notice `save-current-buffer' there? >And that's before giving the handler any freedom >to display.) > >> > 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. > >What caller? Caller of what - a bookmark handler? >Do you see any other callers of handlers, besides >`bookmark-insert' (which is already broken for >bookmarks that don't go to a text buffer/file)? > >What kind of display control do you want a caller >of a handler to have? And why does that seem to >you to be a need? Can you elaborate on some >handler-calling you have in mind? > >> ...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? > >Redundant? A handler's responsible for any >displaying. It can, but it need not, use the >value of `bookmark-jump-display-function' to do >that. That's all. > >What's the question? By non-custom handler I >guess you mean the default handler - it invokes >`bookmark-jump-display-function'. > >BTW, with & without the patch there's this >"redundancy": `bookmark-bmenu-switch-other-window' >calls `bookmark--jump-via', passing a display >function. `bookmark-bmenu-other-frame' calls >`bookmark-jump', passing a display function. > >The change the patch provides is to give handlers >control over display - including choosing not to >display anything. > >> 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. > >I posted a patch that I think does what I propose. >It's very close to the code of Bookmark+, which >has been used for quite a while now. Feel free to >not apply the patch or to apply it and change any >doc strings. > >If you replace var `bookmark-jump-display-function' >with binding a function in `bookmark--jump-via' >then I'll try to adjust my code for that. If I >can't do so reasonably then the syncing of handler >behavior between Bookmark+ and bookmark.el might >not be so seamless. > >If you have an alternative patch to propose, >please do so. I'll try to take a look.