Re: Pending contents in org documents (Re: Asynchronous blocks for everything (was Re: ...))

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

Ihor Radchenko <yantar92@posteo.net> writes:

Thanks for your review Ihor!

> Bruno Barbier <brubar.cs@gmail.com> writes:
>
>> I rewrote the API, rename many things, moved the code around and
>> sorted everything into heading/subheading sections.  This is hopefully
>> less confusing and a lot simpler; and the documentation hopefully
>> explains better how to use it.
>
> Thanks! It does look more clear.
>
>> The updated section "Commentary", in org-pending, describes the 3 steps
>> that are needed to mark and use a pending region: a PENREG (I've renamed
>> "PINFO" to "PENREG", for PENding REGion, more specific).
>
> I feel that org-pending-penreg (org-pending-<pending region>) is
> redundant. Maybe better use org-pending-region?

PENREG is the name of the structure; the "org-pending" is the
mandatory library prefix; this mechanically gives the name.  A PENDREG
object is not a "region" in Emacs sense.

Do you see a better name for the structure PENREG, so that it doesn't
sound like a "region" ?


>> WDYT of this version ?
>
> Comments on org-pending-pendreg struct:
>
> 1. It is not clear why you need a separate ~virtual~ field. When
>    ~region~ is nil it already implies that the pending region is
>    virtual.

It's a constant.  Calling a function looks more like we need to
recompute it each time, and, we could even change it.  And
cl-defstruct writes the function for us.

Do you prefer a manually written function ?


> 2. ~id~ field is semi-internal and assumed to be a number.
>    Maybe we can do something more similar to Emacs process API:
>    
>    (make-process &rest ARGS)
>    ...
>    :name NAME -- NAME is name for process.  It is modified if necessary
>    to make it unique.
>
>    We can replace id with human-readable name that can also be supplied
>    when creating a pending region

Good idea. Done.


> 3. ~source~ field must match the ~region~ marker buffer. Then, why do we
>    need this field at all? May as well just use (marker-buffer (car region))

The "source" is the region requesting the update.  The pending region
is the "target" of the update, i.e. the region that will be updated.


For example, in DEMO_ONLY, with org-babel, these 2 regions are never
the same:
   1. the source is the source code block,
   2. the target (pending region) is the result region.

I tried to improve the documentation of `org-pending-penreg' (source &
region).


> On the design of ~org-pending~ and ~org-pending-task-connect~:
>
> 1. I feel confused about the overall design of the interaction between
>    pending region and the associated task.
>
>    Functions like ~org-pending-task-send-update~ imply that pending
>    region is rather decoupled from from associated task and the task
>    should arrange manually for sending updates to the pending region
>    object.

Exactly: the task implementation must use these ~org-pending-task-XXX~
functions to send updates to one (or more) pending region(s).


>    On the other hand, there is ~task-connection~ that is used to kill
>    associated task/process or to "await" for it. This time, pending
>    region is strongly coupled with the task, killing it upon deleting
>    the pending region.


These are optional features; and only ~org-pending~ will know if and
when those might be useful.  That's why the task needs to provide
callbacks here.
     1. cancel: Emacs may, in exceptional cases only,
     send a "cancel" to the task, meaning, "The user destroyed the
     pending region, and thus, Emacs will not use any update for it".

     2. insert-details: If, and only if, the user decides to
     investigate what happened, Emacs will ask the task if it has any
     details to add, that might help the user (like exit-code for an
     OS process, stderr for an OS process or link to a log file, etc.)

     3. get (await): It's an (unofficial) way, (in the degenerate case
     where the task implementation gives up on asynchronicity) to
     block until the outcome is available.  `org-pending' itself
     doesn't use it; DEMO_ONLY uses it with org-babel to define the
     synchronous executions.

>    I think that we need more (optional) coupling between pending region
>    and the associated task. We should be able to get more information
>    about the task from pending region side - get logs, current status,
>    exit status, etc.


>    More specifically, I think that we need (1) allow to pass task as an
>    argument for ~org-pending~.

That's actually what I started with, but, I couldn't make it work.

Breaking it like this is what allowed me to get the most generic and
simplest API that works for anything: threads, callbacks, OS processes,
timers.

If org-pending takes a "task" as an argument, then, we have to define
a universal API for whatever a "task" might be: threads, processes,
callbacks, timers, etc. and any combination of them.

It looks simpler to say that the "task" (whatever "task" means), MAY
call:
    - org-pending-task-send-update (:progress xxx1)
    - org-pending-task-send-update (:progress xxx2)
    - org-pending-task-send-update (:progress xxx3)
      
then, finally MUST either call:

    - org-pending-task-send-update (:success RESULT)
or:
      org-pending-task-send-update (:failure RESULT)


