Use org-ql to match org headings to be alerted.

[rickyson96]

Hi! I've implemented a way to fetch only the related headings for alerting.

I kind of cheat a little using org-ql, but I think it worked out pretty well.

It should fix #27, though I'm not sure if I have handled all use cases.

I try to implement it in a way that's backward compatible, so some customization might not be in place right now.

We could also potentially make the org-alert-check customizable too, but I've put this off for now since I think the current implementation is good enough.

To use this new feature, we need to add:

;; or setopt for fancier option :)
(setq org-alert-match-string 'org-ql
      ;; use non-greedy regex to match the `from' time.
      org-alert-time-match-string "\\(?:SCHEDULED\\|DEADLINE\\):.*?<.*?\\([0-9]\\{2\\}:[0-9]\\{2\\}\\).*>")

[ntBre]

Wow thanks for this! I didn't know about org-ql. I need to take a slightly closer look at how it works, but it seems really promising. Is there any reason not to use org-ql exclusively? (ah besides backwards compatibility) If we're already adding the dependency, I wouldn't mind making this the default/only behavior. org-alert-match-string has always felt very brittle to me anyway.

[rickyson96]

If you're okay with the breaking changes, I think we can remove much of the old logic and depend solely on org-ql (note the changes on run-at-time too because it enables the cronjob behaviour).

[rickyson96]

I've updated the implementation to only use org-ql.

The problem is that I've borked the REMINDERN property. I've done a little bit of testing to see if we could do it with org-ql, but haven't get to implement it on the PR yet. :D

[ntBre]

Let's not change any default values, especially if they aren't related to the PR. 0 is a truthy value here and will cause (if org-alert-notify-after-event-cutoff to be true but then to disable alerting after the event.

[rickyson96]

yeah, you're right.

I don't quite understand this before and assume that the default behaviour is 0 (especially since I mostly use plain timestamp and didn't really archive my scheduled events).

[ntBre]

I also didn't know about rx. While it's pretty neat, I would slightly prefer using the normal string syntax. At least it seems like evaluating this won't show it to the user. When I evaluate it, I get

"<.*?\\([0-9]\\{2\\}:[0-9]\\{2\\}\\).*[>-]"

which is almost the same but not quite as the one in the current code:

<.*\\([0-9]\\{2\\}:[0-9]\\{2\\}\\).*>"

Is there a reason for the new ? (non-greedy?) and the trailing -?

[rickyson96]

This is, again, tied to my use case.
I use org-gcal to sync my events, and I mostly got the <2024-09-27 Fri 10:00-11:00> format.

But I think it's reasonable default, since it should handle the default format, and it makes more sense to alert using the earlier time when we actually schedule timestamps with range formats.

Now that I think more about it, the regex should consume the rest without needing the trailing -, so I'll remove it. But still, I think the non-greedy syntax is a better default.

---

Regarding rx: I'm so used to read the rx format instead of regex, to the point I actually have downloaded xr package to revert string regex back to rx format. Its just my preference leaking into others package 😂.

I'll change this back to the string syntax, and let me know if you want the non-greedy changes on a separate PR instead. I'll happy to do so!

---

oh and just for more context: I do deliberately remove the \\(?:SCHEDULED\\|DEADLINE\\):.* part since it's the point of this PR

[ntBre]

Not a big deal, and I can clean these up myself, but I'd definitely prefer not to have whitespace changes in unrelated parts of the PR.

[rickyson96]

I tried to limit these, but I think this one slipped out.

[ntBre]

Thanks again for doing this! I've added a few superficial comments, mostly trying to avoid changes unrelated to adding org-ql. I probably won't get to a fuller review until this weekend, but besides these minor issues it looks very promising.

I'm not necessarily against these other changes (except changing the default after-event behavior), but I would rather separate them from the main task at hand to make this easier to review.

[Remillard]

Just was running into this very problem -- largely I do not use Scheduled or Deadline except for a few significant things, but meeting notices are something I very much would like to get a warning for. I tried to follow the discussion but in the end I'm not entirely sure I know how to set this up, aside from looking and installing the org-ql package.

Are there some examples of putting this into practice? It might also be an interesting exercise to mark an agenda item with a property that makes it remindable (to distinguish from birthdays and other sundry times that don't need a 10 minute reminder).

[ntBre]

This is still an open pull request that has not been merged into the version of org-alert available on Melpa. You could give it a try by installing the version from this PR branch (https://github.com/rickyson96/org-alert/tree/master), but I'm not quite sure how to take advantage of the new org-ql changes either. I've been dragging my heels on merging this because I'm not super enthusiastic about taking on more dependencies, and I need to familiarize myself with org-ql myself to review this fully.

mark an agenda item with a property that makes it remindable

This is also an interesting suggestion, but from the previous discussion, I think this PR also makes it harder to access properties in general. Again, I think it'll take some work for me to feel comfortable with org-ql and resolve these points.

I'm still somewhat optimistic that org-ql will both simplify the code and add some of these handy features, but I haven't had a chance to look closely enough yet.

[ntBre]

@rickyson96 Sorry for the delay on this. I've come back around on switching to org-ql, and I've just merged a PR adding more tests and some CI. Could you rebase or merge master here and clean up the git conflicts? Based on the previous discussion, I expect at least the REMINDERN tests to fail, but hopefully the others will still pass.

No rush on this, obviously, and happy holidays! And if I left you hanging too long, I'm also happy to take it from here. Thanks again for bringing org-ql to my attention either way!

[ntBre]

Ah tests are failing because org-ql isn't installed. Can you extend ci.sh with the new packages that need to be added:

org-alert/ci.sh

Lines 5 to 7 in 0bc04ce

	emacs -batch -f package-initialize \
	--eval '(add-to-list (quote package-archives) (quote ("melpa" . "http://melpa.org/packages/")))' \
	--eval '(use-package alert :ensure t)'

I'm picturing something like this:

 emacs -batch -f package-initialize \ 
 	  --eval '(add-to-list (quote package-archives) (quote ("melpa" . "http://melpa.org/packages/")))' \ 
 	  --eval '(use-package alert :ensure t)' \
 	  --eval '(use-package org-ql :ensure t)'

I guess a line for ts also? I'd make the change myself, but I don't think I can push to your branch.

[rickyson96]

Hey @ntBre, I've also not been able to take on this until recently.
Last time I left this, I've made sure to let the REMINDERN works, so hopefully I can catch everything here.

currently I'm still figuring out running ERT on my machine though, and seems like it fails even with the dependencies. I'll add those to ci files too, but it'll probably take a while to resolve this.

[ntBre]

Sounds good, no rush at all. On my machine, I just run make test in the test subdirectory, but I'm not sure how fragile this test setup is. I have a feeling there must be a better approach, but I was just happy to get something running today.

I'm curious to play with org-ql too, so I'll help to resolve the test failures once we get CI running!

[rickyson96]

updated the ci dependencies.

I see, I don't notice the Makefile there 😂

One thing that I realized is if I load the test directly to my emacs session, I can run M-x ert, but it breaks my emacs perceived time. It has something to do with the TZ env that's being set in the test. Not sure if there's a better way out, though.

[ntBre]

Oh weird, I was wary of messing with TZ, but I thought wrapping it in with-environment-variables instead of setting it directly would help.

I'm debugging the test failures now, starting with the remindern test. It looks like if I remove the (let ((org-alert-notify-after-event-cutoff 60)) line, the test passes, so it looks like an issue with the after-event-cutoff part of the query.

Something weird is going on with the check-alert-some test because it passes when I run it alone (with -eval '(ert-run-tests-batch-and-exit "check-alert-some")' as the last line in the Makefile) but fails when run with the other tests.

The caching section of the org-ql readme makes me wonder if we're running into an issue with the cache for check-alert-some. I think the query would be identical between check-alert-none and check-alert-some, with the only change being us overriding current-time, so that hitting the cache would make sense to me. I was able to get around this by making a separate org file (plain2.org) for that test but not by using set-file-times.

However, this makes me a little worried about using org-ql in general. It will usually be the case that the agenda file has not changed, so I think this will mean the queries always return the same values. The cache section in the README makes it sound like it's not yet possible to invalidate the cache easily too.

Just for a concrete example, even after the plain2.org "fix", I added two more calls to org-alert-check to the check-alert-some test:

(ert-deftest check-alert-some ()
  (with-test-org "plain2.org"
    (with-current-time (25704 52957 0 0) ; 9:45:01
      (org-alert-check)
      (org-alert-check)
      (org-alert-check)
      (should (= (length test-alert-notifications) 3)))))

But this fails with 1 notification received instead of 3, again presumably because of this caching. This works as expected on the current master branch.

[ntBre]

I guess we can just use something like this to empty the cache:

(let ((org-ql-cache (make-hash-table)))
  (org-ql-select ...))

As suggested here, so we don't have to give up on org-ql entirely.

[rickyson96]

huh thats weird, I've never noticed it during my daily use.

I used this snippet to interactively test using org-ql, changing the scheduled time in test.org file to few minutes ahead of now.

(let ((org-directory ".")
      (org-agenda-files '("test.org")))
  (org-alert-check))

If I eval it multiple times, I'm able to get the alert multiple times. I've also changed it to setq-ing the variables and run org-alert-check interactively, it still produces the same result.

Does the cache only happens because we're running it as a test (thus having the buffer to point the cache to)?

[ntBre]

Hmm, you're right, I get multiple notifications with your snippet too. Maybe I was too quick to blame the cache.

However, changing check-alert-some to disable the cache does make it pass:

(ert-deftest check-alert-some ()
  (let ((org-ql-cache (make-hash-table)))
    (with-test-org "plain.org"
      (with-current-time (25704 52957 0 0) ; 9:45:01
        (org-alert-check)
        (should (= (length test-alert-notifications) 1))))))

I'd probably lean toward doing this in org-alert-check both to pass the test and just in case the cache can act up in real scenarios too.

[rickyson96]

I've added the cache invalidation and other stuff, but the default test is still failing.
One hypothesis that I have is that we only check the time for the alert, thus if the time is after the scheduled time, but it's actually past event, we wouldn't get the alert.

I'm not sure whether this is from master branch too, but one way out of this is to also check the day on org-alert--check-time.

[ntBre]

Well it looks like all the tests are passing here. I just want to resolve the org-alert-notify-after-event-cutoff rather than comment out that part of the tests and then this is good to go.

I still need to read more about the available org-ql query types to suggest anything here, but my guess is that we're missing some kind of conditional on whether org-alert-notify-after-event-cutoff is defined in org-alert-check. If it's undefined, we don't want an upper bound on when to notify (ie always send late notifications, even hours or days later). I'm not honestly sure how useful/preferable this behavior is, but it is the current behavior.

Btw, it appears to be safe to comment the cutoff back in for check-alert-none-remindern. Only check-alert-some-remindern is failing for me locally with the comments removed.

This is fine to leave out of the scope of this PR, but I also feel like (hope?) we could use org-ql more heavily. Does it provide any of the parsing functionality that we're currently doing manually? I'm dreaming of something SQL-like such as

SELECT headline, time, properties 
FROM org-agenda
WHERE time > org-alert-notify-cutoff AND time < org-alert-notify-after-event-cutoff

Basically if org-ql could handle all of the selecting behavior, we would just map over the results to send notifications for each headline: time pair, and we could delete everything like org-alert--parse-entry, org-alert--check-time, etc.