[PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]


The current behavior of `org-add-planning-info' does not include 
the time of day when
it is passed the TIME argument. This is because 
`org-time-was-given' is initially let-bound to nil
during the scope of the function.

The attached patch removes that binding and allows the caller to 
pass the time of day in the above case.

An example use case I use with org-capture templates:

(defun +org-schedule-relative-to-deadline ()
  "Prompt for optional deadline, then an optional schedule time.
The scheduled default time is the deadline."
  (interactive)
  (condition-case nil
      (org-deadline nil)
    (quit nil))
  (let ((org-overriding-default-time (or (org-get-deadline-time 
  (point))
                                         org-overriding-default-time)))
    ;; works with patch, but without it the time of day is not 
    included in timestamp.
    (org-schedule nil (org-read-date 'with-time 'to-time nil 
    "SCHEDULE: " org-overriding-default-time))))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-add-planning-programmatic-time-of-day --]
[-- Type: text/x-patch, Size: 1291 bytes --]

From 2b0840a3963a16b4b92bfdad794a1327aa6f8bd4 Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Tue, 18 Aug 2020 17:09:50 -0400
Subject: [PATCH] org: (org-add-planning-info): Respect caller's
 `org-time-was-given' value

* lisp/org.el (org-add-planning-info): Respect caller's `org-time-was-given' value

