From: Ihor Radchenko <yantar92@posteo.net>
To: Bruno Barbier <brubar.cs@gmail.com>
Cc: emacs-orgmode <emacs-orgmode@gnu.org>,
Jack Kamm <jackkamm@gmail.com>, Matt <matt@excalamus.com>
Subject: Re: Pending contents in org documents
Date: Fri, 02 Aug 2024 16:48:35 +0000 [thread overview]
Message-ID: <87sevm50rg.fsf@localhost> (raw)
In-Reply-To: <66a9fa21.5d0a0220.624f7.d3d6@mx.google.com>
Bruno Barbier <brubar.cs@gmail.com> 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 "\\<org-pending-pending-keymap>"
> "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 <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
next prev parent reply other threads:[~2024-08-02 16:48 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-01 11:58 [BUG] Unexpected result when evaluating python src block asynchronously [9.7-pre (release_9.6.17-1131-gc9ed03.dirty @ /home/yantar92/.emacs.d/straight/build/org/)] Ihor Radchenko
2024-02-01 14:56 ` Bruno Barbier
2024-02-03 1:30 ` Jack Kamm
2024-02-04 15:07 ` Ihor Radchenko
2024-02-05 1:37 ` Jack Kamm
2024-02-05 14:29 ` Ihor Radchenko
2024-02-06 19:24 ` Bruno Barbier
2024-02-07 16:19 ` Ihor Radchenko
2024-02-07 17:40 ` Bruno Barbier
2024-02-08 3:21 ` Jack Kamm
2024-02-15 20:02 ` Asynchronous blocks for everything (was Re: [BUG] Unexpected result when evaluating python src block asynchronously [9.7-pre (release_9.6.17-1131-gc9ed03.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]) Matt
2024-02-16 17:52 ` Bruno Barbier
2024-02-18 21:14 ` Matt
2024-02-19 0:31 ` Jack Kamm
2024-02-20 10:28 ` Ihor Radchenko
2024-02-20 10:46 ` tomas
2024-02-20 11:00 ` Ihor Radchenko
2024-02-20 11:03 ` tomas
2024-02-21 15:27 ` Bruno Barbier
[not found] ` <notmuch-sha1-61e086e33bd1faf1a123c1b0353cf2102c71bdac>
2024-02-28 10:18 ` Pending contents in org documents (Re: Asynchronous blocks for everything (was Re: ...)) Bruno Barbier
2024-03-02 10:03 ` Ihor Radchenko
2024-03-02 10:57 ` Bruno Barbier
2024-03-02 11:13 ` Ihor Radchenko
2024-03-02 18:06 ` Bruno Barbier
[not found] ` <notmuch-sha1-d2799a191385bf51811d7788856a83b4f5a1fe3b>
2024-03-07 17:08 ` Bruno Barbier
2024-03-07 18:29 ` Ihor Radchenko
2024-03-08 14:19 ` Bruno Barbier
2024-03-13 9:48 ` Ihor Radchenko
2024-03-19 9:33 ` Bruno Barbier
2024-03-20 10:23 ` Ihor Radchenko
2024-03-21 10:06 ` Bruno Barbier
2024-03-21 12:15 ` Ihor Radchenko
2024-03-25 17:46 ` Bruno Barbier
2024-03-27 11:29 ` Ihor Radchenko
2024-03-30 22:53 ` Rudolf Adamkovič
2024-04-04 16:35 ` Bruno Barbier
2024-04-04 16:33 ` Bruno Barbier
2024-04-11 11:44 ` Ihor Radchenko
2024-04-19 11:23 ` Bruno Barbier
2024-04-20 10:07 ` Ihor Radchenko
2024-05-12 16:43 ` Bruno Barbier
2024-05-19 9:39 ` Ihor Radchenko
2024-05-23 16:31 ` Bruno Barbier
2024-05-24 9:49 ` Ihor Radchenko
2024-05-30 19:01 ` Bruno Barbier
2024-05-31 9:48 ` Ihor Radchenko
2024-06-01 6:28 ` Pending contents in org documents Bruno Barbier
2024-06-03 11:04 ` Ihor Radchenko
2024-06-15 7:49 ` Bruno Barbier
2024-06-16 9:31 ` Ihor Radchenko
2024-07-07 9:15 ` Bruno Barbier
2024-07-07 12:13 ` Ihor Radchenko
2024-07-18 8:05 ` Bruno Barbier
2024-07-19 14:23 ` Ihor Radchenko
2024-07-31 8:47 ` Bruno Barbier
2024-08-02 16:48 ` Ihor Radchenko [this message]
2024-08-12 7:14 ` Bruno Barbier
2024-08-13 9:49 ` Ihor Radchenko
2024-02-19 0:15 ` Asynchronous blocks for everything (was Re: [BUG] Unexpected result when evaluating python src block asynchronously [9.7-pre (release_9.6.17-1131-gc9ed03.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]) Jack Kamm
2024-02-21 15:43 ` Bruno Barbier
2024-02-19 9:06 ` Ihor Radchenko
2024-02-19 19:47 ` Matt
2024-02-19 20:10 ` Ihor Radchenko
2024-02-20 8:32 ` Ihor Radchenko
2024-02-20 17:04 ` Jack Kamm
2024-02-21 16:03 ` Bruno Barbier
2024-02-23 12:11 ` Ihor Radchenko
2024-02-23 13:24 ` Bruno Barbier
2024-02-24 11:59 ` Ihor Radchenko
2024-02-24 16:42 ` Bruno Barbier
2024-02-24 19:54 ` Matt
2024-02-28 10:21 ` Bruno Barbier
2024-02-08 3:26 ` [BUG] Unexpected result when evaluating python src block asynchronously [9.7-pre (release_9.6.17-1131-gc9ed03.dirty @ /home/yantar92/.emacs.d/straight/build/org/)] Jack Kamm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sevm50rg.fsf@localhost \
--to=yantar92@posteo.net \
--cc=brubar.cs@gmail.com \
--cc=emacs-orgmode@gnu.org \
--cc=jackkamm@gmail.com \
--cc=matt@excalamus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).