From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Drew Adams Newsgroups: gmane.emacs.devel Subject: RE: [External] : Re: [PATCH] Let bookmark handlers do everything, including display the destination Date: Tue, 6 Sep 2022 00:07:04 +0000 Message-ID: References: <8735dt5si6.fsf@red-bean.com> <87ilma397o.fsf@red-bean.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="35811"; mail-complaints-to="usenet@ciao.gmane.io" Cc: "emacs-devel@gnu.org" To: Karl Fogel , Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Sep 06 02:08:22 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 1oVM8b-00097a-SR for ged-emacs-devel@m.gmane-mx.org; Tue, 06 Sep 2022 02:08:22 +0200 Original-Received: from localhost ([::1]:49942 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oVM8a-00033N-Pd for ged-emacs-devel@m.gmane-mx.org; Mon, 05 Sep 2022 20:08:20 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:43702) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oVM7Y-0002N2-Rj for emacs-devel@gnu.org; Mon, 05 Sep 2022 20:07:16 -0400 Original-Received: from mx0b-00069f02.pphosted.com ([205.220.177.32]:20018) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oVM7U-00020r-Ph for emacs-devel@gnu.org; Mon, 05 Sep 2022 20:07:15 -0400 Original-Received: from pps.filterd (m0246632.ppops.net [127.0.0.1]) by mx0b-00069f02.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 285Kmkuu032732; Tue, 6 Sep 2022 00:07:08 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=corp-2022-7-12; bh=z6JMrrznte6j5/W/Ityz2yJInWXLbNzUg4AUWwmmRzc=; b=AyRixZ9FtbtT+/GupdjIaQAzM6H+88h6NKzX+03iOLiMunHtd7hYSC+O9scmzVO//ipe UvMPS/IcPa7XS9kHWRZQti1vG0J9wQPSejiAOeA16LDZCRfToSfIXboACcpJnEI9Kd1A /5/S9UYa0Oj+Czp67hdw9seL2cqwrcTFpI0ybZiZXufblMHrNYWVNh8U5IiOEyhOneWj aaB2COXW10H3hHH1T28QZJy/EMVN20Eefc5UQHAdn8F+m/GFVXQK7lymwATg0ZBPgAnt n3SC+nb/fuQtc1ZKbtJDm4iz/P3LQLO8jaIO+RSCcxDeITJplkY4Mjxj8jS80SST50aH bQ== Original-Received: from phxpaimrmta03.imrmtpd1.prodappphxaev1.oraclevcn.com (phxpaimrmta03.appoci.oracle.com [138.1.37.129]) by mx0b-00069f02.pphosted.com (PPS) with ESMTPS id 3jbxhsm8we-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 06 Sep 2022 00:07:07 +0000 Original-Received: from pps.filterd (phxpaimrmta03.imrmtpd1.prodappphxaev1.oraclevcn.com [127.0.0.1]) by phxpaimrmta03.imrmtpd1.prodappphxaev1.oraclevcn.com (8.17.1.5/8.17.1.5) with ESMTP id 285K2vsp030676; Tue, 6 Sep 2022 00:07:06 GMT Original-Received: from nam11-bn8-obe.outbound.protection.outlook.com (mail-bn8nam11lp2168.outbound.protection.outlook.com [104.47.58.168]) by phxpaimrmta03.imrmtpd1.prodappphxaev1.oraclevcn.com (PPS) with ESMTPS id 3jbwc276q9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 06 Sep 2022 00:07:06 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DJWbPfhvmjHVu8DFhugL+J/Y9S4w5jWBNFXeEP/KiIpmgD5w5Z+TxISJd1DWdwDotUjJ5HEeVP7WM3IAYthuMdYNfSGHbvdqFA8W82dyJNy5DdWQ2FAkAl+SS5P4zzFJ8rfuVMODvCd8ilQfJgmpkCcXTNTvA0MV9IUhiNTQcjIEHJhxcEPuC9FrqtLDAJyVIqcu+kX1/05gQLzawSU7k450hnO7XdG1mJu6wBPnsZMQCeFZSYFeRPgy+wvBW3XruNLlgXtjpr/0Uw9qvFBCNCguDfB57Yf95euROaeHN7vVOPUHCysfhH5Jk4ZmheN1hAtKUJcO41CNfk5PXB4YlA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=z6JMrrznte6j5/W/Ityz2yJInWXLbNzUg4AUWwmmRzc=; b=exMZEi35qG+5OnUOVXrjysca1kBCmbKQyRQO20In46/Ou7nwoSf/8/PIy8lIvx40Bc1FkVNKLisq7c2FktTf9ozBccUyynz3HvmO40a6To9iUzw3QCIsPPa+I67qqE/wg4xulxFYTGQAukBl08uG0Hl2lhARKyfCxqanWo/ajpsViWjv4d2wf0/FpeSp3HGImwGRGSggVLMk6+uRDRHQ1iBQhYoTATR8auLv5RgPikwRdlQHb/178oaaK1/Vnd8AaTvKKKk3FloHRCpRZ21y0g017ZUZ4g5QZ3qFWn3fzOV2pm956XwW8CP/l9CTdVKHV9ooKdPe+3mj5AN9qcyqRA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oracle.com; dmarc=pass action=none header.from=oracle.com; dkim=pass header.d=oracle.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.onmicrosoft.com; s=selector2-oracle-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=z6JMrrznte6j5/W/Ityz2yJInWXLbNzUg4AUWwmmRzc=; b=IPpghl+CcNwB/YfOXqAhxVoCUXc3SG7Mhd6ojXe73ukZLflRVS9TrqWiO1FDrJWeBUWgfbq4jkcekT1jV7VqwfyFv2sTnPv77pmfbiuiPZgX29pwgBR2CNa9adWfQwClBnN+r1zHEEvA5Ts23tR5rHOrIapORd3jF+7HEVXXPIQ= Original-Received: from SJ0PR10MB5488.namprd10.prod.outlook.com (2603:10b6:a03:37e::19) by IA1PR10MB6121.namprd10.prod.outlook.com (2603:10b6:208:3ab::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5566.21; Tue, 6 Sep 2022 00:07:04 +0000 Original-Received: from SJ0PR10MB5488.namprd10.prod.outlook.com ([fe80::570:ff21:c9e1:22f1]) by SJ0PR10MB5488.namprd10.prod.outlook.com ([fe80::570:ff21:c9e1:22f1%3]) with mapi id 15.20.5588.018; Tue, 6 Sep 2022 00:07:04 +0000 Thread-Topic: [External] : Re: [PATCH] Let bookmark handlers do everything, including display the destination Thread-Index: AdivV1Of7BPjLOJdQwK4ZEV0yP9BkwMqiFMXAVfMJQA= In-Reply-To: <87ilma397o.fsf@red-bean.com> Accept-Language: en-US Content-Language: en-US x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 65fa1f60-a34e-4ec1-ffda-08da8f9bbab4 x-ms-traffictypediagnostic: IA1PR10MB6121:EE_ x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 600AdOd5JBtTA558eELvfDlHFUSV6ax97DLxODktJFWlPcVMDCBLOeo7FzpMO7uqeo7hPJpdg5OgjqYY0wdQzsO9H6k36IhSi7UcocSN8Ht6O9bzrIMqBhaSMumF7EAFgPVowP4tBFD9toTHaFx6Yh+IUo6Nj9WKzigbmKYwIpiRHjP6ruA9rD40amvjfLs1begX5sfJmaosoncO2BtBJyne5LwFem7VkyN7xMa4UQfcTfThC6XzFIHvmg4Zz5NKbSR+Ns+CYvYcO/URGy7HYTjsOpZ/v/bKnMqgZoKFji6zA3xdrv+31RnJQI4MPlHxflgPxZHD8Z1ChlCBs+2K3Mt9ITSGeZuKRd5ke8D/h5Qs5ojJeXf3M++oeuw9e2rE5lBrFF/MLY+dYHnjUq0P7r0s+sTv4DWOGNpRqvvXpYpIyCMVhi6tw22LaCz2Q0yZlbJtjZ3PPz2LQe6rDo0Dow6iCbImpQtRLgEFMuSCe0CJGJ4894HzX1DBRgPboXn+pI+/1dzPj2J1Pqlv6opO4kdHyhFI2oOSJgFm9T9nOV4hyi6jnipOQ/Nk7xb48xZV4e5entWAlxmBF4PdswX6wcDWKJ90ZNAPwlLtq0eLC6z+SEoXTMKuSpIMkA+6JeGmJL1+maL/i3QLEWJnsLALA6DYbaHD3SccKIqzpR22mnj+MYRohEpKqBT9uBWoyb+pTuhJzckl6oV3YsrYJi1V7dWA1TejhzgTkP97zIE4o/UIG5hIhlGic+kGddqjB qWxKyLwM6ZUzs5+yZu5H38Cww== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:SJ0PR10MB5488.namprd10.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230016)(136003)(366004)(376002)(39860400002)(396003)(346002)(30864003)(66946007)(2906002)(71200400001)(316002)(9686003)(33656002)(83380400001)(186003)(7696005)(6506007)(44832011)(26005)(66476007)(52536014)(38070700005)(8936002)(76116006)(66446008)(66556008)(110136005)(55016003)(64756008)(4326008)(122000001)(41300700001)(478600001)(38100700002)(5660300002)(8676002)(86362001); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?UGouszW9ikPCx9kUyqqXuvKz/RLH4aB9qBXPMfnoKcxQ8djEtBtSUwkzmjuo?= =?us-ascii?Q?wib4DeDiBAuFe5i9k9lirmoyT8TJs2GZOFsTGZEnRtnGc/cVGeXaa/ORNY8s?= =?us-ascii?Q?lzaEqCSg8nefL6jJNaS6my2P5KM42Qp99wG5bPzdEB3egbWBKVw9tbOBcH7E?= =?us-ascii?Q?A9K1FgO9LmM0HmK5Y/eZjSqaJmTvZtC/+ZlQEdpUH+x/uJu534Ic6a/VlqcY?= =?us-ascii?Q?eyJb1xOAXoGo3gV5CJ7cnWx0093lWQyMFJJdsR4X60jAWoVDthQPyilEnZVK?= =?us-ascii?Q?tDnKpMWpATnL6FH3oTUA+L0XH5irTbwTab8YQHQ6iD++9zepiT0qqBUJ1oDp?= =?us-ascii?Q?YKU6fwEVS32CuSP56/g8VpDVo41EZlGFkbwQjALRGRYMIDzVKe3hyA4fwQ7Y?= =?us-ascii?Q?VG+rO5OGqI6gdbqzWh93XM39DFUOKYwcFLQY0v4lnhbl8L/tDE0tWWKJYJcs?= =?us-ascii?Q?HMYGO6Gb1u2/JFYHR+BOyWbsTbrSmXzI/zwNihv08AQUhfnsp7WPfzzzKb1C?= =?us-ascii?Q?Zt1hcULxZAy03vxj4xHmDyfsMVenjaxy68XikHixn5mmGLUOclN43RRHS0cF?= =?us-ascii?Q?yDNU37TSiCdCvtX0Zm6X3UfQSk+5ENlDdOivJ7sOvGX7K2m51EQyFPckbLHW?= =?us-ascii?Q?zL X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB5488.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 65fa1f60-a34e-4ec1-ffda-08da8f9bbab4 X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Sep 2022 00:07:04.0848 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: wg0XJO6aqRWi3G/hlncd18Pg3J+1I3aa6dvFvXTtlqWsy86urG9mxQIGb7MgrA5pizrvc199a5QhqT3eFV81mw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB6121 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1 definitions=2022-09-05_16,2022-09-05_03,2022-06-22_01 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 malwarescore=0 mlxscore=0 phishscore=0 mlxlogscore=999 suspectscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000 definitions=main-2209050119 X-Proofpoint-GUID: eCB_1keoREDeOE-nRmPjvRnIkEMy6QVM X-Proofpoint-ORIG-GUID: eCB_1keoREDeOE-nRmPjvRnIkEMy6QVM Received-SPF: pass client-ip=205.220.177.32; envelope-from=drew.adams@oracle.com; helo=mx0b-00069f02.pphosted.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 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, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=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:294765 Archived-At: > >> 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=20 (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. =20 > > 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=20 > > the call to `bookmark-handle-bookmark`? >=20 > Of course -- obvious...=20 > 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. =20 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. =20 > 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.