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

[WIP]Support more :lines formats and internal links #190

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

llcc
Copy link

@llcc llcc commented May 24, 2023

This PR adds support for a more flexible :lines format and org-mode internal links.

I hope this is helpful! Let me know if you have any other questions.

@llcc llcc changed the title Support more :lines formats Support more :lines formats and internal links May 24, 2023
@llcc llcc marked this pull request as ready for review May 24, 2023 01:50
@llcc llcc changed the title Support more :lines formats and internal links [WIP]Support more :lines formats and internal links May 24, 2023
@nobiot
Copy link
Owner

nobiot commented May 24, 2023

@llcc Thank you for the initiative, and I really appreciate the effort.

The intention is great. Let me share two things and make some requests in this context:

  1. There is a branch (feature/org-internal-link) that explores support internal links more generically

    coderef, custom-id, fuzzy, and radio are internal link types.
    Test radio and fuzzy so far.

    I have encountered some issues in the live-sync feature. Have you encountered anything like it? (the location of the region gets dislocated by 1 point). I'd love to see this tested more thoroughly and any issue, if any reproducible, resolved.

  2. There is another branch (PR Refactor/src lines #187) trying to refactor function org-transclusion-content-range-of-lines to separate the logic for thing-at-point and lines

    I feel that function org-transclusion-content-range-of-lines has become a bit too complex for me and us (wider community) to continue to maintain it easily for long-term.

    Your PR touches it (right?) too. I would like to get this refactorinng done first before moving on to enhanching it any further. @devcarbon-com has kindly agreed to work on it when he has a bit more time in his hand. I'd appreciate if you could also look at it if you feel up for it.


That's the current context and requests on my end.

For the PR, I feel that you have very personal, concrete use case -- how do you use the current src-lines features and what motivate you to enhance it?

To me :lines -5,7-8,4,9- looks really complex. Appearance of complexity is not an issue but I am sure there is a motivation and concrete issue that you would like to resolve -- would you be able to share your background a bit more by any chance?

@nobiot
Copy link
Owner

nobiot commented May 24, 2023

One more ting... Org-transclusion is part of ELPA, this means there is paperwork involved for me to be allowed to accept any contribution. Below is an excerpt of the relevant part from README. Would you be willing to start the process? I don't need you to have completed the process to accept your PR; but the paperwork is required if PRs of more than 15 lines of code to be accepted into the codebase.

Org-transclusion is part of GNU ELPA and thus copyrighted by the Free Software Foundation (FSF). This means that anyone who is making a substantive code contribution will need to “assign the copyright for your contributions to the FSF so that they can be included in GNU Emacs” (Org Mode website).

@llcc
Copy link
Author

llcc commented May 25, 2023

  1. There is a branch (feature/org-internal-link) that explores support internal links more generically

Okay.. I was previously unaware of the feature/org-internal-link branch. It appears to support more link types than my PR 👍 .
Hmm.. But I was unable to get it work with #+transclude: [[table-name]] for #+NAME: table-name. Is this not yet supported?
The error is caused by

(if (not (string= type "id")) (setq buf (find-file-noselect path))

I have encountered some issues in the live-sync feature. Have you encountered anything like it? (the location of the region gets dislocated by 1 point). I'd love to see this tested more thoroughly and any issue, if any reproducible, resolved.

Although I have not yet had the opportunity to test it, I personally rarely have the need to live edit in the same buffer. I'll take a look on it later.

  1. There is another branch (PR Refactor/src lines #187) trying to refactor function org-transclusion-content-range-of-lines to separate the logic for thing-at-point and lines
    Your PR touches it (right?) too. I would like to get this refactorinng done first before moving on to enhanching it any further.

Agree on this. I'll wait util your refactor is complete.

For the PR, I feel that you have very personal, concrete use case -- how do you use the current src-lines features and what motivate you to enhance it?

I frequently need to transclude parts of tables or lists into other buffers. For tables, I usually need the header, a horizontal line, and some rows of the table, as well as the last #+tblfm: line. For lists, I usually insert a <<target>> in the parent list, then transclude random one of its sub-lists many times. In these cases, the current :lines range is not enough.

In my PR, I have addressed the limitation by extending the :lines range to consider the bounds of the elements. For example, :lines 1- now transcludes all the way to the table end, instead of to the end of the source buffer. This change makes it easier to transclude parts of tables and lists, or other types of elements.

@llcc
Copy link
Author

llcc commented May 25, 2023

One more ting... Org-transclusion is part of ELPA, this means there is paperwork involved for me to be allowed to accept any contribution.

I have signed the document. I can send you a copy if you need.

@nobiot
Copy link
Owner

nobiot commented May 26, 2023

Thank you for your detail. No need to send me a copy of the FSF paperwork; your statement is enough for me.

I frequently need to transclude parts of tables or lists into other buffers. For tables, I usually need the header, a horizontal line, and some rows of the table, as well as the last #+tblfm: line. For lists, I usually insert a <<target>> in the parent list, then transclude random one of its sub-lists many times. In these cases, the current :lines range is not enough.

From this I understand that this PR is support tables in Org file. Is this correct? Then we should take a different approach. I will follow up on this over the weekend or next week.

Very briefly:

  • source-lines.el is designed for non-Org text files
  • I would suggest a dedicated properry like table-rows
  • There is another approach to do this

I will come back with more concrete details.

Edit: I only did a quick experiment for tables only but the same principle should apply to lists (to be verified).

@nobiot nobiot mentioned this pull request May 28, 2023
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.

None yet

2 participants