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

Fixes part of issue #91 regarding nested headings #96

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

Conversation

fakedrake
Copy link

Fixed the combination of some breaking assumptions of
org-transclusion-remove in the presence of nested transclusioins:

  • the character right before the beginning of a transclusion is not
    necessarily an adjacent transclusion. It might be the parent
    transclusion.

  • Using insert-before-markers to re-insert the #+transclude: ...
    element loses information about the text properties present before
    inserting the cloned text. This is still prone to bugs but using
    insert-before-markers-and-inherit instead avoids the common case
    errors.

Fixed the combination of some breaking assumptions of
`org-transclusion-remove` in the presence of nested transclusioins:

- the character right before the beginning of a transclusion is not
  necessarily an adjacent transclusion. It might be the parent
  transclusion.

- Using `insert-before-markers` to re-insert the `#+transclude: ...`
  element loses information about the text properties present before
  inserting the cloned text. This is still prone to bugs but using
  `insert-before-markers-and-inherit` instead avoids the common case
  errors.
When inserting the #+transclusion: ... tag on transclusion removal we
want to inherit the transclusion properties when it was nested but not
when it is adjasent to another transclusion.
@nobiot
Copy link
Owner

nobiot commented Oct 9, 2021

Thank you. I will need to spend a bit more time.

I didn't know that it was possible to manually call the add function on a transclusion within another transclusion. That itself is a bug -- discrepancy from my original design and should have been prevented in the first place.

Your PR tried to make this bug legitimate.

This might work but... From my own experience of developing the remove function, it is an area that would require a significant amount of regression if I were to accept your PR.

I hesitate to take it as it is for this reason but at the same time it might be a fruitful avenue to pursue...

@fakedrake
Copy link
Author

fakedrake commented Oct 9, 2021

Since yesterday I gave rubbed against at least a few of the problems you imply. The biggest problem seems to be that when the transcluded text is removed and the transclude keyword is insert-before-marks, it's hard to tell which marks will be moved. Consider the following case:

we have 3 consecutive transclusions:

#+transclude: A
#+transclude: B
#+transcude: C

then all of them are expanded.

then we try to remove B: The transcluded text is removed but before the keyword is reinserted we are in the unfortunate position to have the beginning of C and the end of A to coincide. Then there is no way to insert the keyword of B without doing something complex to distinguish between A and C.

I have a branch that has a few features I need that you may not necessarily want to add to master (see end). In there I mitigated this issue by including two newlines at the end of the transcluded text that do not get any of the overlays except the read-only ones. When a transclusion is removed the newlines are also removed. This way the end of a transclusion never coincides with the beginning of the next one.

Features you may not want:

  • When transcluding an org file that has a #+title, the transcluded text is wrapped in a heading and headings internally are demoted.
  • A variable org-transclusion-enable-recursive-add controls whether org-transclusion-add-all transcludes recursively
  • Removing works as one might expect with nested transclusions.

nobiot added a commit that referenced this pull request Oct 16, 2021
transclusion inside another. This should not happen by design.

This commit add `org-transclusion-check-add` and in it a new check has been
added to prevent this case. Currently it checks the following two cases:

Case 1. Element at point is NOT #+transclude:
Case 2. #+transclude inside another transclusion"

The PR #96 attempts go beyond this design. It currently seems to complicate
the cases for `org-transclusion-remove`. We might revist the case for
nesting (a special case of transclusion within another transclusion);
however, I am not prepared to go through regression and unaticipated issues
with this major change of assumptions.
@nobiot
Copy link
Owner

nobiot commented Oct 16, 2021

For now, I am putting a new check to prevent any case of transclusion in another with the new commit 330d761.
Sorry that I am not prepared to spend time and efforts for regression at the moment.

I believe with this change this following case won't happen, and hopefully every character (if it's in a trascnslusion) right before the beginning of a transclusion is always an adjacent transclusion. No nesting, so no parent transclusion.

the character right before the beginning of a transclusion is not
necessarily an adjacent transclusion. It might be the parent
transclusion.

In addition...
The current design deals with the case of three consecutive transclusion; I cannot reproduce it (this is one of the things I tested so I was hoping it won't happen).

#+transclude: A
#+transclude: B
#+transcude: C

Screenshot from 2021-10-13 11-23-43

@devcarbon-com
Copy link
Contributor

Just wanted to pitch in to say that I'm quite interested in recursion as well. Thanks for looking into this!

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

3 participants