Re: Pending contents in org documents

[Bruno Barbier <brubar.cs@gmail.com>]

Hi Ihor,

Ihor Radchenko <yantar92@posteo.net> writes:

> Bruno Barbier <brubar.cs@gmail.com> writes:
>
[...]
>>> Ideally, we should have no hard-coded color names.
>>
[...]
> 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)

I've simplified everything, inheriting only basic faces that come
preloaded with emacs.  The display looks good enough, and, the code
will be easier to maintain.  Thanks.



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

Ideally, I agree :)

Though, IME, for internal details of experimental code, improving the
code readability and adding inline comments is more effective.



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

Done.



> Also, [13] is not very readable.  Better use ?\C-m (or what did you mean?)

I just blindly pasted those keys from the Emacs source code, to get
the button-like behavior.  I've replaced it, using keymap-set and
"RET".

To me, '?\C-m' doesn't look simpler than the ASCII code 13 :)



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

Done.


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

I tried to improve the current documentation (only ':region' overlays
are read-only). Almost every line of code is now in the documentation:
that internal function is now officially declared "carved in stone",
and, it should ship it as-is :)


I've renamed `org-pending--owner' to `org-pending--real-owner', and I
improved the comment about it in the code.  The property
`org-pending--before-delete' is now gone.


> It is especially important to document properties that other functions
> make use of.

That's why I like closures so much: keep the internal details internal
in one place; no need to split the logic and drop many parts in many
different places :)



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

I think it's the same as above, or I missed something.
Done ? :)


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

Then, I'll have to document it ... No, thanks :)

More seriously, org-pending doesn't need to mark/unmark.  If an
overlay has been created read-only, it just needs a way to delete it,
and to also delete the attached hacks and, possibly try to undo their
effects.



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

Right. The new face is `org-pending-locked'. Thanks.


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

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 ?



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


You can see what happens if you remove this hack and run the provided
examples (that's how I discovered the problem a while ago):

     in scratch/bba-pending-contents/my-async-tests.org
     Section "Test cases" (near the end)


> Also, "ownder" buffer is reachable via the associated REGLOCK object
> (`org-pending-reglock-owner').
>

Right. But it's a hack about overlays and indirect buffers: I prefer
to keep this hack independent of reglocks.



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

Correct: this is to support indirect buffers.  I improved the inline
comment.

Only the ':region' overlay is projected with
org-pending--add-overlay-projection, to protect the region from being
modified.

Here, this is about an *outcome* overlay (:success or :failure). That
could be seen as another kind of "projection" indeed, but this one is
used only in `org-pending-delete-outcome-marks', and it's only using
one text property to create a link.



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

I moved the logic inside `org-pending--delete-overlay'. I reduced the
memory usage (for remove-text-properties calls).  I'm not sure if it's
still "overengineered" though.



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

You're right that `org-pending--make-overlay' should set the
'org-pending-reglock' property. Done.

I've also moved most of the configuration for outcome overlays from
`org-pending-post-insert-outcome-default' into
'org-pending--make-overlay'.  It makes more sense like that,
indeed. Thanks.


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

Done.


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

That function just returns non-nil when live-p, no other promises.
I've quoted the keywords.



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

Ooops: seconds. Fixed. Thanks!



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

Indeed, way simpler! :) Thanks!


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


> Also, '(:failure (org-pending-user-cancel "Canceled)) would be shorter.

Shorter ... but that message is sent to unknown consumers.  With
'list', every consumer gets its own copy (they are free to mutate it
if they so wish).  With the "quote" version, every consumer will share
the same message, and, if one consumer mutates it, he will change it
for all other consumers, past, present and future, until Emacs is
restarted.  Definitely not the kind of bugs I would like to
investigate.

I added a comment about this choice in the code.  Thanks.


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

I added the function `org-pending-lock-or-outcome-at-point', which
will, indeed, be required if anybody would like to customize the
keymaps of pending or outcome overlays. Thanks!


OK, that's it to answer your last review! Thanks!


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

 - (already applied) I've changed the API in ob-core to fix some
   corner cases (results silent/none/etc.) and error handling: I'm
   moved back to using a callback to handle the outcome: it's a
   simpler and more flexible API than a reglock, plus, the reglock
   doesn't always exist.

 - (incoming) I'm going to redesign the ':nasync' parameter (I've been
   using asynchronous executions for everything by default (but elisp)
   for a few months now, and I realised async = "yes" or "no" is not
   enough for my needs). The plan is to have 3 execution modes:

   - schedule: schedule the execution, return immediately, report the
     outcome when it is known (i.e. async),
     
   - execute: schedule the execution, block the user until the outcome is
     known and handled (i.e. sync), raise on failure, return the result
     on success.
     
   - send: schedule the execution, return immediately, and forget
     about it (do not report success nor failure).

I'm thinking about using ':execution-mode' as the new parameter name
(to replace ':nasync').

After these changes, I will have to recheck everything thoroughly; so,
that will be a good time to fix the `org-pending' interface like you
requested a long time ago.

It's now:

   (cl-defun org-pending (region ...)
   
and, as you said, using two positions will better match the Emacs way,
so it will become:

   (cl-defun org-pending (start end ...)



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

I've updated my public branch.

Thanks again Ihor, for your time and your review,

Bruno