chg: Make hooks to set up extensions removable #261

[nobiot]

@josephmturner

Wishing you a joyous New Year! Apologies for taking for so long to get back to you (I wanted to fix the infinite loop issue first...). I believe this is a good-enough fix. No breaking changes for users, and minor-modes can co-exist. The user manual need to be updated (especially how to use extensions -- on a separate issue, I am working on Transient-based menu UI in the background.

Your -html.el is also adjusted based on this new way. Let me know what you think.

---

Added the following utility functions to support the move

• org-transclusion-extension-functions-add-or-remove
• org-transclusion-extension-set-a-hook-functions

Modified function org-transclusion-load-extensions-maybe so that setting org-transclusion-extensions with the customize facilities will automatically activate the corresponding minor mode for each extension.

Use of customize and org-transclusion-extensions is optional. Users can ignore these and use extensions by loading and activating minor modes in their init.el file.

So, this below should work (each minor mode function has the autoload cookie, so the library should not have to be explicitly loaded).

(with-eval-after-load 'org-transclusion
    (org-transclusion-indent-mode +1))

Adding to the list and requiring the library like this below did not use to be
necessary...

(with-eval-after-load 'org-transclusion
     (add-to-list 'org-transclusion-extensions 'org-transclusion-indent-mode)
     (require 'org-transclusion-indent-mode))

The Customizing facility would load the active extension library with
require.... But I think this was never intuitive.

[josephmturner]

Thank you!! A Joyous New Year to You too!!

I pushed another commit to the fix/extension-to-minor-mode branch. I don't see it show up in this PR, though (?). It uses the cl-loop macro to simplify the logic for adding and removing hooks. This is a stylistic choice; of course, feel free to ignore it if you like. If you keep it, then 5573584 should be pruned.

org-transclusion-load-extensions-maybe now assumes that each extension's mode is named EXTENSION-NAME-mode, which may not always be the case. For a related example, in @phikal's setup.el, we found that a feature's mode map can be tricky to predict, and so the relevant feature which attempted to do so was deprecated. (thread 1, thread 2).

I suggest removing the call to org-transclusion-load-extensions-maybe from the bottom of org-transclusion.el, since the primary concern in #261 is that extra extensions not be loaded merely by requiring org-transclusion.el. This is a backward-incompatible change, and AFAIK there's no mechanism in Emacs to indicate deprecation of the current behavior. What do you think?

Thank you!

Joseph

[nobiot]

@josephmturner, thank you for your comment. I could not work on this thread on the weekend (I would like to sit down and take a decent amount of focus time to respond). I will come back in due course — just wanted to make a quick comment and thank you.

[nobiot]

Thank you for your engagement and patience—I really appreciate it. I've been reflecting on this exchange and your suggestions. I believe our discussion touches on two main points:

1. The concern raised in Make hooks to set up extensions removable #261, which points to the fact extra extensions are loaded merely by requiring org-transclusion.el.

2. To fix this problem, we will need to introduce a backward-incompatible change.

Point 1

I'd like to take a step back and examine the reason behind the concern (1). From my understanding , this stems from this coding convention outlined in (info "(elisp) Coding Conventions").

The first convention in that page states:

   • Simply loading a package should not change Emacs's editing
     behavior.  Include a command or commands to enable and disable the
     feature, or to invoke it.

     This convention is mandatory for any file that includes custom
     definitions.  If fixing such a file to follow this convention
     requires an incompatible change, go ahead and make the incompatible
     change; don't postpone it.

I agree with this convention: "Simply loading a package should not change Emacs's editing behavior." I interpret it say this:

Simply loading `org-transclusion.el` should not change Emac's
editing behavior for the user. Include a command or commands so
that the user can make conscious decision and enable and disable
the feature that changes the editing experience.

So..., the issue arises not from loading extensions per se, but from loading them in a way that unintentionally changes a user's editing experience. I believe this interpretation aligns with the convention—would you agree? If so, it suggests that both the previous and current behaviors do not violate this principle.

src-lines and font-lockextensions are turned on by default (intentional design). I believe most Org-transclusion users keep the defaults, so these options can be considered integral to the core feature set. With the src-lines the user would explicitly call org-transclusion-add or activate org-transclusion-mode. The font-lock library changes the font-lock for the link value part of the #+transclude: keyword. I believe most users would expect the link part be fontified in the same way as normal Org links.

The indent-mode extension is off by default. The user would need to enable it and then turn on org-indent-mode. For the html extension, you would know how it behaves as the author much better than I.

This is my view for the first point about extra extensions being loaded merely by requiring org-transclusion.el. If you agree with understanding of the issue, I think the change I have introduced in this PR is in line with Emacs Lisp coding convention as outlined in the manual.

Point 2

Now... maybe you don't agree with me on my interpretation of the coding convention; in this case, it may be better for me to stop here. ... but since I have got you wait for long, I will continue a little longer.

I've introduced a minor mode for each extension to align with the way currently common to similar to how vertico handles its extensions like vertico-unobtrusive-mode, vertico-grid-mode, vertico-indexed-mode and others.

org-transclusion-load-extensions-maybe now assumes that each extension's mode is named EXTENSION-NAME-mode, which may not always be the case.

This is true and I agree. I. plan to move away from this model for future extensions. This will be the last set of extensions activated via org-transclusion-extensions; subsequent extensions should simply be a library and introduce a minor mode(s) to enable / disable its features to the user, the same way as the vertico examples above. This way, I think we can avoid the issue of mismatching extension-names and mode-names, while maintaining backward compatibility.

I suggest removing the call to org-transclusion-load-extensions-maybe from the bottom of org-transclusion.el, since the primary concern in #261 is that extra extensions not be loaded merely by requiring org-transclusion.el.

Conclusion

I think keeping org-transclusion-load-extensions-maybe as is can maintain backward compatibility, while addressing the issue raised in #261.

I would love to hear your thoughts on my perspective.

[josephmturner]

Thank you, @nobiot! I think the changes you propose in this PR are very good.

If I understand what you're saying, some extensions (font-lock, src-lines) are considered part of the core package and should always be activated, while others (indent-mode, html, and external extensions, like hyperdrive-org-translusion) should not always be activated.

I think this distinction is already covered by the defcustom org-transclusion-extensions. Right?

Since org-transclusion-activate already calls org-transclusion-load-extensions-maybe, why do we need to call org-transclusion-load-extensions-maybe at the bottom of org-transclusion.el?

I think we should load extensions as lazily as possible, since org-transclusion might be required before the user actually needs to load any extensions. For example, the user might call org-transclusion-make-from-link or they might require org-transclusion in init.el, but until they call org-transclusion-activate, there's no need to load extensions.

Can we simply remove these few lines from org-transclusion.el?

;; Load extensions upon loading this file
(org-transclusion-load-extensions-maybe)

This will be the last set of extensions activated via org-transclusion-extensions

That sounds good.

Thank you!!

Joseph