Hi Ihor,
It has been quite some time, sorry.
Quick summary for this update:
1. I've handled all your points, well, I hope so.
2. I've merged into a current "main".
3. I added an example using the new "futur.el" package.
4. I added some region helpers to handle regions; regions must now be
at least 2 characters long.
5. I didn't update the example that uses threads (my main Emacs is
compiled without threading support).
6. I didn't update the example that uses the async package.
About the integration with org, I already had 2 existing modes
"execute" (i.e. synchronous execution) and "schedule"
(i.e. asynchronous execution); I added a 3rd one called "send"
(i.e. send and don't care).
As I still don't have started the update of my copyright assignment
(new employer), I've changed the branch name, just in case:
bba-ngnu-pending-contents
https://framagit.org/brubar/org-mode-mirror/-/commits/bba-ngnu-pending-contents
End of summary, let's now anwser your last questions/comments one by
one.
Ihor Radchenko <[email protected]> writes:
> Bruno Barbier <[email protected]> writes:
...
>
> Aside: it looks like 'help-echo uses `substitute-command-keys' without a
> need to run it manually. So, we don't really need to do it:
>
> 33.19.4 Properties with Special Meanings (Elisp manual)
>
> ‘help-echo’
> If text has a string as its ‘help-echo’ property, then when you
> move the mouse onto that text, Emacs displays that string in the
> echo area, or in the tooltip window (*note Tooltips::), after
> passing it through ‘substitute-command-keys’.
Thanks. I've removed `substitute-command-keys' for `help-echo'.
>>>> ;; 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".
>
> Then, may you re-iterate what exactly is the problem if we do allow the
> overlays to be copied? Maybe we do not really need all the hassle with
> text properties?
A buffer owns its overlays. Once copied, each overlay begins its own
independant life in each buffer. If we don't remove the unrequested
copies, we would need to detect them anyway and implement some kind of
*live* synchronization. It's best to fix the copy mistakes by
deleting them immediately.
>>>> (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))
>
> But I do not see any calls to `signal' in such context. Do I miss something?
You're right. org-pending doesn't "signal" failures by itself; it just
make sure the consumer of the update (that uses org-pending) can
easily signal failures if it needs to.
Note that I've switched to using plain symbols (:failure => failure,
:success => success, etc.)
>> (cl-defun org-pending--new-button-like-keymap (&key read-only)
>> "Return a new keymap for use on reglock overlays.
>> If READ-ONLY is non-nil, add bindings for read-only text else for
>> editable text."
>> (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
>> (keymap-set map "RET" #'org-pending-describe-reglock-at-point))
>> map))
>
>> (defvar org-pending-outcome-keymap
>> (org-pending--new-button-like-keymap :read-only nil)
>> "Keymap for outcome overlays.")
>
>> (defvar org-pending-pending-keymap
>> (org-pending--new-button-like-keymap :read-only t)
>> "Keymap for pending region overlays.")
>
> Maybe we can make `org-pending-pending-keymap' inherit from
> `org-pending-outcome-keymap'? This way, if the latter is customized, the
> former will automatically update the bindings.
It's not clear to me why a key for an outcome would necesseraly be a
valid key for a reglock ... or the reverse :)
It might be better to define a `org-pending-base-keymap' and make both
inherit, but, let see if we really need this.
>> (defun org-pending--make-overlay (reglock type begin-end)
>> ...
>> (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)))
>
> You call `make-read-only' exactly once. cl-flet is redundant.
It clearly states my intent (although I'm not sure my attempt at
implementing it is good enough). The goal is not to avoid duplication
here.
>> ...
>> (when (memq type '(:success :failure))
>> (let ((bitmap (pcase type
>> (:success 'large-circle)
>> (:failure 'exclamation-mark)))
>> (face (pcase type
>> (:success 'org-done)
>> (:failure 'org-todo)))
>
> Bitmap and fringe face should be customizeable.
Done.
(IMHO, these kinds of non-breaking changes could easily be done later,
when a user of org-pending requests it)
>> ( outcome nil
>> :documentation
>> "The outcome. nil when not known yet. Else a list: (:success RESULT)
>> or (:failure ERROR)")
>
> Nitpick: Here, and in a couple of nearby docstrings, there is a missing "." at
> the very end
Sorry. Thanks.
>> ( properties nil
>> :documentation
>> "A alist of properties. Useful to attach custom features to this
>> REGLOCK." )
>
> Nitpick: Usually, "properties" imply plist. You might consider using a
> plist instead of alist or changing the field name to properties-alist.
The data model is fine. Let's change its name then.
I've renamed "property" to "attribute", "org-pending-reglock-property"
to "org-pending-reglock-attribute" and same for set.
>> ( -delete-outcome-marks (lambda ())
>
> Can just use #'ignore as the default value.
Done. Thanks.
>> (defun org-pending-reglock-useless-p (reglock)
>> "Return non-nil if REGLOCK is useless.
>> When a REGLOCK becomes useless, org-pending will, at some point, forget
>> about it."
>> (funcall (org-pending-reglock--useless-p reglock)))
>
> I feel like this idea of -useless-p is a kludge. We should use GC
> functionality instead, so that Emacs can take care about cleaning up
> things.
It's not really about Emacs GC.
(I'm glad you think I care about memory and CPU cycles, but I'm afraid
I really don't :) )
I just flag a reglock as useless if I spot it. Then, we can skip it
any time we see one (like not displaying it to the user). That Emacs
will GC it at some point later may be good, but, it wasn't the point.
> Note that Emacs exposes custom GC handlers to Elisp level.
> Check out "2.4.20 Finalizer Type" section of Elisp manual.
Thanks, but let's hope we won't need dive this deep for org-pending.
>> (cl-defun org-pending (region
>> &key anchor (name "REGLOCK")
>> (on-outcome #'org-pending-on-outcome-replace))
>> ...
>> ... The default ON-OUTCOME
>> function replaces the region on success and ignores failures; in all
>> cases, it returns the outcome region (see the function
>> `org-pending-on-outcome-replace').
>
> It is not clear what is used as the replacement by the default
> ON-OUTCOME.
Good point. Thanks. Improved.
>> You may set/update the following fields of your reglock to customize its
>> behavior:
>> - Emacs may have to destroy your locks; see the field
>> `before-destroy-function' if you wish to do something before your
>> lock is destroyed.
>
> It is not clear which "field" you are referring to when reading from top
> to bottom. Maybe, we can move "(cl-describe-type
> \\='org-pending-reglock)" a bit earlier to avoid confusion.
Good idea. Done.
>> You may add/update your own properties to your reglock using the field
>> `properties', which is an association list.
>
> I think that we should refer to `org-pending-reglock-property' here.
Done: I'm now referring to `org-pending-reglock-attribute' and
`org-pending-reglock-set-attribute'.
>> (let ((to-marker (lambda (p)
>> ;; Make sure P is a marker.
>> (or (and (markerp p) p)
>> (save-excursion (goto-char p) (point-marker)))))
>
> (copy-marker p) is shorter and does not involve moving point around.
>
> Also, we may want to copy these markers unconditionally, even when p is
> already the marker, it may have funny insertion type that slurps text
> inserted _after_ the marker (and locked region). So, maybe a simple
> unconditional (copy-marker p) is what we want.
Thanks. I've refactored adding some helper functions for region,
to-marker is gone.
>
> > (save-excursion
> > (setq anchor
> > (if (not anchor)
> > (let ((abeg ;; First non-blank point in region.
>
> The fact that ANCHOR does not include indentation is missing from the
> docstring.
Done.
>
> > (save-excursion (goto-char (car region))
> > (re-search-forward "[[:blank:]]*")
>
> `skip-chars-forward' would be slightly faster, as we do not need
> match-data here.
Done.
>
> > (point-marker)))
> > (aend ;; Last position on first line
> > (save-excursion (goto-char (car region))
> > (end-of-line)
>
> This will behave funny inside invisible text, because it honors point
> adjustments. Better use `line-end-position' + `copy-marker'.
But the doc for `line-end-position' says (info "(elisp) Text Lines"):
Return the position that ‘(end-of-line COUNT)’ would move to.
Anyway, I'm now using `line-end-position', with
`inhibit-field-text-motion'. I'm now also trying to not get past the
end of the region, and trying to avoid getting empty anchor. The doc
for ANCHOR is now incomplete again, but I guess it's ok.
>
>> (setq reglock (org-pending--make
>> :scheduled-at (float-time)
>> :-creation-point (point-marker)
>
> What is the purpose of -creation-point? It does not look like it is
> documented in `org-pending' docstring. Are there implicit assumptions
> about it in the other internal functions?
'-creation-point' is an internal field of `org-pending-reglock and is
documented there: it stores the point that was current when the
reglock was created. org-pending moves the point there while handling
updates.
One assumption is that it always exists ... which is wrong :(
org-pending now moves the point to the beginning of the reglock, while
handling updates (I'm not sure it's a good idea to move at all).
... but that breaks org-babel that relies on that assumption :(
I added a workaround in ob-core to get the same behavior as before in
org-babel only, storing the point from where the execution was
triggered. Maybe that assumption is OK in org-babel case.
>
>> :-on-outcome on-outcome
>> ;; useless-p returns non-nil when a reglock becomes
>> ;; useless and we may forget about it. We'll update the
>> ;; function when we get the outcome.
>> :-useless-p (lambda () nil)
>
> #ignore
Done ... although it's less clear now that it's function that returns
false :)
>
>> (defun org-pending-describe-reglock (reglock)
>> "Describe REGLOCK in a buffer.
>
> Maybe "Display a popup buffer describing REGLOCK."?
It's better indeed. The function is now returning the buffer.
>
>> (one-line "Duration"
>> ;; TODO nice human format like 1m27s
>> (format "%.1fs" (org-pending-reglock-duration reglock)))
>
> `org-duration-from-minutes'
That would have been nice indeed, thanks!
I did it but then I realized that I need to require org to do that.
As it's just for logging purposes (not for display in a nice
calendar), I just wrote a small function to write it in a "kind of"
ISO time format. So, it will be "00:00:27", not "1m27". I guess it's
good enough.
>
>> (defun org-pending-post-insert-outcome-default (lock message outcome-region)
>> "Default value for `org-pending-post-insert-outcome-function'."
>> ;; We add some outcome decorations to let the user know what
>> ;; happened and allow him to explore the details.
>> (let* ((outcome-ovl (org-pending--make-overlay lock (car message)
>> outcome-region)))
>> (push `(apply delete-overlay ,outcome-ovl) buffer-undo-list)
>
> This will err when undo is disabled in buffer (and `buffer-undo-list' is
> set to t).
Fixed. Thanks.
>> (cl-defun org-pending--update (reglock status data)
>> "Update REGLOCK to its new STATUS, using DATA.
>> Update the REGLOCK display to match the status STATUS (:scheduled,
>> :progress, :success, :failure). Also update the REGLOCK as needed.
>> Return nothing."
>> (cl-labels
>> ((add-style (status txt)
>> "Add the style matching STATUS over the text TXT."
>> (propertize txt 'face (org-pending-status-face status)))
>
> `add-style' is not used anywhere.
Thanks.
>> (short-version-of (msg)
>> "Compute the short version of MSG, to display on the anchor.
>> Must return a string."
>> (if msg
>> (car (split-string (format "%s" msg) "\n" :omit-nulls))
>> ""))
>
> Maybe `truncate-string-to-width'?
I couldn't find the right END-COLUMN to truncate. What really matters
is that progress messages fit on one line.
>
>> (overlay-put anchor-ovl
>> 'before-string
>> ...
>> (propertize
>> (pcase status
>> (:scheduled "⏱")
>> (:pending "⏳")
>> (:failure "❌")
>> (:success "✔️"))
>
> 1. indicator strings should be customizeable.
> 2. We need ASCII fallbacks of these symbols, if they cannot be displayed
Done.
>> (unless (and (consp outcome-region)
>> (or (integerp (car outcome-region))
>> (markerp (car outcome-region)))
>> (or (integerp (cdr outcome-region))
>> (markerp (cdr outcome-region))))
>> (error "Not a region"))))
>
> `integer-or-marker-p'
Thanks.
>> (if (not outcome-region)
>> (setf (org-pending-reglock--useless-p reglock)
>> (lambda () t))
>
> #'always
Thanks.
>
>> (let* ((pt (org-pending-reglock--creation-point reglock))
>> (buf (marker-buffer pt)))
>> (message "org-pending: Handling update message at %s@%s: %s"
>> pt buf upd-message)
>> (save-excursion
>> (with-current-buffer buf
>> (save-excursion
>> (goto-char pt)
>
> It will be shorter to use `org-with-point-at' from org-macs.el
> Also, when marker is outside, narrowing, your version will behave
> unexpectedly.
It's not clear to me what to do about narrowing; we probably need to
ignore it. That's what I'm now doing. And I'm still trying for
org-pending to not depend on org, so I'm trying to avoid having to
load org-macs.
>
>> (with-current-buffer buf
>> (save-excursion
>> (save-restriction
>> (widen)
>
> Or just (org-with-point-at start ...)
`org-with-point-at' would have been good, but, it would have
introduced a dependency on org.
>> (if (> (- end start) 1)
>> ;; Insert in the middle as it's more robust to
>> ;; keep existing data (text properties, markers,
>> ;; overlays).
>> ...
>
> See `replace-region-contents'
I'm not sure `replace-region-contents' is good here. I was just trying
to avoid to get an empty region, because you can't recover from it:
the markers before and after all get mixed up. I'm now forbidding
regions that don't have at least 2 characters; it's simpler and the
behavior should be predictable.
>
>> (defun org-pending-locks-in (start end &optional owned)
>> "Return the list of locks in START..END.
>
>> Return the list of REGLOCK(s) that are alive between START and END, in
>> the current buffer.
>
>> When OWNED is non-nil, ignore locks that are not owned by this buffer.
>
>> See also `org-pending-locks-in-buffer-p'."
>> (let ((reglocks)
>> (here nil)
>> ovl)
>> (while (and (< start end)
>
> Mind the narrowing.
The function `org-pending-locks-in' takes a region and thus respects
the current narrowing. The function `org-pending-locks-in-buffer-p'
ignores the narrowing. Is there something wrong here ?
>
>> (defun org-pending--mgr-handle-reglock-update (reglock update)
>> "Handle the update UPDATE for this REGLOCK.
>> Return nothing."
>> (message "org-pending: reglock update for id=%s: %s"
>> (org-pending-reglock-id reglock) update))
>
> Is it always desired to display a message each time a reglock is updated?
Definitely not. It's now a no-op.
Bruno
