From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1.migadu.com ([2001:41d0:303:e16b::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms13.migadu.com with LMTPS id wKcFC9INrWYiJAAA62LTzQ:P1 (envelope-from ) for ; Fri, 02 Aug 2024 16:48:18 +0000 Received: from aspmx1.migadu.com ([2001:41d0:303:e16b::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1.migadu.com with LMTPS id wKcFC9INrWYiJAAA62LTzQ (envelope-from ) for ; Fri, 02 Aug 2024 18:48:18 +0200 X-Envelope-To: larch@yhetil.org Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=posteo.net header.s=2017 header.b=lbp1COTw; 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=posteo.net ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1722617298; 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=xwtgMZ8fWAabbP/ixADTcjHNnCMQlYbmJIPXxvlCXjU=; b=Ap2WXKFL32J0VOGdzpM93EyLiSJXw0Lh/HN8mOdadyRkhopbKFEY2LsSM5j4GANwTp2C/1 xENP14rELl/kvj5zGnUI0HyhdORLX5uhfH5FO6W3gePcGvs1MjQhrbGA30dSLhP8W2nZ/V ACdwmoQF7T2Gg/EFH/irlKADulqlouqxndP3obZnWVoysa15vyPs84AAePs0/HeZvsblpZ 4lG10XrqPPvgaeqE2t8NT272gEPf6IvSOhUqBxZH2ft5VDAYo6nHP9OKMNiaLHkFDo1Lia V7cR+uvv3awuCLMDugAxcUPvwr/BilrdCt8mr/JEtSPxl4jk5S9zv0tRq2Udeg== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=pass header.d=posteo.net header.s=2017 header.b=lbp1COTw; 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=posteo.net ARC-Seal: i=1; s=key1; d=yhetil.org; t=1722617298; a=rsa-sha256; cv=none; b=eoJsClFaghdtVJyG0iiOtLJpaZXiyUmSeB/mqnpqU8OQ9DzCd9B56SDTfIPlCqL3h7xza1 4GZd8XcmmibPXVCnlxWHGMlYu/s+85riTFXa86VXzaACPqZBtNyRXVo86VXW16n4FvY4Hp yj5VgbkuvnFg//+ck5niMc+fFZQO+eu2u9fhic80no7+vudFFLpYL0TSSPbkJeTUZ8T6sq gOgihrro+RA+Wb/BxU4D8MbdpmV1XSln8xnKnSptND0pg5sljQLsbWotR8lNBKkB1GqrKx 5EyNLF3u8bxEaznwqoLBVcIfMcNpQhRvxQFhjdxlH3uA/cRyEi+MsuYEAWmTpQ== 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 ABF5617252 for ; Fri, 2 Aug 2024 18:48:17 +0200 (CEST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sZvRB-0008Ol-0I; Fri, 02 Aug 2024 12:47:29 -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 1sZvR7-0008OR-Qe for emacs-orgmode@gnu.org; Fri, 02 Aug 2024 12:47:26 -0400 Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sZvR5-0007LD-01 for emacs-orgmode@gnu.org; Fri, 02 Aug 2024 12:47:25 -0400 Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id CC7E6240029 for ; Fri, 2 Aug 2024 18:47:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1722617236; bh=g2seIOUWH9ogZAdvp4ZEB5ISphM8BEtoIw1OiNLYmjM=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:Content-Type: From; b=lbp1COTwRLyKZHFb+OLWQIvZiDXme4lE7Hyl6pACMrJ+R6jrqKoswZHhj1WahAHze DNf8UrBfHeBNhV4igpqIOR1MZOp6BxiioqGW4SQhN7vpWtocI7RwJv06MM3QyTxIWi 4/dBRPs3jvhqOK9ag0HVDKiByO5uqzQ54e/mwffPpiHaXakNWqTT1fRQQazVe5rlak /XSZjm2tPEnToLsBf5/XzzetNFCOWLoGtE2UHHOyN2w78edwXoah8xvToZcl2KkE5o TZhJsqf+sDVv2wzcePH1MWRepWeedoKvybCdUBsAx6E5VgUjx5/BCxSuIfKHViyZSv mUxYL/QE9WZDA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4WbBZX0LNrz9rxB; Fri, 2 Aug 2024 18:47:15 +0200 (CEST) From: Ihor Radchenko To: Bruno Barbier Cc: emacs-orgmode , Jack Kamm , Matt Subject: Re: Pending contents in org documents In-Reply-To: <66a9fa21.5d0a0220.624f7.d3d6@mx.google.com> References: <87o7d0mm54.fsf@localhost> <87frwjlr1a.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> Date: Fri, 02 Aug 2024 16:48:35 +0000 Message-ID: <87sevm50rg.fsf@localhost> MIME-Version: 1.0 Content-Type: text/plain Received-SPF: pass client-ip=185.67.36.65; envelope-from=yantar92@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 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_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, RCVD_IN_VALIDITY_SAFE_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 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-Migadu-Queue-Id: ABF5617252 X-Migadu-Scanner: mx13.migadu.com X-Migadu-Spam-Score: -8.16 X-Spam-Score: -8.16 X-TUID: n2TRD7lFENJv Bruno Barbier writes: >> Then, there is no point in this function - users will never know about >> it. Maybe you do expose it as a button, but also supply a yes/no prompt >> asking for confirmation? >> > > The function 'org-pending-unlock-NOW!' is part of the API, it's not a > command Emacs end users. > > If we make it a command and display that button by default, we'll need > also an option to not display it by default, and, probably an other > option to not ask for confirmation. Let see later if we really need > to provide all this. Ok. >> Ideally, we should have no hard-coded color names. > > They were derived from built-in ones (error, success, org-tag, etc.). > > I've redesigned the faces and put them all in org-pending, so that > org-pending is indenpendent of Org. > > I'm now computing some colours from built-in faces to avoid colour > names. I'm not sure it's what you meant, as even Org itself doesn't do > this. Sorry, that's not what I meant. I was hoping that you can make use of inherit face attribute to define faces. Using just a part of an existing face is not a good idea - faces can be changed to make the _full_ combination of foreground/background/etc legit, but not necessarily to make individual color values reusable. 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) >>> (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. > I've pushed the changes to my branch. Thanks! Next set of comments :) > (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). Also, [13] is not very readable. Better use ?\C-m (or what did you mean?) > (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. > (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. It is especially important to document properties that other functions make use of. > (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? > (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. > (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). > (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. > ;; 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? Also, "ownder" buffer is reachable via the associated REGLOCK object (`org-pending-reglock-owner'). > (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'? > (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. 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. > (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. > (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'. > (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? > (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 :) > (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? Also, '(:failure (org-pending-user-cancel "Canceled)) would be shorter. > (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? -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at . Support Org development at , or support my work at