Re: Pending contents in org documents

[Ihor Radchenko <yantar92@posteo.net>]

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>