Re: Pending contents in org documents

[Ihor Radchenko <yantar92@posteo.net>]

Bruno Barbier <brubar.cs@gmail.com> writes:

>>>           (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.
>
> 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 ?

Hmm. Not really. I misread the strings a bit and thought that they are
more similar than they actually are.

Aside: it looks like 'help-echo uses `substitute-command-keys' without a
need to run it manually. So, we don't really need to do it:

33.19.4 Properties with Special Meanings (Elisp manual)

    ‘help-echo’
         If text has a string as its ‘help-echo’ property, then when you
         move the mouse onto that text, Emacs displays that string in the
         echo area, or in the tooltip window (*note Tooltips::), after
         passing it through ‘substitute-command-keys’.

>>>       ;; 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".

Then, may you re-iterate what exactly is the problem if we do allow the
overlays to be copied? Maybe we do not really need all the hassle with
text properties?

>>> (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))

But I do not see any calls to `signal' in such context. Do I miss something?

> Just in case, don't jump to a detailed code review of my changes in
> ob-core yet, I've some planed changes there.

Sure. That will be after we tidy up the core library.

> 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).

Here it is :)

> (cl-defun org-pending--new-button-like-keymap (&key read-only)
>   "Return a new keymap for use on reglock overlays.
> If READ-ONLY is non-nil, add bindings for read-only text else for
> editable text."
>   (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
>       (keymap-set map "RET" #'org-pending-describe-reglock-at-point))
>     map))

> (defvar org-pending-outcome-keymap
>   (org-pending--new-button-like-keymap :read-only nil)
>   "Keymap for outcome overlays.")

> (defvar org-pending-pending-keymap
>   (org-pending--new-button-like-keymap :read-only t)
>   "Keymap for pending region overlays.")

Maybe we can make `org-pending-pending-keymap' inherit from
`org-pending-outcome-keymap'? This way, if the latter is customized, the
former will automatically update the bindings.

> (defun org-pending--make-overlay (reglock type begin-end)
> ...
> (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)))

You call `make-read-only' exactly once.  cl-flet is redundant.

