Account for org-extend-today-until in org-journal-open-current-file

[SqrtMinusOne]

This makes org-journal-open-current-file respect org-extend-today-until. The variable is used the same way as in org-today.

Yes, I had to invoke that function at 3 AM on Monday to discover that bug.

[bastibe]

I can't figure out how this works. From what I can gleam from the source code, the other parts of the code that use org-extend-today-until seem to take a different approach: https://github.com/bastibe/org-journal/blob/master/org-journal.el#L802

Could you comment on why this works? time-since seems to return a time-delta, not a point-in-time, but org-journal--get-entry-path appears to require a point-in-time?

[SqrtMinusOne]

(time-since arg) is (time-subtract nil arg) for non-string values of ARG. time-subtract takes two time values and returns a time value, where nil is the same as (current-time).

An integer is a possible time value, see (elisp) Time of Day. So, (time-subtract nil number-of-seconds) moves the time value by number-of-seconds to the past.

E.g. (time-subtract nil 0) is the current time, see

(format-time-string "%FT%T%z" (time-subtract nil 0))

Or, to make it a year ago:

(format-time-string "%FT%T%z" (time-subtract nil (* 60 60 24 365)))

Thus (time-since (* 3600 org-extend-today-until)) is the time value org-extend-today-until hours ago.

As I said, this is how org-today works, see https://github.com/bzg/org-mode/blob/098f0815916fcfd88b24d7a0a842c3b294b4383d/lisp/org.el#L5061.

time-to-days also accepts a time value ((elisp) Time Calculations). Ironically, days-to-time is not the inverse of time-to-days, so I decided not to reuse org-today.

As for your link, I've changed the code as follows and it seems to work the same:

(let* ((time (time-since (* 3600 org-extend-today-until)))
       (org-extend-today-until-active-p
        (> (time-to-days nil)
           (time-to-days time)))
       ...)
  ...)

I can commit that as well.

[bastibe]

I see! Thank you very much for the clarification, this is clever!

I think I would prefer (time-subtract (current-time) (* 3600 org-extend-today-until)), as it makes the intent clearer (moving the time). It seems to me that the intent of time-since is instead for measuring a time difference, but maybe that's just a misconception on my part.

[SqrtMinusOne]

Maybe you're right, changed that.

As for me, I often look at the source when I see a function I don't understand, and the nature of time-since is immediately clear from doing that. But perhaps I wrote too much elisp.

[bastibe]

As for me, I often look at the source when I see a function I don't understand

I tend to look at the documentation only, which is probably where my confusion comes from. But I'll also freely admit that my elisp is quite rusty these days.

The Pull request looks good to me. Is it ready to merge from your end?

[SqrtMinusOne]

Is it ready to merge from your end?

It is.

The errors in CI look like they weren't introduced by me.

[casch-at]

Thank you @SqrtMinusOne

Yes, I had to invoke that function at 3 AM on Monday to discover that bug.
That's unfortunate. That must have been very "annoying".

I see a couple of other things we could improve as well.

1. We should write (a) test(s) for this "rare" occurring bug. If we or upstream make a mistake @SqrtMinusOne very likely is going to create another PR to fix it again :-D.
2. We should avoid the magic number 3600 or add a function which makes it obvious.
3. There are 3 places where org-extend-today-until is used, a re-factoring might be the better solution in the long run
   

   org-journal/org-journal.el

   Line 805 in 129dede

   (org-extend-today-until-active-p (and (not time) (< (nth 2 now) org-extend-today-until)))

   
   

   org-journal/org-journal.el

   Line 1271 in 129dede

   (* 3600 org-extend-today-until)))))

   
   

   org-journal/org-journal.el

   Line 1619 in 129dede

   (when (< (nth 2 (decode-time (current-time))) org-extend-today-until)

Anyway, it would be perfectly fine to merge it, and create a new issue to resolve one or the other point a little :-) later.

[casch-at]

Ah, regarding the CI, I know about it, I know how to fix it quickly, but I don't like my "dirty hotfix"🙈

[SqrtMinusOne]

I can do 1 and 3 in this PR a bit later, no big deal.

As for 2 - I can write it as (* 60 60), but I feel like it's immediately obvious that 3600 is the number of seconds in an hour, especially in the context of time manipulation 🙂

Org Mode devs do not seem to have a problem with hardcoding it, although I admit that core Emacs and ELPA often have lower code quality than what's required by MELPA. Hyperbole is still not admitted to MELPA...

But anyway, I really don't like something such as this:

(defconst org-journal--seconds-in-hour (* 60 60)
  "The number of seconds in an hour.")

[bastibe]

Perhaps a (let ((seconds-per-hour 3600))) would do the trick? Or simply add a comment? Personally, I find 3600 acceptable, as it is clear from context that this converts hours to seconds. But if it's confusing to others, it's probably worth being explicit.

[casch-at]

As for 2 - I can write it as (* 60 60), but I feel like it's immediately obvious that 3600 is the number of seconds in an hour, especially in the context of time manipulation 🙂

Never mind. But we should ensure that org-extend-today-until works as expected in all the locations it is used, aka write more tests. I'm currently working on moving to Eask.

I'll create a new task for the other two points (1 and 3).