implement mmm-indent-region

[stephe-ada-guru]

I finally got around to working on mmm-indent-region. Initial patch attached.

One issue is whether we actually need indent-dont-widen, and whether this works for other modes. I've tested with wisitoken-grammar-mode.
mmm-indent-region.patch.txt

[dgutov]

Could you describe the benefits? Performance? Does it change the current behavior?

[stephe-ada-guru]

I take it back ; apparently I did not test carefully enough. I have code like:
declaration

  : PERCENT token_keyword_non_grammar IDENTIFIER declaration_item_list
    %((wisi-statement-action [1 statement-start])
      (wisi-name-action 3)
      (wisi-face-apply-action
       [1 nil font-lock-constant-face
          2 nil font-lock-keyword-face
          3 nil font-lock-function-name-face])
      (wisi-indent-action [nil nil nil (wisi-hanging 4 2)]))%

where %()% delimits an elisp submode. I want to keep the initial indent. But when the buffer is narrowed to the submode region, "(wisi-statement-action" is in column zero. In addition, indenting these lines screws up following overlays; they need to be updated after indenting each region.

I'll see if I can fix this.

[stephe-ada-guru]

Fixed both problems: updated patch attached. I suspect there should be a user option for "preserve indent of first line".
mmm-indent-region.patch.txt

[dgutov]

He Stephen,

I think you didn't answer my questions. What is the overall goal of this patch?

First of all, indent-region-function shouldn't contradict indent-line-function. If it creates a different indentation behavior, that's a problem. The only thing it's allowed to improve is performance.

Second, if we do choose to improve mmm-indent-line, we should first try to see whether it improves the behavior in different configurations. And hopefully doesn't break others which indent okay currently.

Also see #75 for a similar, yet unfinished, discussion.

[stephe-ada-guru]

Dmitry Gutov <notifications@github.com> writes:

I think you didn't answer my questions. What is the overall goal of this patch?

To allow indent-region-function to work in an mmm-mode buffer.

First of all, `indent-region-function` shouldn't contradict `indent-line-function`. If it creates a different indentation behavior, that's a problem. The only thing it's allowed to improve is performance.

Exactly. The _default_ indent-region-function delegates to indent-line, and mmm-mode sets indent-line-function, which is why indent-region works in many modes. But without this patch, an indent-region-function provided by a major mode does not respect the submodes. So for example in wisitoken-grammar-mode (for bison-like grammar files), with this code: ``` declaration : PERCENT token_keyword_non_grammar IDENTIFIER declaration_item_list %((wisi-statement-action [1 statement-start]) (wisi-name-action 3) (wisi-face-apply-action [1 nil font-lock-constant-face 2 nil font-lock-keyword-face 3 nil font-lock-function-name-face]) (wisi-indent-action [nil nil nil (wisi-hanging 4 2)]))% ; ``` where %()% delimits an elisp submode, indent-region-function indents the elisp as if it were in wisitoken-grammar-mode, which is wrong. With the patch, the elisp code is correctly indented by lisp-indent-region, the rest of the buffer by wisitoken-grammar-mode.

Second, if we do choose to improve `mmm-indent-line`, we should first try to see whether it improves the behavior in different configurations. And hopefully doesn't break others which indent okay currently.

This patch does not alter mmm-indent-line, although that probably should set use-markers to t, now that I think of it. It only affects behavior in modes that set indent-region-function, which are currently broken (so wisitoken-grammar-mode may be the only such mode!). In those modes, it provides correct behavior. There are still issues around preserving the indent of the first line; I have not yet made that work for an Ada submode in wisitoken-grammar-mode.

Also see #75 for a similar, yet unfinished, discussion.

That provides mmm-indent-region as a user command, not as a setting for indent-region-function, so it seems to addressing a different issue. That code is not affected by this patch

…

-- -- Stephe

[dgutov]

The default indent-region-function delegates to indent-line, and
mmm-mode sets indent-line-function, which is why indent-region works in
many modes.

