Re: Pending contents in org documents

[Ihor Radchenko <yantar92@posteo.net>]

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>