bug#41897: 28.0.50; JavaScript comment filling with mhtml-mode

[Alan Mackenzie <acm@muc.de>]

Hello again, Simen.

On Sat, Jun 20, 2020 at 20:27:22 +0200, Simen Heggestøyl wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > The patch below fixes both these errors, and seems to allow your test
> > case to work.

> Yes, this seems to fix it in both emacs-27 and master, thanks.

> However in emacs-27 with the patch applied I now get a strange
> side-effect: After filling the comment, mhtml-mode seems to lose track
> of where the JavaScript portion of the buffer is. Please see the
> attached image. The position where point is should still be recognized
> as HTML+JS, but it's recognized as plain HTML+ instead after
> filling. This doesn't happen on master.

Yes.  mhtml-mode keeps track of the position of the JavaScript bit by
applying text-properties.  In certain circumstances, when point is at
(point-min), it fails to re-apply those properties after removing them.
This was happening in the filling, which uses narrowing.  I think I've
got this fixed, now.  (See patch below.)

> > You haven't said what version of Emacs you're running.

> I put the version number in the bug subject but forgot to mention it in
> the description, sorry about that.

No, my apologies: I completely missed it in the subject line.  I'm happy
that you're working in master, and not Emacs 26.  :-)

Anyhow, I've made significant progress on the problem.  As well as the
problem above, I've repaired a few defects with auto-fill-mode.  It's
more or less working.  But M-q isn't completely working.  If a buffer
contains the long line of your test case, followed by another line, M-q
doesn't fill them together, leaving the short line untouched.

I've had problems with auto-fill-function, and the current state in the
patch involves mhtml-mode assuming auto-fill-function is enabled.  This
obviously needs fixing, but that's the way it is for now.  Sorry.

There's still a bug with M-q where it sometimes throws an error about a
search being "on the wrong side of point".  I haven't had time to
investigate this, yet.

