Hi Ihor,
Ihor Radchenko <yanta...@posteo.net> writes:
> Bruno Barbier <brubar...@gmail.com> writes:
>
>> About how much to decorate, it depends on the user, I guess. For
>> example, when org-pending is used for org babel, it should be obvious
>> that one has to click on "#+RESULTS:".
>>
>> The current decoration is not the best for discoverability, indeed.
>> But decorating the whole outcome region would be too much IMHO, and,
>> it might interfer with other buffer fontifications.
>
> We may do what flycheck/flyspell do. Maybe fontifying not the whole
> region, but at least the region part where the indication was placed.
>
> I really find the current behavior unintuitive even for experienced Emacs
> users.
I added 2 faces; org-pending is now using those faces for the
whole outcome region. I personally don't like it and won't use it
though :)
>> I've refactored the code and added two variables so that it's now
>> configurable, see 'org-pending-outcome-pre-display-function' and
>> 'org-pending-outcome-post-display-function'.
>
> Makes sense. Although, "display" part in the names is very confusing -
> it sounds way too similar to `pre-redisplay-functions', which is
> something entirely different.
I've renamed them to pre-insert-outcome/post-insert-outcome.
> What about removing org-pending-output-pre-display-function entirely (we
> can add it later, if necessary) and renaming
> org-pending-outcome-post-display-function to
> `org-pending-on-outcome-functions' - an abnormal hook executed after
> ON-OUTCOME action.
I am using it, So, I would prefer to keep it :)
The timing does matter, so I would prefer to keep being explicit about
it (i.e. keep the pre/post prefix).
These 2 functions define the implementation; they are not hooks.
>>> 2. I tried to do M-x undo while the reglock was active, and got an
>>> error. I'd expect that undo would work, skipping the region.
>>
>> I'm not sure exactly what you did, nor which error you got. I've
>> noticed that the hacks (to handle indirect buffers) were flagging the
>> buffer as "modified" (using text properties). I've fixed that.
>>
>> Is the problem solved now ?
>
> No.
>
> What I did is:
>
> 1. make repro
> 2. Insert the example code from the top comment and uncomment it
> 3. M-x eval-buffer
> 4. *While regloc is active*, press C-x u
> 5. Observe
>
> Debugger entered--Lisp error: (org-pending-error "Cannot modify a region
> containing pending content")
> signal(org-pending-error ("Cannot modify a region containing pending
> content"))
> (closure (t) (&rest _) (signal 'org-pending-error (list "Cannot modify a
> region containing pending content")))(1 81)
> primitive-undo(1 ((1 . 81) (t . 0)))
> undo-more(1)
> undo(nil)
> funcall-interactively(undo nil)
> command-execute(undo)
Thanks for the details.
This is what is supposed to happen: the 'undo' is trying to erase the
whole buffer content, but that buffer contains a reglock, thus,
org-pending is explicitly interrupting that operation and raising a
meaningful error instead.
What other behaviour are you expecting here ?
>>> 3. I tried M-x undo _after_ reglock was unlocked, and I got "TO REPLACE"
>>> word highlighted. I did not expect it to be highlighted.
>>
>> I couldn't get that behavior, but the undo wasn't correct either.
>>
>> org-pending is now directly instrumenting the buffer-undo-list, and
>> manually adding an undo-boundary.
>>
>> Do you still see your problem ?
>
> No, this problem is solved now.
Perfect! Thanks for testing.
(I removed the undo-boundary from org-pending; I moved it to the
example)
>>> 4. If I try to cancel the reglock, it does get canceled, but *Region
>>> Lock* buffer is not updated - Live? still displays "yes Cancel".
>>
>> It's by design.
>>
>> The function `org-pending-describe-reglock' works like
>> `describe-variable' and other similar functions. You need to revert
>> the buffer (g) to update it.
>
> I still find it confusing.
> Mostly because it is not clear if pressing "cancel" does anything.
I added a header to make it clear that the info of the buffer is a
snapshot at a given time. And, that the user needs to hit the usual
key 'g' to revert the buffer.
When clicking "Cancel", org-pending now aknowledges that the cancel
request has been sent, using a message.
>> The reglock is live in its buffer.
>
> What do you mean by that??
> How can reglock be active in the buffer that does not contain the locked
> text? How can even have different state in different buffers?
Sorry, I meant:
The reglock displays its status, and keeps it up-to-date, in the
buffer containing the locked content.
>>> Does it mean that clicking "cancel" does not guarantee that the region
>>> will not be updated by the running process/timer?
>>
>> Yes, org-pending does not enforce that; and it should not, else it
>> would forbid valid use cases of org-pending. A given user of
>> org-pending may decide to garantuee that though (using a suitable
>> function for :on-outcome).
>
> Imagine that the process that locked the region hangs. As a user, I'd
> like to be able to edit text in buffer if I see that the lock is taking
> too long. How can I do it without closing the buffer?
As a user, I would tell the process to hurry up, possibly throwing
data away, as I need to edit that region (i.e. click "cancel").
Whoever started the process (that locked the region) should provide you a
way to stop that process, by answering the cancel request and/or
by providing another suitable interface.
The default implementation will unlock the region immediately,
completely disregarding any "process" and thus, will allow you to
immediately edit the content.
>>> In my eyes, there is no difference between user request and "kill". If
>>> users asks things to stop, no modifications should be allowed to
>>> the region.
>>
>> There is no relation between "kill" and "cancel".
>>
>> For "kill", *Emacs* is killing the owner of the lock; there is nothing
>> to update. This is synchronous, immediate and definitive.
>>
>> For "cancel", the *user* is sending an asynchronous message to
>> org-pending that it would be nice to release this particular lock
>> sooner, if possible; that message implies that the user doesn't care
>> about the outcome, but, if that outcome is available, then, just don't
>> waste it: insert it in the document.
>>
>> Should I rename "kill" and "cancel" to something better ?
>
> See my example above. For me, users should have access to "kill" - to
> unlock the pending region and make sure that nothing unexpected happens
> henceforth with that text. The unrelying Elisp that is creating the lock
> should indeed be able to intercept such user request and process it, but
> not refuse it, keeping the region locked.
org-pending is just the messenger here. It doesn't start any "process"
and it doesn't refuse anything, it fully cooperates :)
Your "kill" definition looks like the current default "cancel"
implementation.
To avoid further confusion, I'm not using the word "kill" anymore
about reglocks in org-pending.
I added a function 'org-pending-unlock-NOW!' which unlock the region
immediately. The uppercase "NOW!" emphasizes that it's not the
"safe" way to unlock a region.
>> I simplified the pcase. I switched back to the "my-" prefix. I'm not
>> sure why you're using quoted lambdas, as if we were in 2011 ;) I guess
>> it's so that this example works when copy/pasted to scratch for
>> evaluation in a non lexical-binding buffer.
>
> Yes.
ok. I'll try to keep that in mind. Thanks.
>> I've left the 'warn' messages, but, I'm not sure we should. In my
>> case, they are just killing my window configuration, even stealing the
>> window where the lock itself was.
>
> You can use (message ...) instead. It is also fine.
Good. Let's not promote the idea that raising popups from background
jobs at unpredictable time is a good idea: I replaced 'warn' with
'message'.
I've missed that you added a 1s sleep in the custom cancel function.
This was wrong: the documentation says that this function must return
*immediately* (see documentation of the field 'user-cancel-function').
I fixed it. I also restored the fact that, the outcome of a cancel
request may be either a success or a failure.
>
>>> In the above, it is not fully clear for me what BEG and END arguments in
>>> `org-pending-reglock-insert-details-function' mean and where the
>>> insertion happens.
>>
>> I've removed '_start' and '_end' from the example: they are advanced
>> features (see the documentation of the field insert-details-function).
>
> Please link to that documentation somewhere in top comment, linking to
> the defstruct. Maybe something like:
>
> Elisp programs can further alter various fields of REGLOCK object to
> alter its behavior. See the docstrings in `org-pending-reglock'.
Done.
I've just pushed a new version.
Thanks again for your patience and the many reviews.
Bruno