Allows programmatically passing a time of day to `org-schedule'.

e.g. if one wanted to schedule a task relative to its DEADLINE time of day:

(org-schedule nil (org-read-date t t nil "SCHEDULE: "
                                 (org-get-deadline-time (point))))
---
 lisp/org.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index fb95590fc..aabdaebb9 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -10605,7 +10605,7 @@ among `closed', `deadline', `scheduled' and nil.  TIME indicates
 the time to use.  If none is given, the user is prompted for
 a date.  REMOVE indicates what kind of entries to remove.  An old
 WHAT entry will also be removed."
-  (let (org-time-was-given org-end-time-was-given default-time default-input)
+  (let (org-end-time-was-given default-time default-input)
     (when (and (memq what '(scheduled deadline))
 	       (or (not time)
 		   (and (stringp time)
-- 
2.28.0

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

Hi,

No Wayman <iarchivedmywholelife@gmail.com> writes:

> The current behavior of `org-add-planning-info' does not include the
> time of day when
> it is passed the TIME argument. This is because `org-time-was-given'
> is initially let-bound to nil
> during the scope of the function.
>
> The attached patch removes that binding and allows the caller to pass
> the time of day in the above case.

Applied in maint as c7abcd514, thanks.

-- 
 Bastien

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Bastien <bzg@gnu.org> writes:

> Applied in maint as c7abcd514, thanks.

Thanks, Bastien. I noticed org-end-time-was-given is bound 
similarly.
This binding doesn't affect my usage, but I wonder if it is 
necessary.
If you think it isn't, I'd be happy to submit another patch 
removing it.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Applied in maint as c7abcd514, thanks.
>
> Thanks, Bastien. I noticed org-end-time-was-given is bound similarly.
> This binding doesn't affect my usage, but I wonder if it is necessary.
> If you think it isn't, I'd be happy to submit another patch removing
> it.

Actually, "optimistic merging" was too optimistic and your change
breaks a test, as Kyle reported here:

https://orgmode.org/list/87tuwb35wh.fsf@kyleam.com/

If you see what's going on, can you report?

-- 
 Bastien

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Applied in maint as c7abcd514, thanks.
>
> Thanks, Bastien. I noticed org-end-time-was-given is bound similarly.
> This binding doesn't affect my usage, but I wonder if it is necessary.
> If you think it isn't, I'd be happy to submit another patch removing
> it.

Well, for the time being, I reverted the change (reverting c7abcd514)
because it broke a test.  If you can investigate why the test broke,
that'd be helpful.  Thanks!

-- 
 Bastien

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Bastien <bzg@gnu.org> writes:

> If you can investigate why the test broke,
> that'd be helpful.  Thanks!

Unfortunately I'm unable to get the test suite to run correctly.
I've followed the instructions in the testing/README, but the 
tests hang due to some sort misconfiguration with python and 
readline on my end:

> Warning (python): Your ‘python-shell-interpreter’ doesn’t seem 
> to
> support readline, yet ‘python-shell-completion-native-enable’ 
> was t
> and "python" is not part of the
> ‘python-shell-completion-native-disabled-interpreters’ list. 
> Native
> completions have been disabled locally.

I'm running Emacs 28.0.5 on Arch linux. Searching for a solution, 
but haven't come up with anything useful yet.
I've tried adding (setq python-shell-completion-native-enable nil) 
in the --eval program from the commandline,
but this only silences the warning. Still hangs. I'll keep 
reseraching.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Kyle Meyer]

No Wayman writes:

> Unfortunately I'm unable to get the test suite to run correctly.
> I've followed the instructions in the testing/README, but the 
> tests hang due to some sort misconfiguration with python and 
> readline on my end:
>
>> Warning (python): Your ‘python-shell-interpreter’ doesn’t seem 
>> to
>> support readline, yet ‘python-shell-completion-native-enable’ 
>> was t
>> and "python" is not part of the
>> ‘python-shell-completion-native-disabled-interpreters’ list. 
>> Native
>> completions have been disabled locally.
>
> I'm running Emacs 28.0.5 on Arch linux. Searching for a solution, 
> but haven't come up with anything useful yet.
> I've tried adding (setq python-shell-completion-native-enable nil) 
> in the --eval program from the commandline,
> but this only silences the warning. Still hangs. I'll keep 
> reseraching.

Hmm, since 4df12ea39 (ob-python: Improve robustness of session
evaluation, 2020-09-06), the ob-python tests stall for me as well.

    $ make BTEST_RE='test-ob-python' test-dirty
    selected tests: test-ob-python
    Running 14 tests (2020-09-08 00:31:36-0400)
    Can’t guess python-indent-offset, using defaults: 4
    executing Python code block...
    Shell native completion is enabled.
    <hangs>

That's on a Debian system with the python executable pointing to Python
2.7.16.  If I set org-babel-python-command to python3 (3.7.3) at the top
of test-ob-python.el, I see the same thing.  I haven't dug any farther
yet.  Jack, presumably you don't see the stall on your end?

No Wayman, you could try running more targeted tests to get around the
stall.  For example, here's how to run just tests matching "test-org/".
In order to trigger the failure, I'm running it on the parent commit of
73a78c251 (Revert "org.el (org-add-planning-info): Use caller's
`org-time-was-given' value", 2020-09-06).

    $ make BTEST_RE='test-org/' test-dirty
    [...]
    Test test-org/org-read-date backtrace:
      signal(ert-test-failed (((should (equal "2012-03-30" (org-test-at-ti
      ert-fail(((should (equal "2012-03-30" (org-test-at-time "2012-03-29
      (if (unwind-protect (setq value-12923 (apply fn-12921 args-12922)) (
      (let (form-description-12925) (if (unwind-protect (setq value-12923
      (let ((value-12923 (quote ert-form-evaluation-aborted-12924))) (let
      (let* ((fn-12921 (function equal)) (args-12922 (condition-case err (
      (lambda nil (let* ((fn-12881 (function equal)) (args-12882 (conditio
      ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
      ert-run-test(#s(ert-test :name test-org/org-read-date :documentation
      ert-run-or-rerun-test(#s(ert--stats :selector "test-org/" :tests [#s
      ert-run-tests("test-org/" #f(compiled-function (event-type &rest eve
      ert-run-tests-batch("test-org/")
      ert-run-tests-batch-and-exit("test-org/")
      (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
      org-test-run-batch-tests("test-org/")
      eval((org-test-run-batch-tests org-test-select-re))
      command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
      command-line()
      normal-top-level()
    Test test-org/org-read-date condition:
        (ert-test-failed
         ((should
           (equal "2012-03-30"
                  (org-test-at-time "2012-03-29 16:40" ...)))
          :form
          (equal "2012-03-30" "2012-03-30 00:40")
          :value nil :explanation
          (arrays-of-different-length 10 16 "2012-03-30" "2012-03-30 00:40" first-mismatch-at 10)))
       FAILED   76/113  test-org/org-read-date
    [...]


As I mentioned in the message linked upstream, it doesn't happen when
running just test-org/org-read-date (BTEST_RE='test-org/org-read-date'),
so there's some sort of interaction between tests.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Jack Kamm]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

Kyle Meyer <kyle@kyleam.com> writes:

> That's on a Debian system with the python executable pointing to Python
> 2.7.16.  If I set org-babel-python-command to python3 (3.7.3) at the top
> of test-ob-python.el, I see the same thing.  I haven't dug any farther
> yet.  Jack, presumably you don't see the stall on your end?

No, the tests don't stall on my end (Archlinux with manually compiled
emacs 28). I also tested on a debian10vm and the tests passed there too.

But since we know it's related to 4df12ea39 that gives some
clues...could you try the attached patch to see if it fixes things?

Also, could you try executing some simple ob-python session blocks and
see if they hang? e.g.,

#+begin_src python :session :results output
  print(1+1)
#+end_src

#+begin_src python :session :results value
  1+1
#+end_src


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: python-eoe-patch.patch --]
[-- Type: text/x-patch, Size: 1728 bytes --]

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 1cded4515..a5af55892 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -223,6 +223,9 @@ (defun org-babel-python-initiate-session (&optional session _params)
     (org-babel-python-session-buffer
      (org-babel-python-initiate-session-by-key session))))
 
+(defvar org-babel-python-eoe-indicator "org_babel_python_eoe"
+  "A string to indicate that evaluation has completed.")
+
 (defconst org-babel-python-wrapper-method
   "
 def main():
@@ -324,7 +327,9 @@ (defun org-babel-python--send-string (session body)
 	   (comint-output-filter-functions
 	    (cons (lambda (text) (setq string-buffer
 				       (concat string-buffer text)))
-		  comint-output-filter-functions)))
+		  comint-output-filter-functions))
+	   (body (format "%s\nprint('%s')"
+			 body org-babel-python-eoe-indicator)))
       (if (not (eq 'python-mode org-babel-python-mode))
 	  (let ((python-shell-buffer-name
 		 (org-babel-python-without-earmuffs session)))
@@ -333,13 +338,10 @@ (defun org-babel-python--send-string (session body)
 	(py-shell-send-string body (get-buffer-process session)))
       ;; same as `python-shell-comint-end-of-output-p' in emacs-25.1+
       (while (not (string-match
-		   (concat "\r?\n?"
-			   (replace-regexp-in-string
-			    (rx string-start ?^) "" comint-prompt-regexp)
-			   (rx eos))
+		   org-babel-python-eoe-indicator
 		   string-buffer))
 	(accept-process-output (get-buffer-process (current-buffer))))
-      (substring string-buffer 0 (match-beginning 0)))))
+      (org-babel-chomp (substring string-buffer 0 (match-beginning 0))))))
 
 (defun org-babel-python-evaluate-session
     (session body &optional result-type result-params)

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Jack Kamm <jackkamm@gmail.com> writes:

> Also, could you try executing some simple ob-python session 
> blocks and
> see if they hang? e.g.,
>
> #+begin_src python :session :results output
>   print(1+1)
> #+end_src
>
> #+begin_src python :session :results value
>   1+1
> #+end_src

These both work for me now on master as of f17d301e1.
I ran make test and recieved the following output:

>1 unexpected results:
>   FAILED  test-ob-python/session-multiline

Applied the patch you provided, re-ran make test and recieved:

> Ran 862 tests, 862 results as expected, 0 unexpected (2020-09-09 
> 00:12:49-0400, 15.237936 sec)
> 12 expected failures

Source blocks executed correctly as well.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Kyle Meyer]

Jack Kamm writes:

> No, the tests don't stall on my end (Archlinux with manually compiled
> emacs 28). I also tested on a debian10vm and the tests passed there too.
>
> But since we know it's related to 4df12ea39 that gives some
> clues...could you try the attached patch to see if it fixes things?
>
> Also, could you try executing some simple ob-python session blocks and
> see if they hang? e.g.,
>
> #+begin_src python :session :results output
>   print(1+1)
> #+end_src
>
> #+begin_src python :session :results value
>   1+1
> #+end_src

With Org's master (f17d301e1), the second one hangs for me with Emacs
26.3.  Quickly stepping through org-babel-python--send-string, it
appears to get stuck in the accept-process-output call.

Using an Emacs built from master, there's no hang on my end either.  If
you'd find it useful, I can try to bisect that to a specific Emacs
commit tomorrow.

The Emacs 26.3 hang goes away after installing your patch that restores
the use of a marker.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Jack Kamm]

Kyle Meyer <kyle@kyleam.com> writes:

> With Org's master (f17d301e1), the second one hangs for me with Emacs
> 26.3.  Quickly stepping through org-babel-python--send-string, it
> appears to get stuck in the accept-process-output call.
>
> Using an Emacs built from master, there's no hang on my end either.  If
> you'd find it useful, I can try to bisect that to a specific Emacs
> commit tomorrow.
>
> The Emacs 26.3 hang goes away after installing your patch that restores
> the use of a marker.

Thanks Kyle and Nowayman for checking the patch.

I was able to reproduce the problem on emacs-26.3 (but not 27.1). I'll
be more diligent testing on those versions in future.

I pushed an updated version of my patch to master in 939cf16bc. It
solves the problem for me on emacs-26.3. No need to bisect Emacs (but
thanks for offering!). But do let me know if you encounter any more
problems with this.

Best,
Jack

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Kyle Meyer <kyle@kyleam.com> writes:

> As I mentioned in the message linked upstream, it doesn't happen 
> when
> running just test-org/org-read-date 
> (BTEST_RE='test-org/org-read-date'),
> so there's some sort of interaction between tests.

Sorry, I was a little late getting to this, and it seems to be 
resolved- but this was my experience as well.
Running just that test didn't seem to fail.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

Hi No Wayman,

I pushed 4f49ebb6d, a small variant of your initial patch,
which pass the test fine by checking whether the variables
are bound outside or not, ignoring them if not.

Thanks again for the fix!

-- 
 Bastien

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Bastien <bzg@gnu.org> writes:

> Hi No Wayman,
>
> I pushed 4f49ebb6d, a small variant of your initial patch,
> which pass the test fine by checking whether the variables
> are bound outside or not, ignoring them if not.
>
> Thanks again for the fix!

Sounds good to me! Thanks, Bastien.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[Bastien]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Hi No Wayman,
>>
>> I pushed 4f49ebb6d, a small variant of your initial patch,
>> which pass the test fine by checking whether the variables
>> are bound outside or not, ignoring them if not.
>>
>> Thanks again for the fix!
>
> Sounds good to me! Thanks, Bastien.

Well, back to square one -- the "fix" breaks setting the end time 
of an existing schedule timestamp for me with.

I reverted it as 771c66f79.

-- 
 Bastien

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

Bastien <bzg@gnu.org> writes:

> No Wayman <iarchivedmywholelife@gmail.com> writes:
>
> Well, back to square one -- the "fix" breaks setting the end 
> time
> of an existing schedule timestamp for me with.
>
> I reverted it as 771c66f79.

Unfortunate, but I see the same breakage with the patch. I stepped
through trying to set and end time of an existing schedule 
timestamp
and I think the problem may be in `org-read-date-analyze'.
The sections commented:

>    ;; Check if a time range is given as a duration

>    ;; Check if there is a time range

Might be where the problem is.
Not 100% on why yet as I don't have deep knowledge of how this 
function is
supposed to work and it is a little opaque. I'll keep researching
and see if I can come up with anything.

Re: [PATCH] org-add-planning-info: respect caller's given time [9.3.7 (release_9.3.7-716-g3d4876 @ /home/n/.emacs.d/straight/build/org/)]

[No Wayman]

> I'll keep researching and see if I can come up with anything.

This is particularly difficult.
When instrumenting `org-add-planning-info` with edebug the 
resulting timestamp always has its time portion (given and/or 
end-time) ignored.
Tried it with both my personal setup and emacs -q. No luck.
I'm more likely to hack around this than figure out a proper 
patch,
though I still consider it a bug that org-schedule is unable to be 
informed by org-time-given and org-end-time-given I originally 
described.