Re: Improvements to adjust-parens

Re: Improvements to adjust-parens

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

>
>
> I have recently started to use your adjust-parens package for Emacs. I
> have made a few changes to my copy that I thought you might want to
> incorporate upstream. I have attached a patch of all my changes; below are
> the ChangeLog entries for my changes (also inclued in the patch):
>
>     * adjust-parens.el (adjust-parens-mode): Convert to minor mode
>     (global-adjust-parens-mode): Add globalized mode. Uses a custom
>     var to determine which modes to enable the mode in.
>     (adjust-parens-p): Only adjust parens after fixing indentation and
>     moving point to end of indentation.
>     (adjust-parens-and-indent): Correct handling of prefix arg. Now
>     only explicit numeric prefix arguments (i.e. not `C-u') affect the
>     number of parens adjusted, and negative & zero prefix are handled
>     correctly.
>
>     * adjust-parens-tests.el (apt-mode-test): Add tests for minor mode
>     enabling/disabling
>     (apt-indent-dedent-test): Add tests for behavior when indentation
>     is incorrect or point is not at end of indentation
>
> The to main changes of interest are the conversion of adjust-parens into a
> minor mode that can be enabled and disabled, and the changing the initial
> behavior of the TAB key when the current line is wrongly indented or the
> point is at the wrong position. I hope you find this patch useful. Please
> let me know what you think of it.
>
>
Thanks. adjust-parens is in ELPA which requires a copyright assignment
to FSF for non tiny changes. I'm not sure how to check whether you're
on the list of those who have completed the paperwork.

[-- Attachment #2: Type: text/html, Size: 2209 bytes --]

Re: Improvements to adjust-parens

[Ryan]

I believe I have already completed the copyright assignment.

On Mon Nov 25 17:51:54 2013, Barry OReilly wrote:
>
>     I have recently started to use your adjust-parens package for
>     Emacs. I have made a few changes to my copy that I thought you
>     might want to incorporate upstream. I have attached a patch of all
>     my changes; below are the ChangeLog entries for my changes (also
>     inclued in the patch):
>
>         * adjust-parens.el (adjust-parens-mode): Convert to minor mode
>         (global-adjust-parens-mode): Add globalized mode. Uses a custom
>         var to determine which modes to enable the mode in.
>         (adjust-parens-p): Only adjust parens after fixing indentation and
>         moving point to end of indentation.
>         (adjust-parens-and-indent): Correct handling of prefix arg. Now
>         only explicit numeric prefix arguments (i.e. not `C-u') affect the
>         number of parens adjusted, and negative & zero prefix are handled
>         correctly.
>
>         * adjust-parens-tests.el (apt-mode-test): Add tests for minor mode
>         enabling/disabling
>         (apt-indent-dedent-test): Add tests for behavior when indentation
>         is incorrect or point is not at end of indentation
>
>     The to main changes of interest are the conversion of
>     adjust-parens into a minor mode that can be enabled and disabled,
>     and the changing the initial behavior of the TAB key when the
>     current line is wrongly indented or the point is at the wrong
>     position. I hope you find this patch useful. Please let me know
>     what you think of it.
>
>
> Thanks. adjust-parens is in ELPA which requires a copyright assignment
> to FSF for non tiny changes. I'm not sure how to check whether you're
> on the list of those who have completed the paperwork.
>

Re: Improvements to adjust-parens

[Stefan Monnier]

> Thanks. adjust-parens is in ELPA which requires a copyright assignment
> to FSF for non tiny changes. I'm not sure how to check whether you're
> on the list of those who have completed the paperwork.

Ryan did sign his paperwork, thank you,


        Stefan

Re: Improvements to adjust-parens

[Ryan]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

Oh, I forgot to mention, the same changes are in a Github repo, in case 
you want to cherry-pick things: 
https://github.com/DarwinAwardWinner/adjust-parens

On 11/25/13, 5:51 PM, Barry OReilly wrote:
>
>
>     I have recently started to use your adjust-parens package for
>     Emacs. I have made a few changes to my copy that I thought you
>     might want to incorporate upstream. I have attached a patch of all
>     my changes; below are the ChangeLog entries for my changes (also
>     inclued in the patch):
>
>         * adjust-parens.el (adjust-parens-mode): Convert to minor mode
>         (global-adjust-parens-mode): Add globalized mode. Uses a custom
>         var to determine which modes to enable the mode in.
>         (adjust-parens-p): Only adjust parens after fixing indentation and
>         moving point to end of indentation.
>         (adjust-parens-and-indent): Correct handling of prefix arg. Now
>         only explicit numeric prefix arguments (i.e. not `C-u') affect the
>         number of parens adjusted, and negative & zero prefix are handled
>         correctly.
>
>         * adjust-parens-tests.el (apt-mode-test): Add tests for minor mode
>         enabling/disabling
>         (apt-indent-dedent-test): Add tests for behavior when indentation
>         is incorrect or point is not at end of indentation
>
>     The to main changes of interest are the conversion of
>     adjust-parens into a minor mode that can be enabled and disabled,
>     and the changing the initial behavior of the TAB key when the
>     current line is wrongly indented or the point is at the wrong
>     position. I hope you find this patch useful. Please let me know
>     what you think of it.
>
>
> Thanks. adjust-parens is in ELPA which requires a copyright assignment
> to FSF for non tiny changes. I'm not sure how to check whether you're
> on the list of those who have completed the paperwork.
>


[-- Attachment #2: Type: text/html, Size: 3725 bytes --]

Re: Improvements to adjust-parens

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]

       (and (not (use-region-p))
-           (<= orig-pos (point))))))
+           ;; Corrent line is properly indented
+           (= (current-column)
+              (let ((indent (calculate-lisp-indent)))
+                (if (listp indent)
+                    (car indent)
+                  indent)))
+           ;; Point at indentation
+           (= orig-pos (point))))))

Revising the diff to account for nil indent and to put the most
expensive check last:

-      (and (not (use-region-p))
-           (<= orig-pos (point))))))
+      (and (= orig-pos (point))
+           (not (use-region-p))
+           ;; Corrent line is properly indented
+           (let ((indent (calculate-lisp-indent)))
+             (and indent
+                  (= (current-column)
+                     (if (listp indent)
+                         (car indent)
+                       indent))))))))

Regarding:

+       ;; Only pass an explicit numeric prefix, not `C-u' prefix.
+       (unless (listp parg)
+         (prefix-numeric-value parg)))

What sort of use cases do prefix args as lists normally serve? From
what I see, they come in powers of four, which doesn't seem applicable
to adjust-parens. The Elisp manual entry doesn't really cover the
"why" of this kind of prefix arg.

+  (cl-callf or parg 1)
+  ;; Negative prefix arg inverts the behavior
+  (when (< parg 0)
+    (setq parg (- parg)
+          adjust-function
+          (cl-case adjust-function
+            (adjust-close-paren-for-indent 'adjust-close-paren-for-dedent)
+            (adjust-close-paren-for-dedent 'adjust-close-paren-for-indent)
+            (otherwise (error "Unknown adjust-function: %s"
adjust-function)))))
+  (when (> parg 0)

Processing negative prefix arg makes sense. I think it is cleaner to
do it one level higher and pass the right function as the
adjust-function argument. eg:

  (adjust-parens-and-indent
   (if (< parg 0)
       'adjust-close-paren-for-dedent
     'adjust-close-paren-for-indent)
   (abs parg))

+    ;; Move forward (but not back) to end of indentation (but don't
+    ;; change the indentation unlike `indent-for-tab-command'.
+    (when (< (current-column) (current-indentation))
+      (back-to-indentation))))

I figured since <backtab> is unbound in 'emacs -Q', adjust-parens
should do nothing in this case. The analogous <backtab> in Python
appears to also do nothing if it does not dedent.

It might be easier to decide what to do here with better perspective
about how people use <backtab>.

+(define-globalized-minor-mode global-adjust-parens-mode adjust-parens-mode
           (lambda ()
-            (local-set-key (kbd "TAB") 'lisp-indent-adjust-parens)
-            (local-set-key (kbd "<backtab>") 'lisp-dedent-adjust-parens)))
+    ;; Add or remove hook
+    (funcall (if global-adjust-parens-mode #'add-hook #'remove-hook)
+             'after-change-major-mode-hook
+             #'adjust-parens-after-change-mm-hook)
+    ;; Run the hook in existing buffers to enable/disable the mode
+    (dolist (buf (buffer-list))
+      (with-current-buffer buf
+        (adjust-parens-after-change-mm-hook)))))

adjust-parens doesn't seem global in nature, so I'm uncertain about
the choice of a global minor mode. I think a local minor mode makes
sense though. Then the user would enable it in major modes by eg:

  (add-hook 'emacs-lisp-mode-hook #'adjust-parens-mode)

[-- Attachment #2: Type: text/html, Size: 3878 bytes --]

Re: Improvements to adjust-parens

[Ryan C. Thompson]

Replies in line below:

On 11/26/2013 03:04 PM, Barry OReilly wrote:
>        (and (not (use-region-p))
> -           (<= orig-pos (point))))))
> +           ;; Corrent line is properly indented
> +           (= (current-column)
> +              (let ((indent (calculate-lisp-indent)))
> +                (if (listp indent)
> +                    (car indent)
> +                  indent)))
> +           ;; Point at indentation
> +           (= orig-pos (point))))))
>
> Revising the diff to account for nil indent and to put the most
> expensive check last:
>
> -      (and (not (use-region-p))
> -           (<= orig-pos (point))))))
> +      (and (= orig-pos (point))
> +           (not (use-region-p))
> +           ;; Corrent line is properly indented
> +           (let ((indent (calculate-lisp-indent)))
> +             (and indent
> +                  (= (current-column)
> +                     (if (listp indent)
> +                         (car indent)
> +                       indent))))))))
>

Sounds good. Also change "corrent" to "current". That's probably a typo 
on my part.

> Regarding:
>
> +       ;; Only pass an explicit numeric prefix, not `C-u' prefix.
> +       (unless (listp parg)
> +         (prefix-numeric-value parg)))
>
> What sort of use cases do prefix args as lists normally serve? From
> what I see, they come in powers of four, which doesn't seem applicable
> to adjust-parens. The Elisp manual entry doesn't really cover the
> "why" of this kind of prefix arg.

I believe the "C-u" prefix is passed as a list just so you can tell the 
difference between e.g. "C-4" and "C-u", i.e. a numeric vs generic 
prefix argument. I don't think there is ever more than one element in 
the list. In any case, you can make the prefix arguments work however 
you want.

>
> +  (cl-callf or parg 1)
> +  ;; Negative prefix arg inverts the behavior
> +  (when (< parg 0)
> +    (setq parg (- parg)
> +          adjust-function
> +          (cl-case adjust-function
> +            (adjust-close-paren-for-indent 
> 'adjust-close-paren-for-dedent)
> +            (adjust-close-paren-for-dedent 
> 'adjust-close-paren-for-indent)
> +            (otherwise (error "Unknown adjust-function: %s" 
> adjust-function)))))
> +  (when (> parg 0)
>
> Processing negative prefix arg makes sense. I think it is cleaner to
> do it one level higher and pass the right function as the
> adjust-function argument. eg:
>
>   (adjust-parens-and-indent
>    (if (< parg 0)
>        'adjust-close-paren-for-dedent
>      'adjust-close-paren-for-indent)
>    (abs parg))

That's fine. I just did the first thing that worked.

>
> +    ;; Move forward (but not back) to end of indentation (but don't
> +    ;; change the indentation unlike `indent-for-tab-command'.
> +    (when (< (current-column) (current-indentation))
> +      (back-to-indentation))))
>
> I figured since <backtab> is unbound in 'emacs -Q', adjust-parens
> should do nothing in this case. The analogous <backtab> in Python
> appears to also do nothing if it does not dedent.
>
> It might be easier to decide what to do here with better perspective
> about how people use <backtab>.

I forget what my logic was here. Feel free to do whatever you think is 
appropriate. I think backtab is actually not used in many modes.
>
> +(define-globalized-minor-mode global-adjust-parens-mode 
> adjust-parens-mode
>            (lambda ()
> -            (local-set-key (kbd "TAB") 'lisp-indent-adjust-parens)
> -            (local-set-key (kbd "<backtab>") 
> 'lisp-dedent-adjust-parens)))
> +    ;; Add or remove hook
> +    (funcall (if global-adjust-parens-mode #'add-hook #'remove-hook)
> +             'after-change-major-mode-hook
> +             #'adjust-parens-after-change-mm-hook)
> +    ;; Run the hook in existing buffers to enable/disable the mode
> +    (dolist (buf (buffer-list))
> +      (with-current-buffer buf
> +        (adjust-parens-after-change-mm-hook)))))
>
> adjust-parens doesn't seem global in nature, so I'm uncertain about
> the choice of a global minor mode. I think a local minor mode makes
> sense though. Then the user would enable it in major modes by eg:
>
>   (add-hook 'emacs-lisp-mode-hook #'adjust-parens-mode)
>

The global mode isn't really "global". It only activates in buffers with 
approved major modes. This is essentially a way for users who do all 
their customization through M-x customize to auto-enable adjust-parens 
in specific modes, by enabling the global mode and then customizing 
"adjust-parens-enabled-major-modes".

Re: Improvements to adjust-parens

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

> I forget what my logic was here. Feel free to do whatever you think
> is appropriate. I think backtab is actually not used in many modes.

I made it so as lisp-dedent-adjust-parens returns nil for that case,
so you can bind:

  (lambda (parg)
    (unless (lisp-dedent-adjust-parens parg)
      (when (< (current-column) (current-indentation))
        (back-to-indentation))))

to <backtab>.

I adapted your updates and bumped the version in ELPA. Thank you.

[-- Attachment #2: Type: text/html, Size: 548 bytes --]