>  ...
> (when (memq type '(:success :failure))
>   (let ((bitmap (pcase type
>                   (:success 'large-circle)
>                   (:failure 'exclamation-mark)))
>         (face (pcase type
>                 (:success 'org-done)
>                 (:failure 'org-todo)))

Bitmap and fringe face should be customizeable.

>   ( outcome nil
>     :documentation
>     "The outcome. nil when not known yet. Else a list: (:success RESULT)
> or (:failure ERROR)")

Nitpick: Here, and in a couple of nearby docstrings, there is a missing "." at
the very end

> ( properties nil
>   :documentation
>   "A alist of properties.  Useful to attach custom features to this REGLOCK." )

Nitpick: Usually, "properties" imply plist.  You might consider using a
plist instead of alist or changing the field name to properties-alist.

> ( -delete-outcome-marks (lambda ())

Can just use #'ignore as the default value.

> (defun org-pending-reglock-useless-p (reglock)
>   "Return non-nil if REGLOCK is useless.
> When a REGLOCK becomes useless, org-pending will, at some point, forget
> about it."
>   (funcall (org-pending-reglock--useless-p reglock)))

I feel like this idea of -useless-p is a kludge.  We should use GC
functionality instead, so that Emacs can take care about cleaning up
things.

Note that Emacs exposes custom GC handlers to Elisp level.
Check out "2.4.20 Finalizer Type" section of Elisp manual.

Here is a small demo:

(setq bar 'non-nil)
(with-temp-buffer
  (setq-local foo (list 1 (make-finalizer (lambda () (setq bar 'it-hapenned))))))
# may need to do GC several times, until it actually decides to clean up the killed buffer
(garbage-collect)
(garbage-collect)
bar

> (cl-defun org-pending (region
>                        &key anchor (name "REGLOCK")
>                        (on-outcome #'org-pending-on-outcome-replace))
> ...
> ... The default ON-OUTCOME
> function replaces the region on success and ignores failures; in all
> cases, it returns the outcome region (see the function
> `org-pending-on-outcome-replace').

It is not clear what is used as the replacement by the default
ON-OUTCOME.

> You may set/update the following fields of your reglock to customize its
> behavior:
>    - Emacs may have to destroy your locks; see the field
>      `before-destroy-function' if you wish to do something before your
>      lock is destroyed.

It is not clear which "field" you are referring to when reading from top
to bottom.  Maybe, we can move "(cl-describe-type
\\='org-pending-reglock)" a bit earlier to avoid confusion.

> You may add/update your own properties to your reglock using the field
> `properties', which is an association list.

I think that we should refer to `org-pending-reglock-property' here.

> (let ((to-marker (lambda (p)
>                    ;; Make sure P is a marker.
>                    (or (and (markerp p) p)
>                        (save-excursion (goto-char p) (point-marker)))))

(copy-marker p) is shorter and does not involve moving point around.

Also, we may want to copy these markers unconditionally, even when p is
already the marker, it may have funny insertion type that slurps text
inserted _after_ the marker (and locked region).  So, maybe a simple
unconditional (copy-marker p) is what we want.

    > (save-excursion
    >   (setq anchor
    >         (if (not anchor)
    >             (let ((abeg ;; First non-blank point in region.

The fact that ANCHOR does not include indentation is missing from the
docstring.

    >                    (save-excursion (goto-char (car region))
    >                                    (re-search-forward "[[:blank:]]*")

`skip-chars-forward' would be slightly faster, as we do not need
match-data here.

    >                                    (point-marker)))
    >                   (aend ;; Last position on first line
    >                    (save-excursion (goto-char (car region))
    >                                    (end-of-line)

This will behave funny inside invisible text, because it honors point
adjustments. Better use `line-end-position' + `copy-marker'.

> (setq reglock (org-pending--make
>                :scheduled-at (float-time)
>                :-creation-point (point-marker)

What is the purpose of -creation-point? It does not look like it is
documented in `org-pending' docstring. Are there implicit assumptions
about it in the other internal functions?

>                :-on-outcome on-outcome
>                ;; useless-p returns non-nil when a reglock becomes
>                ;; useless and we may forget about it.  We'll update the
>                ;; function when we get the outcome.
>                :-useless-p (lambda () nil)

#ignore

> (defun org-pending-describe-reglock (reglock)
>   "Describe REGLOCK in a buffer.

Maybe "Display a popup buffer describing REGLOCK."?

> (one-line "Duration"
>           ;; TODO nice human format like 1m27s
>           (format "%.1fs" (org-pending-reglock-duration reglock)))

`org-duration-from-minutes'

> (defun org-pending-post-insert-outcome-default (lock message outcome-region)
>   "Default value for `org-pending-post-insert-outcome-function'."
>   ;; We add some outcome decorations to let the user know what
>   ;; happened and allow him to explore the details.
>   (let* ((outcome-ovl (org-pending--make-overlay lock (car message)
>                                                  outcome-region)))
>     (push `(apply delete-overlay ,outcome-ovl) buffer-undo-list)

This will err when undo is disabled in buffer (and `buffer-undo-list' is
set to t).

> (cl-defun org-pending--update (reglock status data)
>   "Update REGLOCK to its new STATUS, using DATA.
> Update the REGLOCK display to match the status STATUS (:scheduled,
> :progress, :success, :failure).  Also update the REGLOCK as needed.
> Return nothing."
>   (cl-labels
>       ((add-style (status txt)
>          "Add the style matching STATUS over the text TXT."
>          (propertize txt 'face (org-pending-status-face status)))

`add-style' is not used anywhere.

> (short-version-of (msg)
>          "Compute the short version of MSG, to display on the anchor.
>          Must return a string."
>          (if msg
>              (car (split-string (format "%s" msg) "\n" :omit-nulls))
>            ""))

Maybe `truncate-string-to-width'?

> (overlay-put anchor-ovl
>              'before-string
> ...
>              (propertize
>               (pcase status
>                 (:scheduled "⏱")
>                 (:pending "⏳")
>                 (:failure "❌")
>                 (:success "✔️"))

1. indicator strings should be customizeable.
2. We need ASCII fallbacks of these symbols, if they cannot be displayed

> (unless (and (consp outcome-region)
>              (or (integerp (car outcome-region))
>                  (markerp (car outcome-region)))
>              (or (integerp (cdr outcome-region))
>                  (markerp (cdr outcome-region))))
>   (error "Not a region"))))

`integer-or-marker-p'

> (if (not outcome-region)
>     (setf (org-pending-reglock--useless-p reglock)
>           (lambda () t))

#'always

> (let* ((pt (org-pending-reglock--creation-point reglock))
>        (buf (marker-buffer pt)))
>   (message "org-pending: Handling update message at %s@%s: %s"
>            pt buf upd-message)
>   (save-excursion
>     (with-current-buffer buf
>       (save-excursion
>         (goto-char pt)

It will be shorter to use `org-with-point-at' from org-macs.el
Also, when marker is outside, narrowing, your version will behave
unexpectedly.

> (with-current-buffer buf
>   (save-excursion
>     (save-restriction
>       (widen)

Or just (org-with-point-at start ...)

> (if (> (- end start) 1)
>                    ;; Insert in the middle as it's more robust to
>                    ;; keep existing data (text properties, markers,
>                    ;; overlays).
> ...

See `replace-region-contents'

> (defun org-pending-locks-in (start end &optional owned)
>   "Return the list of locks in START..END.

> Return the list of REGLOCK(s) that are alive between START and END, in
> the current buffer.

> When OWNED is non-nil, ignore locks that are not owned by this buffer.

> See also `org-pending-locks-in-buffer-p'."
>   (let ((reglocks)
>         (here nil)
>         ovl)
>     (while (and (< start end)

Mind the narrowing.

> (defun org-pending--mgr-handle-reglock-update (reglock update)
>   "Handle the update UPDATE for this REGLOCK.
> Return nothing."
>   (message "org-pending: reglock update for id=%s: %s"
>            (org-pending-reglock-id reglock) update))

Is it always desired to display a message each time a reglock is updated?

-- 
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>