From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0.migadu.com ([2001:41d0:403:4876::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms13.migadu.com with LMTPS id aNvWIay2uWYEOQAAqHPOHw:P1 (envelope-from ) for ; Mon, 12 Aug 2024 07:15:56 +0000 Received: from aspmx1.migadu.com ([2001:41d0:403:4876::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0.migadu.com with LMTPS id aNvWIay2uWYEOQAAqHPOHw (envelope-from ) for ; Mon, 12 Aug 2024 09:15:56 +0200 X-Envelope-To: larch@yhetil.org Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=RB7eq6lI; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org"; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1723446956; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:dkim-signature; bh=p/xeNHoUglPHYWXwFLXzWb1Te9f4+KSnuv3EOJe2CYo=; b=WkaUHK/xQm5A3lfssQRy3ueBrJb496RiFprMLYCx8YjjOYHWuWmCcA8RKf7bOwLwqsqe2m yi4AHQSoOACEMDGY4joHLKBaUsNqhOsNv8gwN1Lif9cpYSN2KXNzgD9vtRPSH6cgAV0r6w K/+vYmzqCt69NgUdNdz4+eASl/DVl8oCZ4Htg3c4zIhcyn2T3me9r2Ah2EsbQv51SwXFhz YFHxusTfuDr3nMMCr6Z+S/IR7TiTAOZylNoQ8F5uxW8Yg8WKLe8jFgXU1sbr1y5PRSFAex 4rRA2Ox/90uFm99AZmOKkbCWHrLjtd4npwS4iA+ZDVI5JpkJFzgJWZvxWgHxjw== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=RB7eq6lI; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org"; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=key1; d=yhetil.org; t=1723446956; a=rsa-sha256; cv=none; b=Oa3RFkWQd5Rrnba6M6/VCAcuIuc8qOaM0DGWGbN9WXcxLmeoA53yTaRQckPTo7w9IWGS99 Bg14xqs2VNUBXdbwskEyvtsle8mn3nrnG2LrczWCr6t5Mb62FZx3DxkF9UEIFMSbLaFoI3 +8lnLby/SSJsKqtcgCII2Vv4hsy4p+DHf6w8ZVVmW8Shs76tTeSC21S7g1Bb2lOZ2+IzRq rQ5+XgUpIeK3fA9c5RsmOBz2k8JWe1QNxvXiFt8oFN1wAkGzoyiU2sZhjYnyLMmxKmPTXV Q1FtTdV9+VvWLWa0ud95E+QUMeQ5lrCEYxsZgFfOGVuPVht1X2M1a0NwW3NqCA== Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 32DAD6E4F1 for ; Mon, 12 Aug 2024 09:15:56 +0200 (CEST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sdPGc-0003Zz-Lf; Mon, 12 Aug 2024 03:14:58 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sdPGa-0003Yy-Bl for emacs-orgmode@gnu.org; Mon, 12 Aug 2024 03:14:56 -0400 Received: from mail-wr1-x434.google.com ([2a00:1450:4864:20::434]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1sdPGT-0002R4-7E for emacs-orgmode@gnu.org; Mon, 12 Aug 2024 03:14:54 -0400 Received: by mail-wr1-x434.google.com with SMTP id ffacd0b85a97d-367940c57ddso2357761f8f.3 for ; Mon, 12 Aug 2024 00:14:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723446887; x=1724051687; darn=gnu.org; h=mime-version:date:references:in-reply-to:subject:cc:to:from :message-id:from:to:cc:subject:date:message-id:reply-to; bh=p/xeNHoUglPHYWXwFLXzWb1Te9f4+KSnuv3EOJe2CYo=; b=RB7eq6lIt2/Op12XjKUX08VIlgv4AVIQ+lQokOlZX6Ym0jbdC4L49t+OAvoutrDquL qKNHZ9kZyuBOOq6UOfyRUVyJFRB5iNstPBzcQIdSZzypr32IXM2bxz8P4RRTF3njompM LjzIFygW6uydX2DiC3pufs5U7D6MHLAQlZOKFlM+rzYrsLkaKhyV63aJjIxNBm/MZLxM d+d9DDCJtnTpxYA3kIMAS1LjXpxLNjytuDaoF/jJ9T9S3jmN62BRfe8F/7vH/oK99jqZ LYgNgAZgButoloHosj3NAlGtjS1IIUUD4Q13S/CIjSpfxt/b3zPXLbpnSaeGeWIfvsti PcCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723446887; x=1724051687; h=mime-version:date:references:in-reply-to:subject:cc:to:from :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=p/xeNHoUglPHYWXwFLXzWb1Te9f4+KSnuv3EOJe2CYo=; b=OX7qYxaR5905B49DXmSNPB60481d68mPAO/sOLattocLvsAmnlE08xEwVHH7ZeBktQ xF624mnwwns/iREE5X5VpfqkSJTmDMaLREOGSnglO2G2ImDTeZAAQwPMCaAfXiXlrrwJ Cc+VAOAN07Vtg9+Su9yzck4djjFqG1e+5hi5NoL4Brv6gdPfWfB95l0IA7tBrqBfh8Ii 8sYgvMP+x2BXqGWiiNhdejKQ5C9hElSwkqavWr8hO8um6FmfXrCe1Ms6yrzdF1BoYcRB McG2aTv2GgyQtW3NYwDCS+Lk8wbwa66sS0gk2N85uzETxbjZCUGQlR1n7TfD+d1XSujb tT3A== X-Gm-Message-State: AOJu0YzoVjeL08Hy+lOKHNDqt8pijh9jUR7uBHhiKiViyNAsfk35N9ML Hx5hrXy1fyPsHGJ4mTZxx4oMs8dMDn/IZJw1mIqbTWWC37uzfgp4 X-Google-Smtp-Source: AGHT+IHcxa1myHeNRZ8vkA0nW2ID+6hwXVerYsiesxDXbx8cPuoy9bfmxB2VM7Xl5j7rNrRZcQSdMA== X-Received: by 2002:a5d:420c:0:b0:35f:2030:d42c with SMTP id ffacd0b85a97d-36d5e3c023cmr5589453f8f.17.1723446886388; Mon, 12 Aug 2024 00:14:46 -0700 (PDT) Received: from localhost ([2a01:e0a:505:3460:1c18:688d:ece4:372e]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36e4cfee71esm6569959f8f.50.2024.08.12.00.14.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Aug 2024 00:14:45 -0700 (PDT) Message-ID: <66b9b665.5d0a0220.21f94d.fccc@mx.google.com> Received: by localhost (sSMTP sendmail emulation); Mon, 12 Aug 2024 09:14:43 +0200 From: Bruno Barbier To: Ihor Radchenko Cc: emacs-orgmode , Jack Kamm , Matt Subject: Re: Pending contents in org documents In-Reply-To: <87sevm50rg.fsf@localhost> References: <87o7d0mm54.fsf@localhost> <6601b872.050a0220.31a67.5a55@mx.google.com> <87le63c3qy.fsf@localhost> <660ed63d.050a0220.36fdd.af23@mx.google.com> <87cyqwyvgw.fsf@localhost> <66225435.5d0a0220.f60e4.c590@mx.google.com> <87r0f02vq2.fsf@localhost> <664f6f54.050a0220.10342.b002@mx.google.com> <87o78vwnds.fsf@localhost> <6658cd1f.050a0220.f6605.de1a@mx.google.com> <87jzja71n7.fsf@localhost> <665abf9f.050a0220.d2a6e.ef6b@mx.google.com> <87plsyz3rn.fsf@localhost> <666d4779.050a0220.13efd.8a2d@mx.google.com> <87ed8xi688.fsf@localhost> <668a5cc4.df0a0220.11fdd.05c1@mx.google.com> <87msmtflhq.fsf@localhost> <6698ccdb.050a0220.63d89.3c1c@mx.google.com> <87y15x4g11.fsf@localhost> <66a9fa21.5d0a0220.624f7.d3d6@mx.google.com> <87sevm50rg.fsf@localhost> Date: Mon, 12 Aug 2024 09:14:43 +0200 MIME-Version: 1.0 Content-Type: text/plain Received-SPF: pass client-ip=2a00:1450:4864:20::434; envelope-from=brubar.cs@gmail.com; helo=mail-wr1-x434.google.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, FREEMAIL_FROM=0.001, MSGID_FROM_MTA_HEADER=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, T_SCC_BODY_TEXT_LINE=-0.01, T_SPF_TEMPERROR=0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: emacs-orgmode-bounces+larch=yhetil.org@gnu.org X-Migadu-Country: US X-Migadu-Flow: FLOW_IN X-Spam-Score: -9.78 X-Migadu-Queue-Id: 32DAD6E4F1 X-Migadu-Scanner: mx10.migadu.com X-Migadu-Spam-Score: -9.78 X-TUID: aHCjiA9M4F+E Hi Ihor, Ihor Radchenko writes: > Bruno Barbier writes: > [...] >>> Ideally, we should have no hard-coded color names. >> [...] > If you have no ideas about faces to inherit from, better keep hard-coded > colors. > > (Also, this is not too critical; just something nice to have for better > inter-operability with Emacs themes) I've simplified everything, inheriting only basic faces that come preloaded with emacs. The display looks good enough, and, the code will be easier to maintain. Thanks. > >>>> (cl-defun org-pending--update (reglock status data) >>> >>> No docstring. Please, add. >> >> I added some basic documentation; I hope this is enough (this is an >> internal function). > > I prefer to have docstrings for every function, including internal > functions. This will make life easier for future contributors when > diagnosing whether a given function behaves as it supposed to. > > Ideally, we should detail what the function is expected to do by its > callers in its docstring. This way, we have a way to check if the code > behaves as it should in the future, after situations like external API > change or unexpected change in another internal API. Ideally, I agree :) Though, IME, for internal details of experimental code, improving the code readability and adding inline comments is more effective. >> (let ((map (make-sparse-keymap))) >> (dolist (k `([touchscreen-down] [mouse-2] [mouse-1])) >> (define-key map k 'org-pending-describe-reglock-at-point)) >> (when read-only >> (define-key map [13] 'org-pending-describe-reglock-at-point)) >> map)) > > Nitpick: #'org-pending... - this will help compiler to catch problems > if something happens to `org-pending-describe-reglock-at-point' (like > rename). Done. > Also, [13] is not very readable. Better use ?\C-m (or what did you mean?) I just blindly pasted those keys from the Emacs source code, to get the button-like behavior. I've replaced it, using keymap-set and "RET". To me, '?\C-m' doesn't look simpler than the ASCII code 13 :) >> (defun org-pending--make-overlay (type beg-end) >> ... >> (let ((overlay (make-overlay (car beg-end) (cdr beg-end))) >> (read-only >> (list >> (lambda (&rest _) >> (signal 'org-pending-error >> (list "Cannot modify a region containing pending content")))))) > > You can factor this lambda out into an internal constant. Done. >> (defun org-pending--make-overlay (type beg-end) >> "Create a pending overlay of type TYPE between BEG-END. > >> The pair BEG-END contains 2 positions (BEG . END). >> Create an overlay between BEGIN and END. Return it. > >> See `org-pending--delete-overlay' to delete it." > > It would be nice to have the docstring detail what kinds of properties > are applied to the overlay: (1) that it is read-only; (2) > org-pending--owner; (3) org-pending--before-delete; (4) keymap. I tried to improve the current documentation (only ':region' overlays are read-only). Almost every line of code is now in the documentation: that internal function is now officially declared "carved in stone", and, it should ship it as-is :) I've renamed `org-pending--owner' to `org-pending--real-owner', and I improved the comment about it in the code. The property `org-pending--before-delete' is now gone. > It is especially important to document properties that other functions > make use of. That's why I like closures so much: keep the internal details internal in one place; no need to split the logic and drop many parts in many different places :) >> (let ((overlay (make-overlay (car beg-end) (cdr beg-end))) >> (read-only >> (list >> (lambda (&rest _) >> (signal 'org-pending-error >> (list "Cannot modify a region containing pending content")))))) > > May you factor this lambda out into an internal constant? I think it's the same as above, or I missed something. Done ? :) > >> (cl-flet ((make-read-only (ovl) >> "Make the overly OVL read-only." >> (overlay-put ovl 'modification-hooks read-only) >> (overlay-put ovl 'insert-in-front-hooks read-only) >> (overlay-put ovl 'insert-behind-hooks read-only))) > > Or maybe even factor out make-read-only into an internal function that > can mark/unmark overlay/region with read-only text properties. Then, I'll have to document it ... No, thanks :) More seriously, org-pending doesn't need to mark/unmark. If an overlay has been created read-only, it just needs a way to delete it, and to also delete the attached hacks and, possibly try to undo their effects. >> (overlay-put overlay 'org-pending type) >> (unless (memq type '(:success :failure)) >> (overlay-put overlay 'face 'secondary-selection) > > Better use a new org-pending-specific face (inheriting from > secondary-selection). Right. The new face is `org-pending-locked'. Thanks. > >> (overlay-put >> overlay 'help-echo >> (substitute-command-keys >> (concat "\\" >> "This content is pending. " >> "\\[org-pending-describe-reglock-at-point]" >> " to know more.")))) > > You set a similar help-echo string in two places. It is a good > candidate to factor things out into a helper function. I'm not sure I understand. The two messages for the user are not the same, the keymaps are not the same. I'll end up with something like this: (defun overlay-set-help-with-keys (overlay message) ...) Is this really what you meant ? >> ;; Hack to detect if our overlay has been copied into an other >> ;; buffer. >> (overlay-put overlay 'org-pending--owner (overlay-buffer overlay)) > > AFAIU, the sole purpose of this is > > > ;; Try to detect and delete overlays that have been wrongly copied > > ;; from other buffers. > > ... but cloning buffer does not copy its overlays. Or do I miss something? If the function 'make-indirect-buffer' is called with a non-nil CLONE argument (like `org-tree-to-indirect-buffer' does), the buffer initial values are copied, and that includes the copy of overlays. Org-pending overlays work only in the buffer that owns the reglock: so, to support indirect buffers, org-pending needs to detect and remove such unwanted copies. Emacs should probably allow us to flag our overlays as "not clonable into other buffers". You can see what happens if you remove this hack and run the provided examples (that's how I discovered the problem a while ago): in scratch/bba-pending-contents/my-async-tests.org Section "Test cases" (near the end) > Also, "ownder" buffer is reachable via the associated REGLOCK object > (`org-pending-reglock-owner'). > Right. But it's a hack about overlays and indirect buffers: I prefer to keep this hack independent of reglocks. >> (when (memq type '(:success :failure)) >> ;; Add a link to the outcome overlay so that we may remove it >> ;; from any buffer. >> (with-silent-modifications >> (add-text-properties (car beg-end) (cdr beg-end) >> (list 'org-pending--outcome-overlay overlay))) > > Do I understand correctly that this is to support indirect buffers? If > so, why is it not a part of `org-pending--add-overlay-projection'? Correct: this is to support indirect buffers. I improved the inline comment. Only the ':region' overlay is projected with org-pending--add-overlay-projection, to protect the region from being modified. Here, this is about an *outcome* overlay (:success or :failure). That could be seen as another kind of "projection" indeed, but this one is used only in `org-pending-delete-outcome-marks', and it's only using one text property to create a link. > >> (overlay-put overlay 'org-pending--before-delete >> (lambda () >> (let ((inhibit-modification-hooks t) >> (inhibit-read-only t)) >> (overlay-put overlay 'modification-hooks nil) >> (overlay-put overlay 'insert-in-front-hooks nil) >> (overlay-put overlay 'insert-behind-hooks nil) >> (org-pending--remove-overlay-projection overlay) >> ;; Force refontification of the result >> ;; (working around indirect buffers hacks). >> (let ((start (overlay-start overlay)) >> (end (overlay-end overlay))) >> (remove-text-properties start end >> (list 'fontified :not-used >> 'font-lock-face :not-used >> 'font-lock-fontified :not-used)) >> (font-lock-flush start end))))) > > This feels like overengineering. I'd rather put the overlay removal > code into `org-pending--delete-overlay'. I see no reason to increase > memory used to store text/overlay properties unless we need to. I moved the logic inside `org-pending--delete-overlay'. I reduced the memory usage (for remove-text-properties calls). I'm not sure if it's still "overengineered" though. > Also, a more general question on `org-pending--make-overlay' design: Why > also not setting 'org-pending-reglock property right within this > function? Same for other places that modify the overlay in place after > creating. It feels like REGLOCK, face, help-echo (or even part of it), > and before-string can easily be parameters of > `org-pending--make-overlay'. At least, REGLOCK. Other properties may > be simply passed as some kind of (... &rest other-properties) argument > for `org-pending--make-overlay'. > IMHO, it would be nice to keep everything related to creating the > overlay in one function rather than spreading it all over the place. You're right that `org-pending--make-overlay' should set the 'org-pending-reglock' property. Done. I've also moved most of the configuration for outcome overlays from `org-pending-post-insert-outcome-default' into 'org-pending--make-overlay'. It makes more sense like that, indeed. Thanks. >> (defun org-pending-reglock-status (reglock) >> "Return the status of REGLOCK. >> The possible status are, in chronological order: >> :scheduled => >> :pending => >> :success >> or :failure." >> (org-pending-reglock--status reglock)) > > The return value is a symbol, right? You need to document this fact. Done. >> (defun org-pending-reglock-live-p (reglock) >> "Return non-nil if REGLOCK is still live. >> A REGLOCK stays live until it receives its outcome: :success or :failure." >> (funcall (org-pending-reglock--get-live-p reglock))) > > Here as well. I suggest marking the outcome types in the docstrings as > `:success' or `:failure'. That function just returns non-nil when live-p, no other promises. I've quoted the keywords. >> (defun org-pending-reglock-duration (reglock) >> "Return REGLOCK duration between its scheduling and its outcome. >> If the outcome is not known, use the current time." >> (let ((start (org-pending-reglock-scheduled-at reglock)) >> (end (or (org-pending-reglock-outcome-at reglock) >> (float-time)))) >> (- end start))) > > Is the return value in seconds? Internal time representation? Ooops: seconds. Fixed. Thanks! >> (defun org-pending-reglock-set-property (reglock prop val) >> "Set the value of the property PROP for this REGLOCK. >> See also `org-pending-reglock-property'." >> (if-let ((b (assq prop (org-pending-reglock-properties reglock)))) >> (setcdr b val) >> (push (cons prop val) >> (org-pending-reglock-properties reglock)))) > > `alist-get' is setf-able as well :) Indeed, way simpler! :) Thanks! >> (defun org-pending--user-cancel-default (reglock) >> "Send a cancel message to REGLOCK to close it. >> Default value for `org-pending-reglock-user-cancel-function'." >> (org-pending-send-update >> reglock (list :failure (list 'org-pending-user-cancel >> "Canceled")))) > > What is the purpose of 'org-pending-user-cancel? `org-pending-user-cancel' is the error/condition symbol, defined using 'define-error'. It's designed so that's easy to "unwrap" a message: (pcase outcome (`(:failure ,err) (signal (car err) (cdr err))) (`(:success ,v) v)) > Also, '(:failure (org-pending-user-cancel "Canceled)) would be shorter. Shorter ... but that message is sent to unknown consumers. With 'list', every consumer gets its own copy (they are free to mutate it if they so wish). With the "quote" version, every consumer will share the same message, and, if one consumer mutates it, he will change it for all other consumers, past, present and future, until Emacs is restarted. Definitely not the kind of bugs I would like to investigate. I added a comment about this choice in the code. Thanks. >> (defun org-pending-reglock-delete-outcome-marks (reglock) > > Not very related, but I am wondering if an API function to retrieve > reglock at point could be useful. WDYT? I added the function `org-pending-lock-or-outcome-at-point', which will, indeed, be required if anybody would like to customize the keymaps of pending or outcome overlays. Thanks! OK, that's it to answer your last review! Thanks! Just in case, don't jump to a detailed code review of my changes in ob-core yet, I've some planed changes there. - (already applied) I've changed the API in ob-core to fix some corner cases (results silent/none/etc.) and error handling: I'm moved back to using a callback to handle the outcome: it's a simpler and more flexible API than a reglock, plus, the reglock doesn't always exist. - (incoming) I'm going to redesign the ':nasync' parameter (I've been using asynchronous executions for everything by default (but elisp) for a few months now, and I realised async = "yes" or "no" is not enough for my needs). The plan is to have 3 execution modes: - schedule: schedule the execution, return immediately, report the outcome when it is known (i.e. async), - execute: schedule the execution, block the user until the outcome is known and handled (i.e. sync), raise on failure, return the result on success. - send: schedule the execution, return immediately, and forget about it (do not report success nor failure). I'm thinking about using ':execution-mode' as the new parameter name (to replace ':nasync'). After these changes, I will have to recheck everything thoroughly; so, that will be a good time to fix the `org-pending' interface like you requested a long time ago. It's now: (cl-defun org-pending (region ...) and, as you said, using two positions will better match the Emacs way, so it will become: (cl-defun org-pending (start end ...) Next set of comments about the org-pending library itself is very welcome (as it's mostly independent of it's integration with org-babel). I've updated my public branch. Thanks again Ihor, for your time and your review, Bruno