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

Would you consider modularising the extensions? #238

Closed
akashpal-21 opened this issue Apr 18, 2024 · 7 comments
Closed

Would you consider modularising the extensions? #238

akashpal-21 opened this issue Apr 18, 2024 · 7 comments

Comments

@akashpal-21
Copy link

akashpal-21 commented Apr 18, 2024

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
Copy link
Owner

nobiot commented Apr 18, 2024

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
Copy link
Owner

nobiot commented Apr 20, 2024

@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
Copy link
Author

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
Copy link
Author

akashpal-21 commented Apr 21, 2024

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
Copy link
Author

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
Copy link
Owner

nobiot commented Apr 21, 2024

Thank you!

@nobiot
Copy link
Owner

nobiot commented Apr 21, 2024

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

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

No branches or pull requests

2 participants