nobiot / org-transclusion

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

Would you consider modularising the extensions? #238

Closed akashpal-21 closed 3 months ago

akashpal-21 commented 3 months ago

Currently org-transclusion.el cannot be simply evaluated and tested --

org-transclusion/org-transclusion-font-lock.el org-transclusion/org-transclusion-src-lines.el org-transclusion/text-clone.el

The above must be evaluated before org-transclusion can finally be evaluated. Ripping apart the requirements is more cumbersome than just evaluating all the extensions together. Are all of them necessary for the core?

org-transclusion/org-transclusion.el

Great package though, do you have some optimisations in the back of your mind I can try, or you can hint for me to explore --

nobiot commented 3 months ago

Hi @akashpal-21 Thank you for your note.

I will come back to this thread. I want to mention two big issues: performance and infinite loop. If you could help us resolve either/both, it would be fantastic. (Can't remember the issue numbers, let me look later).

Re your point:

Currently org-transclusion.el cannot be simply evaluated and tested

How about something like this below?

(use-package org-transclusion
  :load-path ("~/src/org-transclusion")
  :custom
  (org-transclusion-extensions nil))

Refer to org-transclusion-set-extensions and org-transclusion-load-extensions-maybe...

nobiot commented 3 months ago

@akashpal-21

Here are what I think are two biggest issues.

For the performance issue, I was playing with a caching mechanism with a hash table. I could not get it to work to improve the performance, so I put it on hold.

For the infinite loop, I was able to reliably reproduce the issue back then. When I go back to it now, I can only do so more intermittently. I may think of a band-aid patch for it. In any case, I need to spend more time on it.


With regard to your original suggestion about "modularizing", in my view, the features in this package (.el files) are already modular. I tried emulating what Daniel Medler has done with vertico. In it, the main feature is provided by vertico.el and the extensions, by each .el under extensions folder.

As a user, you are instructed to enable (require) extension features as you like. In the Extensions section of README, he provides an example of configuration with using use-package like this:

(use-package vertico-directory
  :after vertico
  :ensure nil
[my omission for the rest of configuration]
)

I find it clean and easy enough for users like me, who know something about Emacs and Emacs Lisp. What I've added to org-transclusion is a way for the user to add extension features via customize with some features active by default. This way, my intention is to make it a little more accessible for those who do not understand Emacs Lisp.

image

I think that it is a side-effect of this user convenience that a developer (like yourself) is required to do something a little more. This little more should be simply adding a source files' directory to load-path. Or this:

(use-package org-transclusion
  :load-path ("~/src/org-transclusion"))

Perhaps I should consider a "CONTRIBUTE" notice as a guide for contribution to this package (this can be a PR, too :)).

text-clone is different to the extensions. It is separate because I have been intending to adjust it and send to Emacs upstream as a patch (I have also been asked to do so); I haven't done this yet. The feature is meant to be an extension to the built-in text-clone-create function in subr.el. This is also something I would like some help with.

I think I am still learning how to make it clear on a public repo like this where the author (me) needs help.

akashpal-21 commented 3 months ago

Understandable -- it is not much of a problem -- I think you have obviously made the correct decision to include some features as active by default -- although I dont use use-package I rely on my own set of protocols to add packages -- so before adding packages to my distribution I like to do a test run and just evaluating the base package is an easier thing, but its not a big deal, it would be detrimental to end users to totally isolate the base package and turn off all features by default.

I will look into those issues, and if I can make anything better I will report back to you. Closing this issue page.

akashpal-21 commented 3 months ago

I checked the cpu-profiler -- the stuck up for large files is the insert operation,

917  52% - command-execute
         758  43%  - funcall-interactively
         758  43%   - execute-extended-command
         758  43%    - command-execute
         758  43%     - funcall-interactively
         746  42%      - org-transclusion-add
         746  42%       - if
         746  42%        - progn
         746  42%         - let*
         744  42%          - if
         744  42%           - org-transclusion-add-callback
         744  42%            - let
         744  42%             - if
         744  42%              - let
         660  37%               - let*
         660  37%                - unwind-protect
         660  37%                 - progn
         660  37%                  - if
         656  37%                   - save-excursion
         656  37%                    - org-transclusion-content-insert
         656  37%                     - let*
         420  23%                      + insert

It is unclear to me how can we optimise the process -- the holdup seems to be because of inherent emacs protocols and doesn't seem to me I can do much about it -- without rewriting the entire insert function 😆

akashpal-21 commented 3 months ago

also regarding the infinite loop - I only triggered it once, I dont know myself how to retrigger it, I will keep my eyes open -- but I have tried most of the combinations I regularly use -- nothing triggers it.

nobiot commented 3 months ago

Thank you!

nobiot commented 3 months ago

also regarding the infinite loop - I only triggered it once, I dont know myself how to retrigger it,

The linked issue has a detailed step-by-step repro. It would be good if you can also see if it is reproducible :)