But without this patch, an indent-region-function provided by a major
mode does not respect the submodes.

Thank you, that makes sense.

I have yet to look into this patch in detail, but what about the fact that mmm-indent-line does not do any narrowing?

Your patch includes a call to narrow-to-region inside mmm-indent-region-list. Won't that create different behavior by default?

[stephe-ada-guru]

Dmitry Gutov <notifications@github.com> writes:

> The _default_ indent-region-function delegates to indent-line, and mmm-mode sets indent-line-function, which is why indent-region works in many modes. > But without this patch, an indent-region-function provided by a major mode does not respect the submodes. Thank you, that makes sense. I have yet to look into this patch in detail, but what about the fact that `mmm-indent-line` does not do any narrowing? Your patch includes a call to `narrow-to-region` inside `mmm-indent-region-list`. Won't that create different behavior by default?

I was copying mmm-mmm-fontify-region-list, which does narrowing. I tested with narrow-to-region commented out; that is actually better, because it elminates the need to preserve the indent of the first line. It also removes the need for indent-dont-widen. I'll test some more, and post a new patch.

…

-- -- Stephe

[dgutov]

I was copying mmm-mmm-fontify-region-list, which does narrowing.

Right. But font-lock-keywords and indent-line-function don't have to be 100% consistent. indent-region-function and indent-line-function do.

We actually have an alternative one (mmm-indent-line-function cat be set to mmm-indent-line-narrowed), but the default indent-region-function should be compatible with mmm-indent-line.

I tested with narrow-to-region commented out; that is actually better,

It's probably better in some cases, yet worse in others. That's why, ultimately, major modes can override mmm-indent-line-function too (html-erb-mode sets it to mmm-erb-indent-line, you can take a look). But it's better to be conservative for the default behavior.

[dgutov]

I'll test some more, and post a new patch.

Looking forward to it.

[stephe-ada-guru]

Here's an updated patch; it's been working well.
mmm-indent-region.patch.txt

[dgutov]

Hi Stephen, I see you've added a mmm-indent-narrow property, which enabled/disables narrowing.

Even so, the default indent-line-function doesn't use it, right? Doesn't that lead to an incompatibility? We'll need to resolve it, one way or another.

[stephe-ada-guru]

Dmitry Gutov <notifications@github.com> writes:

Hi Stephen, I see you've added a `mmm-indent-narrow` property, which enabled/disables narrowing. Even so, the default `indent-line-function` doesn't use it, right? Doesn't that lead to an incompatibility? We'll need to resolve it, one way or another.

Right, I did not mess with indent-line-function. Attached is an updated patch that changes mmm-indent-line to use mmm-indent-narrow.

…

