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

refactor: captures #613

Merged
merged 11 commits into from
Oct 26, 2023
Merged

Conversation

danilshvalov
Copy link
Contributor

This pull request was aimed at improving the extensibility of orgmode captures. At first, I just wanted to add an option to append and prepend empty lines after refiling. It turned out to be a bit more complicated than I thought, so I decided to do a little refactoring. I am ready to change anything else that needed to be changed in orgmode captures, if necessary.

P.S. empty lines were also added.

@danilshvalov
Copy link
Contributor Author

danilshvalov commented Sep 23, 2023

I also added subtemplates to reduce duplication. Now the user can specify the subtemplates as follows:

{
  e = {
    description = 'Event',
    subtemplates = {
      r = {
        description = 'recurring',
        template = '** %?\n %T',
        target = '~/org/calendar.org',
        headline = 'recurring'
      },
      o = {
        description = 'one-time',
        template = '** %?\n %T',
        target = '~/org/calendar.org',
        headline = 'one-time'
      },
    },
  },
}

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
Generally looks ok, but I don't think we need empty_lines option since it can be achieved with \n, unless I'm missing something.

DOCS.md Outdated
* `template` (`string|string[]`) — body of the template that will be used when creating capture
* `target` (`string?`) — name of the file to which the capture content will be added. If the target is not specified, the content will be added to the [`org_default_notes_file`](#orgdefaultnotesfile) file
* `headline` (`string?`) — title of the headline after which the capture content will be added. If no headline is specified, the content will be appended to the end of the file
* `empty_lines` (`table?`):
Copy link
Member

Choose a reason for hiding this comment

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

This can be achieved with \n in the template, there's no need to define it. For example:

    j = {
      description = 'Journal',
      -- Add 3 empty lines before and 3 lines after
      template = '\n\n\n* TODO %?\n\n\n',
      target = '~/sync/org/journal.org'
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment why it doesn't work.

DOCS.md Show resolved Hide resolved
@danilshvalov
Copy link
Contributor Author

Generally looks ok, but I don't think we need empty_lines option since it can be achieved with \n, unless I'm missing something.

This doesn't work if you add text to the headline. Sample template:

{
  description = "Test",
  template = "\n\n** text with newlines",
  target = "~/org/tmp.org",
  headline = "Test",
}

Result after double use of the template:

* Test
** text with newlines
** text with newlines

With empty lines it works:

{
  description = "Test",
  template = "** text with newlines",
  target = "~/org/tmp.org",
  headline = "Test",
  empty_lines = {
    before = 2,
  },
}

Result:

* Test


** text with newlines


** text with newlines

@kristijanhusak
Copy link
Member

This is happening because of this line https://github.com/nvim-orgmode/orgmode/blob/master/lua/orgmode/capture/init.lua#L154

This trims the empty lines before. We need to have this headline in order to be able to properly promote/demote depending on destination.

We can handle this in 2 ways:

  1. Leave it as it is without the empty_lines, which would still allow adding empty lines after with \n
  2. Refactor the capturing to pass around the whole capture file (https://github.com/nvim-orgmode/orgmode/blob/master/lua/orgmode/capture/init.lua#L151) instead of only the first headline. Then handle the item(s) accordingly where they are handled now and add the spacing.

I'd prefer the first since it's a smaller change, but you can also try the 2nd option if you want.
I'm not exactly sure what's the use case for empty lines before.
If you have a template that adds the lines after, and if you use it consistently, you should have proper spacing before and after when you add the headline.

@danilshvalov
Copy link
Contributor Author

Okay, I'll see what I can do. Another advantage of empty_lines is that it does not affect the user's buffer. If I add newlines at the beginning of the content, then the capture buffer starts to look like this:



** text with newlines

Emacs orgmode also has the option empty_lines: https://orgmode.org/manual/Template-elements.html

@kristijanhusak
Copy link
Member

Emacs orgmode also has the option empty_lines: orgmode.org/manual/Template-elements.html

Ah, ok, wasn't aware of this. I thought this is not part of Emacs. We generally try to follow as much as possible.
Could you just move it to properties key as they show it in the document?

@danilshvalov
Copy link
Contributor Author

Emacs orgmode also has the option empty_lines: orgmode.org/manual/Template-elements.html

Ah, ok, wasn't aware of this. I thought this is not part of Emacs. We generally try to follow as much as possible. Could you just move it to properties key as they show it in the document?

Done

@danilshvalov
Copy link
Contributor Author

I have made the following changes:

  1. Now the user can specify a number instead of a table for the empty_lines field (like in Emacs).
  2. Now the buffer empty lines before and after are deleted (like in Emacs).

@kristijanhusak
Copy link
Member

There are a lot of duplicate commits. Can you please squash them?

@danilshvalov
Copy link
Contributor Author

There are a lot of duplicate commits. Can you please squash them?

Done

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

We are almost there. Let's just tweak the empty lines thing as I explained in the comment and I think it's good to go.

lua/orgmode/capture/init.lua Show resolved Hide resolved
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Everything looks great now, thanks!

lua/orgmode/capture/init.lua Show resolved Hide resolved
@kristijanhusak
Copy link
Member

@danilshvalov just please rebase from master since there are some conflicts.

@danilshvalov
Copy link
Contributor Author

@danilshvalov just please rebase from master since there are some conflicts.

Sorry for the long delay. Done

@kristijanhusak kristijanhusak merged commit 938f9af into nvim-orgmode:master Oct 26, 2023
4 checks passed
@kristijanhusak
Copy link
Member

Thanks!

SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
* refactor: captures

* refactor: make headline detached from template

* feat: subtemplates

* refactor: move `empty_lines` to `properties`

* feat: remove buffer empty lines

* fix: properly place capture without headline

* tests: add tests for org captures

* fix: template validation

* chore: remove old tests
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.

2 participants