>    (2) In ~org-pending-task-connect~, we
>    should allow the task to be a process or timer object. Then, we can
>    automatically arrange retrieving process/timer status from the task:
>    Use process sentinel (maybe, modifying existing via ~add-function~)
>    to arrange process status changes to be automatically submitted to
>    the pending region;
>
>    Get log updates via process filter
>
>    Kill process via ~kill-process~

It looks to me like a very specific case (one OS process for one
pending region); and I'm not sure how useful it would be in practice.

But this could easily be implemented on top of the existing API.


>    Similar for timers.

Same, it could easily be defined on top of the existing API.


>    If the argument to ~org-pending-task-connect~ is a lambda, we can use
>    the current approach you implemented on the branch.

> 2. ~org-pending-task-send-update~ name is confusing - it reads as if we
>    send an update _to_ the task. Maybe better ~org-pending-region-update~?

Yes ... I wanted a common prefix for the 3 functions that a "task"
implementation is allowed to use:
    - org-pending-task-connect,
    - org-pending-task-send-update,
    - org-pending-task-not-implemented.

It's not confusing if one ignores the common prefix :-)

I've renamed all these functions from "org-pending-task-" to
"org-pending-ti-" where "ti" stands for "task implementation".


>    Then, we might even drop ~-sentinel~ field in org-pending-penreg
>    object and instead implement that hard-coded ~update~ lambda from
>    ~org-pending~ as a part of ~org-pending-region-update~.

That would require to manually capture (dump/load) the context that
the sentinel closure is automatically capturing.

Why would it be better ? Debugging purposes ?

In this case, the current context is (currently) very small, and there
is an obvious place where to dump/load, so, just tell me if you want
me to eliminate that closure.

    
> 3. I feel that different handling of "owner" and indirect buffers is not
>    right.
>    From the user perspective, it does not matter whether we run an src
>    block from one or another indirect buffers - it makes sense to see
>    the status in all the indirect org-mode buffers.

I just tried to followe Emacs: a buffer owns its overlays; a pending
region is (kind of) an overlay.  Thus, a buffer owns its pending
region.


>    Maybe we can hook into org-mode's fontification and display
>    pending overlays in all the indirect buffers.

Well ... "adding overlays in indirect buffers using font-lock" looks
like a very bumpy road to me ... (being very positive, assuming there
is even a road there ... :-) ). As jit-lock is explicitly disabled in
indirect buffers, I'm not even sure what it would technically mean.


>    Further, it is very confusing that running src block twice from the
>    same buffer is not the same as running the same src block from one
>    buffer and then from another indirect buffer. The current
>    implementation of ~remove-previous-overlays~ makes such distinction
>    for reasons I do not understand.

Technically, the outcomes are overlays too; thus, they belong to
one buffer.

If a user created an indirect buffer to focus on some source blocks,
he should expect to manage everything about them from that buffer.
... that looks to me like a plausible explanation that matches the
technical limitations :-)

We might be able to add some other workarounds for indirect buffers,
to provide seamless switches between buffers.  But would that really
be worth it?

In summary, about indirect buffers, I'm not sure it's a good idea to
try to handle them.  I didn't do much with them, but, I'll already was
able to segfault Emacs.  I would prefer to put the no-clone-indirect
property to the org-mode personally :-)

Couldn't we just to forbid "pending regions" in indirect buffers ?
(pending regions don't exist today, so, that doesn't look that bad, at
least for now)



> 4. I have questions about ~handle-result~ argument in ~org-pending~.
> It is only called on success,

Yes. It means Org needs to handle the exact same result as in the
synchronous case, and, it must handle it in exactly the same way.
That's a design choice.

That's probably why it was easy to write the org-babel examples in
DEMO_ONLY; and they already handle most of the standard Org parameters:
async & sync, session & no session, append/prepend/post/stdin/var, etc.



>  but I can easily see that we need to
> handle things specially on failure as well. For example, insert
> stderr or perform other actions like displaying some buffer.  Or we
> may even hook some special action to clicking on status overlay. For
> example, clicking on "failure" status overlay may raise stderr log.

It's already there, no?

If you click on any result (success or failure, inline block or not,
even dynamic blocks), Emacs pops up a buffer with all the details
(source, start, end, duration, stderr, etc.). The function
`org-pending-describe-penreg' defines what is inserted. A given task is
free to insert log, links, widgets, images, diffs, etc. (by providing
the relevant :insert-details method).

These are design choices that are relevant here:
   1. Do not differ from the synchronous case.
   2. Do not delete a valid result until you know that you have
      something better (where a progress or a failure is not "better").
   3. Do not interrupt the user with popups.

If, in the synchronous case, org-babel writes some logs, then the task
must report a success to org-pending so that Org just behaves the
same.  If the task reports a failure, the user keeps the previous
content.

We could modify how ob-core uses org-pending, adding some options if
some users would like errors to be written down using some Org syntax.

I've pushed my update to the public repo (sorry, forced push again due
to some mistakes).


Bruno