From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp11.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms5.migadu.com with LMTPS id qO4BMEM1lmPwewAAbAwnHQ (envelope-from ) for ; Sun, 11 Dec 2022 20:53:39 +0100 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp11.migadu.com with LMTPS id sBYJMEM1lmMvBwAA9RJhRA (envelope-from ) for ; Sun, 11 Dec 2022 20:53:39 +0100 Received: from mail.notmuchmail.org (yantan.tethera.net [IPv6:2a01:4f9:c011:7a79::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 6FFDB29835 for ; Sun, 11 Dec 2022 20:53:39 +0100 (CET) Received: from yantan.tethera.net (localhost [127.0.0.1]) by mail.notmuchmail.org (Postfix) with ESMTP id 0C4F35E537; Sun, 11 Dec 2022 19:44:23 +0000 (UTC) Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by mail.notmuchmail.org (Postfix) with ESMTPS id 51FB75E01E for ; Sun, 11 Dec 2022 19:44:20 +0000 (UTC) Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p4SFG-0003mu-49; Sun, 11 Dec 2022 14:44:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-Version:Date:References:In-Reply-To:Subject:To: From; bh=2r5N1r3Jn4E7NMfAp6MiOaEx3ZMRD2br8ot7jbKifMY=; b=bNp7Vdw/EKgl+tZKDWQ0 LV4wKHI7R5yH3dUTC3WfL2qtAmHL2I3YaRvolagVi9VPlakQ3+hPS962hwEg7+xKCERAOX9T4Y7FC UI4SoDYf2PkQ3Vqgi2bckEeRl9InuEJX5/ARNzx449A+KnA2DskbljHafk+bIhWjgtWHkEGixAd6Y tRrNuST5OW8Gs+WPnFCuYy0yiqmMJkpMJJinhFCXxTbqm8aX4li31dfqLO4VUZBi5UFjIFd9N2gkD Gbi/DR5LU5uCoOX6Yj/gz+8PmfvVox9unYUDUhJQsw3A+KnmqGiCTRDM6fyYpF4oc5KSH3PwKoMuF qDGQQz3dfTjZMQ==; Received: from cpc103048-sgyl39-2-0-cust502.18-2.cable.virginm.net ([92.233.85.247] helo=rivendell.localdomain) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p4SFF-0001Bq-My; Sun, 11 Dec 2022 14:44:17 -0500 Received: from localhost (rivendell.localdomain [local]) by rivendell.localdomain (OpenSMTPD) with ESMTPA id 151b0bbf; Sun, 11 Dec 2022 19:44:14 +0000 (UTC) From: jao To: David Bremner Subject: Re: [PATCH v7 1/1] emacs: notmuch-tree-outline-mode In-Reply-To: <87v8mhhk83.fsf@tethera.net> (David Bremner's message of "Sun, 11 Dec 2022 14:24:28 -0400") References: <20221104235229.515204-1-jao@gnu.org> <20221104235229.515204-2-jao@gnu.org> <87v8mhhk83.fsf@tethera.net> User-Agent: Gnus/5.13 (Gnus v5.13) X-Attribution: jao X-Clacks-Overhead: GNU Terry Pratchett X-URL: Date: Sun, 11 Dec 2022 19:44:14 +0000 Message-ID: <87ilih4tf5.fsf@mail.jao.io> MIME-Version: 1.0 Message-ID-Hash: 3G6SLO3K37JQWJR24QSSPD423TAPMC6T X-Message-ID-Hash: 3G6SLO3K37JQWJR24QSSPD423TAPMC6T X-MailFrom: jao@gnu.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-notmuch.notmuchmail.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: notmuch@notmuchmail.org X-Mailman-Version: 3.3.3 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Migadu-Country: DE X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1670788419; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-owner:list-unsubscribe:list-subscribe:list-post:dkim-signature; bh=Zd5QXALiwIMFASBa6qAC8Fk4rDpuymBlPxVVm2zfrC0=; b=eKGes/grLjVXr6gXQbCuPgLWXlyR/XRDzLJtgaMuufaEOjc1U0+C1dRUbT3zcI2LKDY1yu xjd6+PmviBboAXBc6dBkGsMz1iJkL6xm/VKBIMO/0PavjcevK6gJSyXZgMYBgGxU/hbpIj 7cLsE66Oh1zGth0BGQ6KYjfl9Mj2SW9tlwiJFmSJkriC4+uJltcg7sddCMTn3FeBHDhoVN nO/ciUvcoD/7X+aV+nlTqLB/Jd1/f41zuLYl4Q90cM8y9+lJcE8wsAQfHv9MbVpU2HJNy+ sjOXYBf2+gYODNrmLR4RQhJ2Rg7BHCRujm6fZxWVNqYEOfT6pa7U9abUbUGJFQ== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("body hash did not verify") header.d=gnu.org header.s=fencepost-gnu-org header.b="bNp7Vdw/"; spf=pass (aspmx1.migadu.com: domain of notmuch-bounces@notmuchmail.org designates 2a01:4f9:c011:7a79::1 as permitted sender) smtp.mailfrom=notmuch-bounces@notmuchmail.org; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gnu.org (policy=none) ARC-Seal: i=1; s=key1; d=yhetil.org; t=1670788419; a=rsa-sha256; cv=none; b=dqJUSWa1KWmcMFV4CkgqIupqKWKp1wRwZuh7FYhGfgLYubRm6DjO2Ab3iD0P/DwZ2+m7u1 gu9PnRQeKfRKy8csX9Fdti0JAve5F5ajR/nEyar846xIi2daD6Ptj2o8PzSSNACfLZqGUu albmAKFygscG9MybqzXGBCbucjPvI8Ei/8oEO+pePLPtqFeoJqZ/VqwQ7f7Fo9AEtUJGkN 5itY3dO6m2d7uGXFSl1cN4wg66ZaYK28llUozcT7vs8xbExH5kRCAk89xOkHwbf9HTe4aO CDIi+hDuPumV0fIY+6twwMXK1BAnJkQi/7Mpd34s2/8CQ7Gw/Q2YII52+PV/YA== X-Migadu-Spam-Score: 6.33 X-Spam-Score: 6.33 X-Migadu-Queue-Id: 6FFDB29835 X-Migadu-Scanner: scn0.migadu.com Authentication-Results: aspmx1.migadu.com; dkim=fail ("body hash did not verify") header.d=gnu.org header.s=fencepost-gnu-org header.b="bNp7Vdw/"; spf=pass (aspmx1.migadu.com: domain of notmuch-bounces@notmuchmail.org designates 2a01:4f9:c011:7a79::1 as permitted sender) smtp.mailfrom=notmuch-bounces@notmuchmail.org; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gnu.org (policy=none) X-TUID: jKOfVtXsVUin Thanks for your comments, David. Unfortunately, i won't be able to work on this for quite a few months. I am hoping someone else might perhaps take the patch over and fix the issues you pinpoint below. I'm adding just a handful quick comments: On Sun, Dec 11 2022, David Bremner wrote: [...] >> +(defcustom notmuch-tree-outline-open-on-next nil >> + "Open new messages under point if they are closed when moving to next one. >> + >> +When this flag is set, using the command >> +`notmuch-tree-outline-next' with point on a header for a new >> +message that is not shown will open its `notmuch-show' buffer >> +instead of moving point to next matching message." >> + :type 'boolean) >> + > > I'm curious about your choice of defaults here. At least the second > seems like t might make more sense? principle of least surprise: next-message in "normal" tree mode always goes to the next message, regardless of whether the current one is on display or not. for personal use, i defined my own commands changing that behaviour a long time ago, and, in that regard, yes, i agree defaulting to t is more natural. but i wasn't sure other users would agree. >> +(defun notmuch-tree-outline--set-visibility () >> + (when (and notmuch-tree-outline-mode (> (point-max) (point-min))) >> + (cond ((eq notmuch-tree-outline-visibility 'hide-others) >> + (notmuch-tree-outline-hide-others)) >> + ((eq notmuch-tree-outline-visibility 'hide-all) >> + (outline-hide-body))))) > > I wouldn't insist, but cl-case (or even pcase) might be clearer here. no opinion here (personally, i feel exactly the opposite, but i think it's just a matter of taste or familiarity). >> +(add-hook 'notmuch-tree-process-exit-functions #'notmuch-tree-outline--on-exit) > > Although it is relatively common in emacs code, I'm a bit skeptical of > using hooks for things not intended to be customized by the user. Should > we just be calling this directly somewhere? i'm neutral on that. with a hook, this package can almost be an independent add-on (it would only need the :level property below). but it's true that doesn't matter if we merge in notmuch. >> +(defsubst notmuch-tree-outline--level (&optional props) >> + (or (plist-get (or props (notmuch-tree-get-message-properties)) :level) 0)) > > As mentioned in my previous reply, I'm still not 100% clear on why we > need both depth and level. i might be misremembering, but i think depth is just an auxiliarly argument taken by that function to know whether it's inserting the tip of a tree or not, not a real depth. level is. so a better way would be to make 'depth' take the values 'level' is currently taking, but i wasn't sure other code would be using depth with its old original meaning (e.g. via and advice; i did at some point). >> +(defsubst notmuch-tree-outline--message-open-p () >> + (and (buffer-live-p notmuch-tree-message-buffer) >> + (get-buffer-window notmuch-tree-message-buffer) >> + (string-match-p (regexp-quote (or (notmuch-tree-get-message-id) "")) > > What's going with the 'or' here? Are we hiding errors? just preventing errors if someone calls this function (via a user-level command) in the "End of search" line or the blank line at the end of the messages list. > >> + (buffer-name notmuch-tree-message-buffer)))) > > At first glance, depending on the buffer name seems fragile? not sure why, or how to make it more robust... >> +;;;; Mode definition >> +(defvar notmuch-tree-outline-mode-lighter nil >> + "The lighter mark for notmuch-tree-outline mode. >> +Usually empty since outline-minor-mode's lighter will be active.") > > I feel like I should know what a "lighter" is in this context, but alas, > I don't the string in the mode line used to show a minor mode is active. aka "minor mode lighter" > Finally: > > According to a quick test, the folding does show up in > (visible-buffer-string) (from test/test-lib.el). So it seems feasible to > have a few tests with folded output? indeed. hope someone will find a bit of time for that! (fwiw, i've used variations of this code for almost a year, so it's relatively well field-tested; but of course that doesn't obviate proper tests) cheers, jao -- More people would learn from their mistakes if they weren't so busy denying them - Harlod J. Smith