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

Add two more insert at point commands #339

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

Conversation

dalanicolai
Copy link
Contributor

Inserting journal style headlines easily can be useful in non-journal buffers
too. Especially when using any 'org-notifications' package (e.g.
org-wild-notifier). This PR
adds two more insert-at-point variants for the org-journal-new-entry and the
org-journal-new-scheduled-entry commands.

I have a project where I would like to use journal style headlines outside a journal file. This small adjustment enables the option to insert journal style headlines at point. I thought it might be useful sometimes for other user too. If you think the extra command is too "exclusive" to be added by default then it is no problem if you remove it. But then it would still be nice to keep the optional `insert-at-point` argument.
Currently the command inserts the time preceding the TODO. This PR implements
fixes (non-desctructively), but it assumes that bastibe#335 and bastibe#337 get merged
first.
@dalanicolai dalanicolai force-pushed the add-two-more-insert-at-point-commands branch from d22ce07 to fad099a Compare February 25, 2021 23:58
@bastibe
Copy link
Owner

bastibe commented Mar 6, 2021

Should 412328e be part of this PR? It seems that the rest of the PR would work without it.

@dalanicolai
Copy link
Contributor Author

I remember that I wrote somewhere that I kept the three commits separate, to make it more clear what happened in each commit as each commit adds a different feature (however I can not find back where I wrote that, so maybe I deleted that accidentally). But indeed each 'later' commit includes the changes in the earlier commits. So you could squash the commits, but I think the commits deserve to be separate.

@bastibe
Copy link
Owner

bastibe commented Mar 9, 2021

The other two commits seem fairly straightforward. But I don't quite understand the purpose or mechanism in 412328e. It seems that it used to insert TODO, and now it will insert SCHEDULED. Is this intentional?

Inserting journal style headlines easily can be useful in non-journal buffers
too. Especially when using any 'org-notifications' package (e.g.
[org-wild-notifier](https://github.com/akhramov/org-wild-notifier.el)).
@dalanicolai dalanicolai force-pushed the add-two-more-insert-at-point-commands branch from fad099a to 3236a83 Compare March 9, 2021 15:24
@dalanicolai
Copy link
Contributor Author

Yeah sorry for the messy PR's. I thought I will purposely create multiple PR's so that you can pick the ones you like, while I can explain things in little chunks (well actually I don't remember exactly why I created multiple PR's, but I do remember that I purposely created multiple commits).

Anyway, to come back to your question... the PR does not remove the TODO, but instead it places it in front of the timestamp as is required for an org TODO (I wrote that in the message of PR #338, but I understand this message got lost in the mess).

Here are two screenshots after using the org-journal-new-scheduled-entry command before and after the commit:
BEFORE
Screenshot_20210309_163225

AFTER
Screenshot_20210309_163510

Additionally, it adds the scheduled stamp, first because the command is called insert-new-SCHEDULED-entry, and second because the scheduled stamp enables to set custom alerts when using the org-wild-notifier package. (This requirement is only implicitly documented in its documentation, but I documented it explicitly for the Spacemacs org layer.

@bastibe bastibe requested a review from casch-at March 10, 2021 13:19
@bastibe
Copy link
Owner

bastibe commented Mar 10, 2021

Your example certainly makes sense. On my machine, org-journal-new-scheduled-entry does not put a time after the bullet. Is this a customization on your end?

@dalanicolai
Copy link
Contributor Author

dalanicolai commented Mar 10, 2021

Do you mean before merging the PR or after merging the PR? (I assume you mean before merging the PR)...
Well, looking at the source code, it sounds strange anyway. It looks like there should get a time inserted after the bullet by the org-journal--insert-entry function. It should also get inserted there when you just use org-journal-new-entry (which is also what org-journal-new-scheduled-entry uses). I don't see any reason why it would not get printed (unless you call it with a universal-argument).

I have tested it also on a fresh install, fresh Emacs with fresh org-journal, and then the time does get inserted when calling org-journal-new-scheduled-entry.

Using magit-blame I do find that Christian worked on that part on 26/01/2021. Are you sure you are using the latest version?

@bastibe
Copy link
Owner

bastibe commented Mar 13, 2021

Are you sure you are using the latest version?

I might not be. Good call.

@bastibe
Copy link
Owner

bastibe commented Mar 21, 2021

Sorry for the delay. Frankly, I feel unqualified for signing off this PR, as I don't understand it (haven't had time to dive deeply enough into it, and haven't been maintaining org-journal for a while).

Is this super important to you, or are you fine with waiting for @cslux to comment? I can probably take some time next week if it's important.

@dalanicolai
Copy link
Contributor Author

No problem at all! I can also comment right at the chunks in the commits to help you figuring to save your time, but to make those commits relevant it would be nice if you could shortly hint on which aspect is not clear (the motivation or the implementation, or maybe it is a more specific thing. You could also add comments/questions right in the commits). Also, are you familiar with edebug? If not, it is really easy to start using it and can be really helpful for making sense what code is doing (just a recommendation). Finally, I wrote short motivations for each commit in their respective commit messages.

Also, if it needs more time to get merged it is no problem at all (just please indicate soon or not so soon :). I can simply make Spacemacs use my fork for the time being. Anyway, thanks for looking after it.

@bastibe
Copy link
Owner

bastibe commented Apr 21, 2021

I've had a talk with @cslux, and he said he'd be back in a couple of weeks. I'll defer this PR until then if that's OK with you.

@dalanicolai
Copy link
Contributor Author

No problem! Thanks for the update... in the meantime I will just point Spacemacs to my fork.

@casch-at casch-at added this to the 2.3.0 milestone 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.

3 participants