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

yasnippet modifies buffer while inhibit-modification-hooks is t #756

Closed
phillord opened this issue Dec 4, 2016 · 8 comments · Fixed by #788
Closed

yasnippet modifies buffer while inhibit-modification-hooks is t #756

phillord opened this issue Dec 4, 2016 · 8 comments · Fixed by #788
Labels

Comments

@phillord
Copy link

phillord commented Dec 4, 2016

The mechanism that yasnippet uses for changing fields means that it changes the buffer while inhibit-modification-hooks is t, if I am reading the code correctly. This means that any other code which depends on these hooks to identify change fails. This happens in the yas--on-field-overlay-modification function.

As an example, it causes a bad interaction with lentic mode phillord/lentic#51.

I'm not sure what the obvious fix is -- probably using post-command-hook rather than "modification-hook" to trigger the changes,

@npostavs
Copy link
Collaborator

npostavs commented Dec 4, 2016

I think it's to avoid recursion. Possibly we should bind yas--inhibit-overlay-hooks instead of inhibit-modification-hooks, i.e., only stop ourselves from recursive modification, not others.

@phillord
Copy link
Author

phillord commented Dec 4, 2016

AFAICT, the let binding of inhibit-modification-hooks makes no difference, as it happens (I already tried removing it!). Emacs itself appears to be already binding this variable to t when it calling the overlay modification hooks. I am checking up that this is the case for sure.

Ironically, let binding inhibit-modification-hooks to nil solves my problem.

@npostavs
Copy link
Collaborator

npostavs commented Dec 4, 2016

Ah. Probably only recording the changes in the after-change function and then performing them in the post command hook is the best solution then.

@phillord
Copy link
Author

phillord commented Dec 4, 2016

Yes. You might not even need the modification-hook, to be honest -- you can use post-command-hook to check for the overlay.

I've submitted a bug to Emacs also. (bug#25111). Let's wait till and see whether there is a better solution in the response to that; switching inhibit-modification-hooks to nil is clearly the simplest solution, although the documentation says to do precisely the opposite.

If not, I'll be happy to send a PR using post-command-hook.

@npostavs
Copy link
Collaborator

npostavs commented Dec 4, 2016

You might not even need the modification-hook, to be honest -- you can use post-command-hook to check for the overlay.

It's mostly an optimization to avoid unneeded mirror updates, but I think modification-hook would still be necessary for the yas--skip-and-clear-field-p part.

@npostavs
Copy link
Collaborator

Probably only recording the changes in the after-change function and then performing them in the post command hook is the best solution then.

Turns out this breaks things, it's hard to say why exactly. Does the following work okay? (I tried installing lentic, but I couldn't figure out how to run the example)

--- w/yasnippet.el
+++ i/yasnippet.el
@@ -3533,9 +3533,10 @@ (defun yas--on-field-overlay-modification (overlay after? beg end &optional leng
   (unless (or (not after?)
               yas--inhibit-overlay-hooks
               (not (overlayp yas--active-field-overlay)) ; Avoid Emacs bug #21824.
               (yas--undo-in-progress))
-    (let* ((inhibit-modification-hooks t)
+    (let* ((inhibit-modification-hooks nil)
+           (yas--inhibit-overlay-hooks t)
            (field (overlay-get overlay 'yas--field))
            (snippet (overlay-get yas--active-field-overlay 'yas--snippet)))
       (save-match-data
         (yas--letenv (yas--snippet-expand-env snippet)

@phillord
Copy link
Author

Apologies for the tardy reply. I missed the original notification.Yes, this patch does work.

Sorry, you had problems with getting lentic to work. Easiest way is to open a buffer, and do M-x lentic-mode-create-new-view-in-selected-window. You get what looks like an indirect buffer with the same contents as the original file, but with different point and in which you can change mode freely.

@npostavs
Copy link
Collaborator

Thanks for confirming.

M-x lentic-mode-create-new-view-in-selected-window.

Ah thanks, I can reproduce the original bug with the old yasnippet version following this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants