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

Make calendar integration optional #378

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dppdppd
Copy link

@dppdppd dppdppd commented Jan 20, 2022

something like this?

@dppdppd dppdppd changed the title made calendar integration optional made calendar integration optional. closes #375 Jan 20, 2022
org-journal.el Outdated
(add-hook 'calendar-today-visible-hook 'org-journal-mark-entries)
;;;###autoload
(add-hook 'calendar-today-invisible-hook 'org-journal-mark-entries)
(if org-journal-enable-calendar-integration
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this breaks the autoloads, which are necessary for making org-journal work in the calendar if it hasn't been invoked yet. So the branch probably needs to happen inside org-journal-mark-entries instead.

org-journal.el Outdated
(define-key calendar-mode-map (kbd "j s y") 'org-journal-search-calendar-year)))
do (define-key org-journal-mode-map (funcall key-func org-journal-prefix-key key) command))))

(if org-journal-enable-calendar-integration
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it break anything if these remained enabled? Or perhaps it would make sense to only disable the first five. I could see a use case for org-journal-new-date-entry and the org-journal-search-* functions even with the calendar highlights disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@bastibe
Copy link
Owner

bastibe commented Jan 23, 2022

Something like this will work. Thank you! But I added two comments that we should discuss or fix.

Copy link
Collaborator

@xeruf xeruf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have not manually tested, but looks good

@casch-at casch-at added this to the 2.3.0 milestone Dec 7, 2023
@casch-at
Copy link
Collaborator

Fixes #375

@casch-at casch-at changed the title made calendar integration optional. closes #375 Make calendar integration optional. closes #375 Feb 18, 2024
@casch-at casch-at changed the title Make calendar integration optional. closes #375 Make calendar integration optional. Fix #375 Feb 18, 2024
@casch-at casch-at changed the title Make calendar integration optional. Fix #375 Make calendar integration optional Feb 18, 2024
@casch-at
Copy link
Collaborator

@dppdppd Is it ok if I push to this branch, and do a squash merge afterwards?

@casch-at casch-at self-assigned this Feb 18, 2024
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.

4 participants