nobiot / org-transclusion

Emacs package to enable transclusion with Org Mode
https://nobiot.github.io/org-transclusion/
GNU General Public License v3.0
917 stars 44 forks source link

Enhancement: Allow org block live sync #176

Closed devcarbon-com closed 1 year ago

devcarbon-com commented 1 year ago

Draft pull request. Preliminary manual testing has been successful.

Not sure when org started support for it, but it works with 9.7-pre, and hopefully 9.6 also.

devcarbon-com commented 1 year ago

This seems to work okay, but I feel like it's a bit hacky and solving the problem at the wrong location.

I think it would be better to add 2 more markers to the text properties in the case of org blocks.

Right now, we have org-transclusion-beg-mkr and org-transclusion-end-mkr that covers the whole range of the content added when the transclusion is expanded. But these do not distinguish between exact content, and modified content (IE. when we add #begin_src lang and #end_src).

One way to do this without having to change a lot of other code would be to add text properties specifying the orginal content range when we modify the src-content in org-transclusion-content-src-lines and then in org-transclusion-content-insert to extract them from content and add markers.

I have a proof-of-concept of this locally on my machine, but I'm not aware of how to best proceed when it comes to pull requests that become inter-dependent.

@nobiot Should I just add it as it's own request, and then merge this (#176) and #157 after it's accepted?

nobiot commented 1 year ago

@devcarbon-com Thank you for all this. I will need to find some time in my calendar for focus time to understand your proposal and questions. Please allow 1 or 2 weekends…

devcarbon-com commented 1 year ago

@nobiot Sure thing!

I'm happy to add a code example if you would like. Sometimes it's easier to read code than English :P.

nobiot commented 1 year ago

Sorry, I think I do not seem to have understood the intent of this PR. See below... I try starting live-sync with the test files below; no change of behaviour.

It will be great if you could provide some example files.

image

#test-live-sync.org
#+transclude: [[file:test-src.org::code]]
#test-src.org
#+name: code
#+begin_src emacs-lisp transclude file:simple.el :lines 1-10 -n  -r
         (save-excursion                 ;; (ref:sc)
            (goto-char (point-min))
            (message "executed."))     ;; (ref:jump)
#+end_src
devcarbon-com commented 1 year ago

Oh I had not tried transcluding code from another org file. I'd need to do some digging to figure out how to do that.

What I have been doing is like this:

Narrative.org

#+transclude: [[file:code.el]]  :src elisp

code.el

(defun hello ()
  (message "hello"))
nobiot commented 1 year ago

Oh, now I got it! All I said in this discussion thread is about transcluding a source code block from another Org file -- i.e. the source being another Org file.

The use case for this PR is transcluding a text file, so my comment there is not applicable -- I'm sure you figured that out.

I have tested the code lightly with my test python files (I should perhaps put into the repo as a common test file set). Your code change works extremely well as far as I could see -- I could even use org-edit-src when live-sync is on. Whoa.

The reason why I didn't let it work by this:

(when (string= "src" type)
  (user-error "No live sync for src-code block"))

is that I could not work out a reliable way to deal with the indentation -- the length of the source and transclusion must exactly the same -- I think you have figured out a way.

nobiot commented 1 year ago

There is one suggestion, if I may. I will write it directly in the review of the source code as well. Could you look into moving the change you made in function org-transclusion-live-sync-buffers-others-default to function org-transclusion-live-sync-buffers-src-lines in file org-transclusion-src-lines.el by any chance? This way:

  1. We can contain the change for "src" type transclusion to the function intended for the purppose
  2. Probably you can remove (if (org-in-src-block-p) clause (because "src" type always results in the source text file be in a src block

What do you think?

nobiot commented 1 year ago

@devcarbon-com one more thing before I forget. Apologies if I have already asked you this.

Org-transclusion is available in ELPA, which means contributors of more than 15 lines of code need to sign the FSF paperwork -- this outlined in this section of README. Have you done this? If not, would mind starting the process? It might take time (it seems to be manual exchange of emails -- in my case, I believe it took me a few weeks -- mostly waiting). But I think we can merge your PR as long as you have started the process and are willing to complete the paperwork.

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).

devcarbon-com commented 1 year ago

@nobiot Oh, I thought I just had to check the box, so I'm glad you brought it up!

Got it started now.

nobiot commented 1 year ago

Thank you!

devcarbon-com commented 1 year ago

So I can do it, but I would need to copy and modify the default function (not DRY) or extract out the common code to a helper function.

That's definitely doable, but I'm not sure how in elisp do that without it being rather verbose, so I wrote a macro that I personally use a lot, but I'm not sure it would be a good idea to use in production code. (I'm a elisp newbie).

Perhaps something like this already exists, but I haven't been able to find it if it does. It's basically the counter-part to let-alist:

(defmacro alist-of-let* (let-bindings &rest body)
  "This is the same as #'let*, except it return an alist of the bindings.
LET-BINDINGS and BODY are the same as in #'let*."
  `(let* ,let-bindings
     ,@body
     (list ,@(mapcar (lambda (binding)
                       (list #'cons
                             `',(car binding)
                             (car binding)))
                     let-bindings))))

So then you can write code like this:

(defun do-the-work ()
  (alist-of-let* ((a 1)
                  (b 2)
                  (c (- b a))))

(defun delegate-the-work ()
  (let-alist (do-the-work)
    (+ .a .b .c)))

(delegate-the-work) ;=> 4

If that seems acceptable to you I'll go that route, or if you know of a better way I'm all ears :).

nobiot commented 1 year ago

That's definitely doable, but I'm not sure how in elisp do that without it being rather verbose, so I wrote a macro that I personally use a lot, but I'm not sure it would be a good idea to use in production code. (I'm a elisp newbie).

I have an idea; a simple one without a macro. Let me get back to you as a suggestion. Please give me until the end of the week (I am hoping to have some time tomorrow but if it fails I need a bit more time).

nobiot commented 1 year ago

@devcarbon-com I have no idea how I can send a commit to your PR so I opened a new one as continuation #178. Your original commits are there, too. Could you take a look?

To be honest, I don't understand the "dot" syntax you show here: (+ .a .b .c) and how the macro works (I'm not familiar with let-alist.

My suggestion is simple and probably crude compared to your macro-based way: it is just to run the default function to create the src-ov and tc-ov, and then move tc-ov's location based on your offset logic... Kinda avoids "repeating yourself" issue, I guess. But not sophisticated. Perhaps this is sufficient?

Let me know of your thoughts... At least, functionally it seems to works in the same way as yours.

(defun do-the-work ()
  (alist-of-let* ((a 1)
                  (b 2)
                  (c (- b a))))

(defun delegate-the-work ()
  (let-alist (do-the-work)
    (+ .a .b .c)))

(delegate-the-work) ;=> 4