search

dgutov / mmm-mode

New official home for mmm-mode, fixed for Emacs >= 23
http://mmm-mode.sourceforge.net/
GNU General Public License v2.0
333 stars 32 forks source link

implement mmm-indent-region #111

Open stephe-ada-guru opened 4 years ago

stephe-ada-guru commented 4 years ago

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 commented 4 years ago

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

stephe-ada-guru commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/purcell/mmm-mode/pull/75 for a similar, yet unfinished, discussion.

stephe-ada-guru commented 4 years ago

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 https://github.com/purcell/mmm-mode/pull/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Looking forward to it.

stephe-ada-guru commented 4 years ago

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

dgutov commented 4 years ago

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 commented 4 years ago

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)

-(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."

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

-(defun mmm-regions-alist (start stop)