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, 19 Jul 2024 14:23:22 +0000 [thread overview]
Message-ID: <87y15x4g11.fsf@localhost> (raw)
In-Reply-To: <6698ccdb.050a0220.63d89.3c1c@mx.google.com>
Bruno Barbier <brubar.cs@gmail.com> writes:
>>> I added a function 'org-pending-unlock-NOW!' which unlock the region
>>> immediately. The uppercase "NOW!" emphasizes that it's not the
>>> "safe" way to unlock a region.
>>
>> I expect to see this function called by some kind of button in the
>> details buffer, so that users can actually call it.
>
> This function should not be used, not even in code, except to work
> around bugs. I would prefer not to provide a button for it.
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?
>> We should provide a user option to suppress the query. Something like
>> what `confirm-kill-processes' does. Maybe even support something akin
>> `process-query-on-exit-flag', but for reglocks.
>
> IMHO, it's the package that uses org-pending that should provide such a
> feature: I may not care about loosing data by ignoring on-going code
> block executions, but, I'll probably care if Emacs aborts some money
> transfers.
I kindly disagree. I am not proposing anything dramatically different
from what Emacs already does.
If you look into `confirm-kill-process' docstring, you will see that
Emacs does, in fact, have mechanisms for Elisp packages to tell whether a
given process is important or not (`process-query-on-exit-flag').
Yet, `confirm-kill-processes' overrides that mechanism if Emacs user
wishes to and sets the value to non-default nil.
There is nothing principally different in org-pending users compared to
processes. So, in order to conform with the rest of Emacs API, we do
need to provide an equivalent user customization.
We will not suppress queries by default, but we ought to give users an
option to suppress these queries. Maybe even follow
`confirm-kill-process' setting in how org-pending behaves.
>> Please describe what the default value of ON-OUTCOME (when ON-OUTCOME is
>> not explicitly provided) does right in the docstring.
>
> Done.
Thanks!
There is one typo in the relevant commit:
+ The default ON-OUTCOME
+function replaces the region on success and ignore failures; in all
^ignores
> I've pushed the changes to my repository.
Continuing the code review.
> (defface org-pending-outcome-failure
> ...
> (defface org-pending-outcome-success
If possible, please derive the faces from either org-faces.el or from
built-in faces like the ones listed in `modus-themes-faces' variable.
Ideally, we should have no hard-coded color names.
> :version "30.1"
Please, use :package-version instead of :version.
> (defun org-pending--status-still-pending-p (status)
This function is trivial and used only once. Maybe you can just inline
it.
> (defvar org-pending--outcome-keymap
> (defvar org-pending--pending-keymap
Why are these keymaps internal? May we better expose them to users for
modification?
It may also be a good idea to add some actual keyboard binding to this
map.
> (defun org-pending--popup-failure-details (exc)
This _internal_ function is unused in org-pending.el, but instead used
in ob-core.el. I am not yet looking into the changes outside
org-pending, but it is generally not ideal when library users need to
call an internal function from the library.
Also, this particular function does not look relevant to org-pending
library per se. It is rather generic and may be inlined.
> (defun org-pending-reglock-live-p (reglock)
You introduce this API function, but yet using
`org-pending-reglock--get-live-p' directly in other places in the
library. It will be more readable to use `org-pending-reglock-live-p'
everywhere.
> (defun org-pending-reglock-duration (reglock)
The docstring does not mention REGLOCK argument.
> (defun org-pending-reglock-property (reglock prop)
... PROP argument.
> (defun org-pending-reglock-set-property (reglock prop val)
> "See `org-pending-reglock-property'."
May as well write the full docstring.
> (defun org-pending-reglock-delete-outcome-marks (reglock)
> "Delete visual hints of the outcome for this REGLOCK, if any.
> Do nothing if the outcome is not known. Do nothing if there are no
We use double space between sentences in the docstrings.
> (cl-defun org-pending (region
> &key anchor name
> (on-outcome #'org-pending-on-outcome-replace))
NAME key is not documented in the docstring.
(and, more generally, please do M-x checkdoc on org-pending.el)
> (user-error "This region lock has been destroyed."))))
Error messages should not end with period.
> (let ((buffer (get-buffer-create "*Region Lock*")))
Please avoid using constant strings. Instead, declare them as defvar.
> (defun org-pending-describe-reglock (reglock)
> "Describe REGLOCK in a buffer.
>
> Describe position REGLOCK.
> The information is displayed in new buffer.
in *a* new buffer... except that it is not really a new buffer, when
another REGLOCK buffer is already being displayed.
May users want to see info about multiple reglocks at the same time?
> (setq-local header-line-format
> (format "Lock info (at %s), hit 'g' to update."
> (format-time-string "%T")))
Please, avoid using hard-coded bindings. Instead, see
`substitute-command-keys' (also see how org-capture.el sets up header
line)
> (defun org-pending--describe-reglock-at-point ()
Why internal? It is a command users may want to call via M-x.
> (defun org-pending-pre-insert-outcome-default (_lock _message)
> "Default value for `org-pending-pre-insert-outcome-function'.")
May as well just use #'ignore
> (defvar org-pending-pre-insert-outcome-function
> (defvar org-pending-post-insert-outcome-function
Should these be defcustom ?
> (overlay-put outcome-ovl
> 'help-echo
> "Last lock outcome, click to popup its full description.")
This assumes fixed key bindings in the lock overlay keymap. Which may or
may not be the case. Please compute 'help-echo programmatically, based on
the key map. (using `substitute-command-keys').
> (cl-defun org-pending--update (reglock status data)
No docstring. Please, add.
> (let* ((reg (org-pending-reglock-region reglock))
> (start (car reg))
> (end (cdr reg))
> (buf (marker-buffer start)))
> (when (buffer-live-p buf)
> (with-current-buffer buf
> (save-excursion
> (if (> (- end start) 1)
What if the buffer is narrowed and reglock is outside the narrowing?
> (with-current-buffer buffer
> (without-restriction
`without-restriction' is only available since Emacs 29. However, we
still support Emacs 27 and will support Emacs 28 for quite a while.
So, please avoid `without-restriction' if possible.
--
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-07-19 14:22 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 [this message]
2024-07-31 8:47 ` Bruno Barbier
2024-08-02 16:48 ` Ihor Radchenko
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=87y15x4g11.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).