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

[Bruno Barbier]

Hi Ihor,

Thanks for your review and detailed explanations.

I've pushed an update that should address most of your comments.

Let me answer point by point below.

Ihor Radchenko <yantar92@posteo.net> writes:

> Bruno Barbier <brubar.cs@gmail.com> writes:
>
>>> 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" ?
>
> Library prefix is also a part of the name and delivers useful
> information. "org-pending-region" and "region" and not the same names.
>
> We make use of prefix semantic in various places:
> - org-export-backend, implying not just "backend", but also "export"
> - org-cite-processor, implying not just "processor", but also "cite"
> - org-lint-checker - "org-lint" + "checker"
> - org-element-deferred - "org-element" + "deferred"
>
> So, there is no need to duplicate information from the prefix - it is an
> integral part of the struct name. Doing otherwise would go again the
> existing naming in Org code base.

Got it. Thanks for the explanation.

I've found a better name: I'm now calling it a "lock".  So I renamed
"PENREG" into "REGLOCK" as "region lock".  The structure is now
`org-pending-reglock'.


>>> 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.
>>
[...]
> Also, ~virtual~ field is unused. So, maybe we can even drop it
> completely. We can always add new fields in future, if a need arises.

The region is not allowed to be nil anymore. All "virtual" issues
are solved! :)


>>> 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 docstring of `org-pending' states that it is a buffer position:
>
>     The SOURCE is the buffer position that requested this pending region.

Sorry. It was, yes.

>> ... 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 am wondering why source must be a buffer position.
> What if we want to mark a region pending for some task not associated
> with a source? And why do we need to know the source at all?

Good point. It was to allow the user to quickly jump to the source
code block; I removed it from org-pending.



>>      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.)
>
> I have to say that I am confused about "insert-details" part. Mostly
> because it is not per se connected to the associated task. It is rather
> an additional handler used to provide debug information about the task
> status and outcome.
> AFAIU, it is conceptually very similar to HANDLE-RESULT function.

You're right: it was confusing.  It's now like a hook, that
org-pending-describe-reglock will use.  It should now be clearer that
it's for "information purposes" only.


> I think that rather than handing HANDLE-RESULT and also TASK-CONTROL, we
> may reduce everything to a single "handler" object that will serve as a
> way for PENREG to communicate back to Elisp. That way, we do not need to
> have a concept of a "task".

I removed the task-control, and, the concept of "task".  HANDLE-RESULT
is gone from org-pending.  `org-pending' has a new keyword: :on-outcome
that will allow to do anything, both on success and on failure.

> Instead, it will be a familiar async API
> with ability to (1) create (2) send signals to (3) receive signals from
> PENREG object.
> `org-pending' will be the entry point to create PENREG object.
>
> `org-pending-ti-send-update' (or maybe simply
> `org-pending-send-update'?) will be a way to send data to PENREG object.
>

I've renamed org-pending-ti-send-update to org-pending-send-update
(now that the task control is gone, the prefix becomes useless).


> HANLDER will be another object we may expose via something like
> (org-pending-handler (&key on-success-function on-cancel-function on-await 
> on-insert-logs) ...)
> Then, PENREG will call appropriate handler function as needed.

As the task-control is now gone:
  - get/await is gone,
  - cancel is now a hook/function of REGLOCK,
  - insert-details is now a hook/function too of REGLOCK.


>>>    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".
>
> I still feel confused. As stated above, it might be a good idea to get
> rid of the concept of "task" completely.

Done.


>>>    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 ?
>
> Yes. Lexical context is implicit and harder to debug, while storing
> necessary data explicitly in the struct slots is more robust as we are
> very clear which context is intended to be captured.

Done.


>>> 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.
>
> I do not think that it is a good analogy.
> Not when we also mark the text read-only in all the indirect buffers
> as well.
> Let me state my idea differently - if some text in buffer is "pending",
> it should be visible in all indirect buffers. Otherwise, as a user, I
> may be confused why some parts of the buffer are read-only.

You're describing the current implementation ... at least, what I
tried to do :)

With the current implementation, they should be clearly visible in all
buffers, using the secondary-selection face.  If they are not, it's
because Org is explicitly removing some text properties.

Oh, I see ... sorry: don't test indirect buffers with empty results, as
Org is almost always removing custom faces on "#+RESULT:" (IIUC, Org
doesn't comply with font-lock-face).

[...]

> But let's postpone indirect buffer discussion to later and focus on more
> high-level design first.
>
ok. Thanks.


[...]

>>>  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).
>
> Your thinking also makes sense, if I use a different definition of
> "failure" (in the context of PENREG, not in the context of exit code of
> the attached process)

org-pending now takes an :on-outcome callback: we can now decide what
to do both on success and on failure, and insert things in the org
document in all cases if needed.


Let me know what you think about this new version,

Thanks again for your help!

Bruno