-- -- Stephe diff --git a/mmm-mode.el b/mmm-mode.el index d2d4e9b..595360e 100644 --- a/mmm-mode.el +++ b/mmm-mode.el @@ -178,6 +178,7 @@ available through M-x customize-group RET mmm." (set (make-local-variable 'syntax-propertize-function) #'mmm-syntax-propertize-function) (set (make-local-variable 'indent-line-function) mmm-indent-line-function) + (set (make-local-variable 'indent-region-function) mmm-indent-region-function) (setq mmm-mode t) (condition-case err (mmm-apply-all) diff --git a/mmm-region.el b/mmm-region.el index a0c5b4f..416ad35 100644 --- a/mmm-region.el +++ b/mmm-region.el @@ -523,7 +523,8 @@ is non-nil, don't quit if the info is already there." (put mode 'mmm-syntax-propertize-function (and (boundp 'syntax-propertize-function) syntax-propertize-function)) - (put mode 'mmm-indent-line-function indent-line-function)) + (put mode 'mmm-indent-line-function indent-line-function) + (put mode 'mmm-indent-region-function indent-region-function)) ;; Get variables (setq global-vars (mmm-get-locals 'global) buffer-vars (mmm-get-locals 'buffer) @@ -724,15 +725,23 @@ region and mode for the previous position." ;; with font-lock, but they aren't used anywhere else, so we might as ;; well have them close. -(defun mmm-submode-changes-in (start stop) +(defun mmm-submode-changes-in (start stop &optional use-markers) "Return a list of all submode-change positions from START to STOP. -The list is sorted in order of increasing buffer position." - (let ((changes (sort (cl-remove-duplicates - (mmm-mapcan (lambda (ovl) - `(,(overlay-start ovl) - ,(overlay-end ovl))) - (mmm-overlays-overlapping start stop))) - #'<))) +The list is sorted in order of increasing buffer position. If +USE-MARKERS is non-nil, result contains markers instead of +integers; required when indenting." + (let ((changes + (sort + (cl-remove-duplicates + (mmm-mapcan (lambda (ovl) + `(,(if use-markers + (copy-marker (overlay-start ovl)) + (overlay-start ovl)) + ,(if use-markers + (copy-marker (overlay-end ovl)) + (overlay-end ovl)))) + (mmm-overlays-overlapping start stop))) + #'<))) (when (or (not changes) (< start (car changes))) (push start changes)) (let ((last (last changes))) @@ -740,9 +749,11 @@ The list is sorted in order of increasing buffer position." (setcdr last (list stop)))) changes)) -(defun mmm-regions-in (start stop) +(defun mmm-regions-in (start stop &optional use-markers) "Return a list of regions of the form (MODE BEG END OVL) whose disjoint -union covers the region from START to STOP, including delimiters." +union covers the region from START to STOP, including delimiters. If +USE-MARKERS is non-nil, result contains markers instead of +integers; required when indenting." (when (> stop start) (let ((regions (cl-maplist (lambda (pos-list) @@ -753,15 +764,17 @@ union covers the region from START to STOP, including delimiters." mmm-primary-mode) (car pos-list) (cadr pos-list) ovl)))) - (mmm-submode-changes-in start stop)))) + (mmm-submode-changes-in start stop use-markers)))) (setcdr (last regions 2) nil) regions))) -(defun mmm-regions-alist (start stop) - "Return a list of lists of the form \(MODE . REGIONS) where REGIONS -is a list of elements of the form \(BEG END OVL). The disjoint union all -of the REGIONS covers START to STOP." - (let ((regions (mmm-regions-in start stop)) +(defun mmm-regions-alist (start stop &optional use-markers) + "Return a list of lists of the form \(MODE . REGIONS) where +REGIONS is a list of elements of the form \(BEG END OVL). The +disjoint union all of the REGIONS covers START to STOP. If +USE-MARKERS is non-nil, result contains markers instead of +integers; required when indenting." + (let ((regions (mmm-regions-in start stop use-markers)) alist) (mapc (lambda (region) (let* ((mode (car region)) @@ -912,19 +925,124 @@ appease modes which rely on constructs like (point-min) to indent." (overlay-end mmm-current-overlay)) (funcall indent-function)) (funcall indent-function))))) +(make-obsolete 'mmm-indent-line-narrowed 'mmm-indent-line 0.6) (defun mmm-indent-line () + "Indent the current line according to the mode for the current line. +Calls the indent function given by the 'mmm-indent-line-function +property of the mode for the current line. If that mode's +'mmm-indent-narrow propery is non-nil, narrow to the submode +region first." (interactive) - (funcall - (save-excursion - (back-to-indentation) - (mmm-update-submode-region) - (get - (if (and mmm-current-overlay - (> (overlay-end mmm-current-overlay) (point))) - mmm-current-submode - mmm-primary-mode) - 'mmm-indent-line-function)))) + (save-excursion + (back-to-indentation) + (mmm-update-submode-region) + (let ((indent-function (get + (if (and mmm-current-overlay + (> (overlay-end mmm-current-overlay) (point))) + mmm-current-submode + mmm-primary-mode) + 'mmm-indent-line-function))) + (if (and mmm-current-overlay + (get mmm-current-submode 'mmm-indent-narrow)) + (save-restriction + (narrow-to-region (overlay-start mmm-current-overlay) + (overlay-end mmm-current-overlay)) + (funcall indent-function)) + (funcall indent-function))))) + +(defun mmm-indent-region-list (mode regions start stop) + "Indent REGIONS (a list of (BEG END OVL)), using MODE indent-region-function. +START and STOP are the boundaries of the area to indent. If +NARROW, narrow to BEG END before calling the +ident-region-function." + (save-excursion + (let ((func (get mode 'mmm-indent-region-function)) + (narrow (get mode 'mmm-indent-narrow))) + (dolist (reg regions) + (cl-destructuring-bind (beg end ovl) reg + (goto-char beg) + ;; Here we do a subset of what `mmm-update-submode-region' + ;; does, but we force it to use a specific mode. + (mmm-set-current-pair mode ovl) + (mmm-set-local-variables (unless (eq mmm-previous-submode mode) + mode) + mmm-current-overlay) + + ;; Adjust BEG END for partial lines + ;; + ;; We want each line indented by the mode that covers the + ;; beginning of the line, not the end. + ;; + ;; For example, in wisitoken-grammar-mode, %()% delimits elisp code: + ;; + ;; extension_aggregate + ;; : '(' expression 'with' record_component_association_list ')' + ;; %((wisi-indent-action [nil + ;; (wisi-anchored 1 1) + ;; (wisi-anchored 1 1) + ;; [(wisi-anchored 1 1) (wisi-anchored 1 1)] + ;; (wisi-anchored 1 0)]))% + ;; | PERCENT END IF + ;; | PERCENT END IF + ;; ; + ;; + ;; '%((wisi-' should be indented by wisitoken-grammar-mode, + ;; '(wisi-anchored 1 0)]))%' by elisp-mode. + ;; + ;; Suppose `start' and `stop' contain the entire outer-mode + ;; statement. Then there are three indent regions: + ;; + ;; 1. 'extension ... %(' ; wisitoken-grammar-mode + ;; 2. '%( ... )% ; elisp-mode + ;; 3. ')% ... ;' ; wisitoken-grammar-mode + + ;; Narrowing is required by modes that look before `beg', + ;; expecting to find source code for `mode' language. We + ;; narrow before adjusting `beg', `end', since the extra + ;; code may affect the indent computation. + (save-restriction + (when narrow (narrow-to-region beg end)) + + (if ovl + ;; We are indenting an inner mode (region 2), and should not indent + ;; the line containing `beg'. + (setq beg (line-beginning-position 2)) + + ;; We are indenting the containing mode (region 1 or 3). + ;; In region 1, we should indent all lines. In region 3, we + ;; should not indent the line containing `beg'. + (if (= start beg) + ;; Assume `beg' is not a submode boundary; indent it. + nil + (setq beg (line-beginning-position 2)))) + + (funcall func (max beg start) (min end stop))) + + (mmm-save-changed-local-variables + mmm-current-submode mmm-current-overlay)) + )))) + +(defun mmm-indent-region (start stop) + "Indent from START to STOP keeping track of submodes correctly. +For `indent-region-function'." + (let ((saved-mode mmm-current-submode) + (saved-ovl mmm-current-overlay)) + (unwind-protect + (progn + (mmm-save-changed-local-variables + mmm-current-submode mmm-current-overlay) + (dolist (elt (mmm-regions-alist start stop t)) + (mmm-indent-region-list (car elt) (cdr elt) start stop))) + (mmm-set-current-pair saved-mode saved-ovl) + (mmm-set-local-variables (or saved-mode mmm-primary-mode) saved-ovl)) + )) + +(defvar mmm-indent-region-function #'mmm-indent-region + "The function to call to indent a region. +This will be the value of `indent-region-function' for the whole +buffer. It's supposed to delegate to the appropriate submode's +indentation function. See `mmm-indent-region' as the starting point.") ;;}}} (provide 'mmm-region)