So here's the current patch for the problem.  Please install it on
master after removing yesterday's patch.  From the .../emacs/lisp
directory, it should work with $ patch -p2 < this-email.  (Or, maybe
there's a git command to do it more directly.)



diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 04b449ecd2..f5b6a4c260 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -4570,7 +4570,7 @@ js-mode
 
   ;; Comments
   (setq-local comment-start "// ")
-  (setq-local comment-start-skip "\\(//+\\|/\\*+\\)\\s *")
+  (setq-local comment-start-skip "\\(?://+\\|/\\*+\\)\\s *")
   (setq-local comment-end "")
   (setq-local fill-paragraph-function #'js-fill-paragraph)
   (setq-local normal-auto-fill-function #'js-do-auto-fill)
diff --git a/lisp/textmodes/mhtml-mode.el b/lisp/textmodes/mhtml-mode.el
index 1ae07c0a30..c5970cbcd1 100644
--- a/lisp/textmodes/mhtml-mode.el
+++ b/lisp/textmodes/mhtml-mode.el
@@ -73,15 +73,18 @@ mhtml-tag-relative-indent
 
 (defconst mhtml--crucial-variable-prefix
   (regexp-opt '("comment-" "uncomment-" "electric-indent-"
-                "smie-" "forward-sexp-function" "completion-" "major-mode"))
+                "smie-" "forward-sexp-function" "completion-" "major-mode"
+                "adaptive-fill-" "fill-" "auto-fill-function"))
   "Regexp matching the prefix of \"crucial\" buffer-locals we want to capture.")
 
 (defconst mhtml--variable-prefix
   (regexp-opt '("font-lock-" "indent-line-function"))
   "Regexp matching the prefix of buffer-locals we want to capture.")
 
-(defun mhtml--construct-submode (mode &rest args)
-  "A wrapper for make-mhtml--submode that computes the buffer-local variables."
+(defun mhtml--construct-submode (mode crucial-localized localized &rest args)
+  "A wrapper for make-mhtml--submode that computes the buffer-local variables.
+CRUCIAL-LOCALIZED and LOCALIZED are lists of symbols which must
+be considered crucial-local and local variables respectively."
   (let ((captured-locals nil)
         (crucial-captured-locals nil)
         (submode (apply #'make-mhtml--submode args)))
@@ -94,6 +97,16 @@ mhtml--construct-submode
       (unless (variable-binding-locus 'font-lock-fontify-region-function)
         (setq-local font-lock-fontify-region-function
                     #'font-lock-default-fontify-region))
+      ;; Get the values from global variables which might become buffer local.
+      (dolist (iter crucial-localized)
+        (let ((sym (if (symbolp iter) iter (car iter)))
+              (val (symbol-value (if (symbolp iter) iter (cdr iter)))))
+          (push (cons sym val) crucial-captured-locals)))
+      (dolist (iter localized)
+        (let ((sym (if (symbolp iter) iter (car iter)))
+              (val (symbol-value (if (symbolp iter) iter (cdr iter)))))
+          (push (cons sym val) captured-locals)))
+
       (dolist (iter (buffer-local-variables))
         (when (string-match mhtml--crucial-variable-prefix
                             (symbol-name (car iter)))
@@ -119,6 +132,8 @@ mhtml--mark-crucial-buffer-locals
 
 (defconst mhtml--css-submode
   (mhtml--construct-submode 'css-mode
+                            '((auto-fill-function . normal-auto-fill-function))
+                            nil
                             :name "CSS"
                             :end-tag "</style>"
                             :syntax-table css-mode-syntax-table
@@ -127,6 +142,8 @@ mhtml--css-submode
 
 (defconst mhtml--js-submode
   (mhtml--construct-submode 'js-mode
+                            '((auto-fill-function . normal-auto-fill-function))
+                            nil
                             :name "JS"
                             :end-tag "</script>"
                             :syntax-table js-mode-syntax-table
@@ -202,12 +219,16 @@ mhtml--stashed-crucial-variables
 (defun mhtml--stash-crucial-variables ()
   (setq mhtml--stashed-crucial-variables
         (mapcar (lambda (sym)
-                  (cons sym (buffer-local-value sym (current-buffer))))
+                  (if (boundp sym)
+                      (cons sym (buffer-local-value sym (current-buffer)))
+                    sym))
                 mhtml--crucial-variables)))
 
 (defun mhtml--map-in-crucial-variables (alist)
   (dolist (item alist)
-    (set (car item) (cdr item))))
+    (if (symbolp item)
+        (makunbound item)
+      (set (car item) (cdr item)))))
 
 (defun mhtml--pre-command ()
   (let ((submode (get-text-property (point) 'mhtml-submode)))
@@ -255,17 +276,14 @@ mhtml--syntax-propertize
    sgml-syntax-propertize-rules))
 
 (defun mhtml-syntax-propertize (start end)
-  ;; First remove our special settings from the affected text.  They
-  ;; will be re-applied as needed.
-  (remove-list-of-text-properties start end
-                                  '(syntax-table local-map mhtml-submode))
-  (goto-char start)
-  ;; Be sure to look back one character, because START won't yet have
-  ;; been propertized.
-  (unless (bobp)
-    (let ((submode (get-text-property (1- (point)) 'mhtml-submode)))
-      (if submode
-          (mhtml--syntax-propertize-submode submode end))))
+  (let ((submode (get-text-property start 'mhtml-submode)))
+    ;; First remove our special settings from the affected text.  They
+    ;; will be re-applied as needed.
+    (remove-list-of-text-properties start end
+                                    '(syntax-table local-map mhtml-submode))
+    (goto-char start)
+    (if submode
+        (mhtml--syntax-propertize-submode submode end)))
   (sgml-syntax-propertize (point) end mhtml--syntax-propertize))
 
 (defun mhtml-indent-line ()


> -- Simen

-- 
Alan Mackenzie (Nuremberg, Germany).