Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notifications: show notifications for all active dates #505

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Maltimore
Copy link
Contributor

This seems like an obvious/easy fix, but maybe (probably) there was a reason it wasn't done like this to begin with?
It works for me so far.
Closes #374

@Maltimore Maltimore marked this pull request as draft February 20, 2023 15:06
@Maltimore
Copy link
Contributor Author

Maltimore commented Feb 20, 2023

I just noticed that this still doesn't work for repeater dates. (But I guess this PR is already an improvement over the status quo).

@jgollenz
Copy link
Contributor

I think this should be a configurable option. This way it doesn't break current behavior. What do you think?

@dtvillafana
Copy link

I have made the same change in my fork, and the reasoning in the linked issue is sound and I agree with it, but from a software product standpoint it would make sense to make it a configurable option for backwards compatibility and extensibility.

@Maltimore
Copy link
Contributor Author

Hey, sorry for not getting back to you @jgollenz, and thanks for your interest @dtvillafana. I'm still using this patch as well.
Making it configurable is of course always an option but to be honest I see absolutely no reason not to include active dates in the agenda, I mean otherwise what's the difference to passive dates?

@Maltimore
Copy link
Contributor Author

Sorry, forget what I said. This is not about the agenda but about notifications. Yes I suppose one could have a config option whether to show notifications for active timestamps. I still wonder what the reasoning is to not show notifications for timestamped events by default, but to show them for SCHEDULED and DEADLINE.

@jgollenz
Copy link
Contributor

I took a look at the docs and actually the behavior you expect should already be happening, so strictly speaking this is a bug fix 😉 It says agenda tasks notifications there, so you are absolutely right. We also already have options for this, albeit the other way round:

deadline_reminder = true,
scheduled_reminder = true,

I tested your change and it looks good to me. When you mark the PR as ready I can merge it 👍

@jgollenz
Copy link
Contributor

jgollenz commented Mar 25, 2023

it would make sense to make it a configurable option for backwards compatibility and extensibility

@dtvillafana in this case behavior was actually not in line with what the docs say, so I don't consider it necessary to add an option. If need for it exists, we can think of introducing it but for now this is a bug-fix.

@Maltimore Maltimore force-pushed the notifications_all_active_dates branch from 90436c9 to 789a267 Compare March 26, 2023 12:00
@Maltimore Maltimore marked this pull request as ready for review March 26, 2023 12:10
@Maltimore
Copy link
Contributor Author

Ok, ready for review!

@jgollenz
Copy link
Contributor

Did some more checks and it seems that

deadline_reminder = false,
scheduled_reminder = false,

have no effect any more. I get notifications for both DEADLINE and SCHEDULED tasks even when they should be turned off. Can you confirm this?

@Maltimore
Copy link
Contributor Author

Ah yes, you're right!
Sorry, I was too eager to just fix my own use-case and didn't think about these settings. I'll have to make this PR much more through. I'll switch it back to draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include plain-timestamps in notifications
3 participants