Help setting nadvice for indent-region

Help setting nadvice for indent-region

[Kaushal Modi]

Hi,

I'd like to advice indent-region so that if a region is not select, it
indents between (point-min) and (point-max).

So I have this:

=====

(defun adv/indent-region (args)
  (when (not mark-active)
    (setq args (list (point-min) (point-max))))
  args)
(advice-add 'indent-region :filter-args #'adv/indent-region)

=====

This usually works, unless I have just launched a fresh buffer in which
there is no mark set.

If I do M-: (mark) in that buffer, I get nil.
In that case, if I call M-x indent-region (with no region selected), I get
this error backtrace:

Debugger entered--Lisp error: (error "The mark is not set now, so there is
no region")
  call-interactively(indent-region nil nil)
  command-execute(indent-region)

If it looks like the error is triggered by call-interactively even before
the advice gets to do its thing.

How can I resolve this using nadvice?

Thanks.

Kaushal Modi

Re: Help setting nadvice for indent-region

[Kaushal Modi]

Aha.. looks like the check_mark in callint.c is raising the error even
before the advice sets the args as the call_interactively's 'r' case is
activated. That's because indent-region has (interactive "r\nP").

So looks like I'll need to advice the whole interactive? What would be a
better solution?

On Fri, Feb 5, 2016 at 6:49 PM Kaushal Modi <kaushal.modi@gmail.com> wrote:

> Hi,
>
> I'd like to advice indent-region so that if a region is not select, it
> indents between (point-min) and (point-max).
>
> So I have this:
>
> =====
>
> (defun adv/indent-region (args)
>   (when (not mark-active)
>     (setq args (list (point-min) (point-max))))
>   args)
> (advice-add 'indent-region :filter-args #'adv/indent-region)
>
> =====
>
> This usually works, unless I have just launched a fresh buffer in which
> there is no mark set.
>
> If I do M-: (mark) in that buffer, I get nil.
> In that case, if I call M-x indent-region (with no region selected), I get
> this error backtrace:
>
> Debugger entered--Lisp error: (error "The mark is not set now, so there is
> no region")
>   call-interactively(indent-region nil nil)
>   command-execute(indent-region)
>
> If it looks like the error is triggered by call-interactively even before
> the advice gets to do its thing.
>
> How can I resolve this using nadvice?
>
> Thanks.
>
> Kaushal Modi
>

Re: Help setting nadvice for indent-region

[Kaushal Modi]

Neat, I hit a brick-wall there.

Advising interactive gives: "advice--normalize: Advice impossible:
interactive is a special form"

On Fri, Feb 5, 2016 at 6:58 PM Kaushal Modi <kaushal.modi@gmail.com> wrote:

> Aha.. looks like the check_mark in callint.c is raising the error even
> before the advice sets the args as the call_interactively's 'r' case is
> activated. That's because indent-region has (interactive "r\nP").
>
> So looks like I'll need to advice the whole interactive? What would be a
> better solution?
>
> On Fri, Feb 5, 2016 at 6:49 PM Kaushal Modi <kaushal.modi@gmail.com>
> wrote:
>
>> Hi,
>>
>> I'd like to advice indent-region so that if a region is not select, it
>> indents between (point-min) and (point-max).
>>
>> So I have this:
>>
>> =====
>>
>> (defun adv/indent-region (args)
>>   (when (not mark-active)
>>     (setq args (list (point-min) (point-max))))
>>   args)
>> (advice-add 'indent-region :filter-args #'adv/indent-region)
>>
>> =====
>>
>> This usually works, unless I have just launched a fresh buffer in which
>> there is no mark set.
>>
>> If I do M-: (mark) in that buffer, I get nil.
>> In that case, if I call M-x indent-region (with no region selected), I
>> get this error backtrace:
>>
>> Debugger entered--Lisp error: (error "The mark is not set now, so there
>> is no region")
>>   call-interactively(indent-region nil nil)
>>   command-execute(indent-region)
>>
>> If it looks like the error is triggered by call-interactively even before
>> the advice gets to do its thing.
>>
>> How can I resolve this using nadvice?
>>
>> Thanks.
>>
>> Kaushal Modi
>>
>

Re: Help setting nadvice for indent-region

[Emanuel Berg]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> So looks like I'll need to advice the whole
> interactive? What would be a better solution?

Advices are always tricky. Instead I'd do something
like this:

    (defun indent-region-dwim ()
      (interactive)
      (if mark-active
          (indent-region (mark) (point))
        (indent-region (point-min) (point-max) )))

And then rebind shortcuts, or use this method:

    (define-key (current-global-map) [remap w3m-quit] #'no-confirm-w3m-quit)

-- 
underground experts united
http://user.it.uu.se/~embe8573

Re: Help setting nadvice for indent-region

[Kaushal Modi]

On Fri, Feb 5, 2016 at 7:30 PM Emanuel Berg <embe8573@student.uu.se> wrote:

> Advices are always tricky. Instead I'd do something
> like this:
>
>     (defun indent-region-dwim ()
>       (interactive)
>       (if mark-active
>           (indent-region (mark) (point))
>         (indent-region (point-min) (point-max) )))
>
> And then rebind shortcuts, or use this method:
>
>     (define-key (current-global-map) [remap w3m-quit]
> #'no-confirm-w3m-quit)
>

Thanks Emanuel.

What you suggested was my plan B and that's what I have executed now. I
have taken your idea to create a generic "region or whole" function
generator.

https://github.com/kaushalmodi/.emacs.d/blob/b013406a864de46f0fb479e339b04945f8f01351/setup-files/setup-editing.el#L686-L716


To all, I am still curious to know if it is possible to achieve the same
using nadvice. If not, then is it an advice limitation that we cannot
override the nature of interactive function arguments? In this case, looks
like we cannot call indent-region with nil args even though we have an
advice designed to set those args automatically to non-nil values.

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> How can I resolve this using nadvice?

Just specify an alternative interactive form in adv/indent-region.  I
would change it into the :around type then, though.


Michael.

Re: Help setting nadvice for indent-region

[Kaushal Modi]

On Sat, Feb 6, 2016 at 5:43 AM Michael Heerdegen <michael_heerdegen@web.de>
wrote:

> Just specify an alternative interactive form in adv/indent-region.  I
> would change it into the :around type then, though.
>

Thanks! That worked!

(defun adv/indent-region (orig-fn &rest args)
  (interactive)
  (if mark-active
      (setq args (list (region-beginning) (region-end)))
    (setq args (list (point-min) (point-max))))
  (message "Args: %S" args)
  (apply orig-fn args))
(advice-add 'indent-region :around #'adv/indent-region)

Re: Help setting nadvice for indent-region

[Kaushal Modi]

For the sake of completeness, this is how I actually needed to implement
this advice in my config:
https://github.com/kaushalmodi/.emacs.d/blob/1f9daf3587863de8561fc34e0bafda80be389dbf/setup-files/setup-editing.el#L686-L731

I am open to comments and criticism.

Thanks!

Re: Help setting nadvice for indent-region

[John Mastro]

Kaushal Modi <kaushal.modi@gmail.com> wrote:
> For the sake of completeness, this is how I actually needed to implement
> this advice in my config:
> https://github.com/kaushalmodi/.emacs.d/blob/1f9daf3587863de8561fc34e0bafda80be389dbf/setup-files/setup-editing.el#L686-L731
>
> I am open to comments and criticism.
>
> Thanks!

My preference is for something a bit simpler, which avoids the use of
macros. Macros can be awesome, but IMO they don't contribute much here.

The downside to my solution is that it doesn't print the "Executed... on
the whole buffer" message, since the advice function never sees the
symbol that names the adviced function. It has access to ‘orig’, of
course, but that may be something that wouldn't print well (e.g. a
compiled Lisp function). However, I don't see that as critical.

(defvar modi/region-or-whole-fns '(indent-region eval-region))

(defun modi/region-or-whole-advice (orig &rest _)
  (interactive)
  (if (use-region-p)
      (funcall orig (region-beginning) (region-end))
    (funcall orig (point-min) (point-max))))

(dolist (fn modi/region-or-whole-fns)
  (advice-add fn :around #'modi/region-or-whole-advice))

-- 
john

Re: Help setting nadvice for indent-region

[Emanuel Berg]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> For the sake of completeness, this is how I actually
> needed to implement this advice in my config

That file is 871 lines. Surely all that isn't how you
solved this particular problem and that alone.

So why not yank it into a Gnus message buffer and fire
it away here as well?

That way it'll be even more "complete" because it'll
be in the mailing list archives for ages - God willing
- where it is Google'able as well.

-- 
underground experts united
http://user.it.uu.se/~embe8573

Re: Help setting nadvice for indent-region

[Emanuel Berg]

John Mastro <john.b.mastro@gmail.com> writes:

> My preference is for something a bit simpler, which
> avoids the use of macros. Macros can be awesome, but
> IMO they don't contribute much here.

Macros and advices are absolutely at the next level
compared to stapling defuns as another bricklayer, but
just because it is more advanced doesn't make it
better - it depends.

> (defvar modi/region-or-whole-fns '(indent-region eval-region))
>
> (defun modi/region-or-whole-advice (orig &rest _)
>   (interactive)
>   (if (use-region-p)
>       (funcall orig (region-beginning) (region-end))
>     (funcall orig (point-min) (point-max))))
>
> (dolist (fn modi/region-or-whole-fns)
>   (advice-add fn :around #'modi/region-or-whole-advice))

OK, if this is "simpler", I'd say my DWIM groyne is
simpler still:

    (defun indent-region-dwim ()
      (interactive)
      (if mark-active
          (indent-region (mark) (point))
        (indent-region (point-min) (point-max) )))

It it also more natural and "human-ish" to read
without all the computer lingo (`funcall' etc.).

Anyway, another interesting difference, where I'm not
sure what is the best way, is

    `mark-active', then (mark) and (point)

vs.

    (use-region-p), then (region-beginning) and (region-help)

What does "the book" say on this?

-- 
underground experts united
http://user.it.uu.se/~embe8573

Re: Help setting nadvice for indent-region

[Kaushal Modi]

On Sun, Feb 7, 2016 at 7:03 PM Emanuel Berg <embe8573@student.uu.se> wrote:

> Anyway, another interesting difference, where I'm not
> sure what is the best way, is
>
>     `mark-active', then (mark) and (point)
>
> vs.
>
>     (use-region-p), then (region-beginning) and (region-help)
>
> What does "the book" say on this?


That's a good question. At times, I use mark-active and at times,
(use-region-p). In this particular case, I used mark-active because (mark)
being nil was what bit me in my first version of advising indent-region. I
believe, using either would be fine here.

> That file is 871 lines. Surely all that isn't how you
> solved this particular problem and that alone.

I am sorry, I did not follow that. The link I pasted was to a particular
commit in my config, highlighting only 46 lines pertaining to this advice
definition. On a PC, clicking that link should show you that highlighted
section of 46 lines in a browser like Chrome/Firefox.

> So why not yank it into a Gnus message buffer and fire
> it away here as well?

I simply find it convenient to read code on github with monospace fonts and
syntax highlighting. I use a wonderful package called git-link to quickly
get permalinks to my code snippets on github.

>> John Mastro
> My preference is for something a bit simpler, which avoids the use of
> macros. Macros can be awesome, but IMO they don't contribute much here.

I like the message telling me exactly what happened i.e. I indented the
whole buffer or I eval'ed the whole buffer. But I can understand that that
does not give much value. My initial purpose to use macro here was to learn
how to use a macro. I like to grow my config with new styles and snippets
of elisp.

Just one important thing I'd like to point out in my code is the necessity
to modify the orig-fn args ONLY when args is nil. This is to protect from
corrupting the args when the advised fn is called by a wrapper fn. E.g. we
do not want to override all 4 args to eval-region (set by eval-defun) with
just 2 args when eval-region is being called by eval-defun.

Finally, thank you all for taking time to go through my code and providing
your feedback.

Kaushal

Re: Help setting nadvice for indent-region

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Mon, 08 Feb 2016 04:22:55 +0000
> 
> On Sun, Feb 7, 2016 at 7:03 PM Emanuel Berg <embe8573@student.uu.se> wrote:
> 
> > Anyway, another interesting difference, where I'm not
> > sure what is the best way, is
> >
> >     `mark-active', then (mark) and (point)
> >
> > vs.
> >
> >     (use-region-p), then (region-beginning) and (region-help)
> >
> > What does "the book" say on this?
> 
> 
> That's a good question. At times, I use mark-active and at times,
> (use-region-p).

Here's what "the book" says:

 -- Variable: mark-active
     The mark is active when this variable is non-‘nil’.  This variable
     is always buffer-local in each buffer.  Do _not_ use the value of
     this variable to decide whether a command that normally operates on
     text near point should operate on the region instead.  Use the
     function ‘use-region-p’ for that (*note The Region::).

Re: Help setting nadvice for indent-region

[Kaushal Modi]

On Mon, Feb 8, 2016 at 12:17 PM Eli Zaretskii <eliz@gnu.org> wrote:

>
> Here's what "the book" says:
>
>  -- Variable: mark-active
>      The mark is active when this variable is non-‘nil’.  This variable
>      is always buffer-local in each buffer.  Do _not_ use the value of
>      this variable to decide whether a command that normally operates on
>      text near point should operate on the region instead.  Use the
>      function ‘use-region-p’ for that (*note The Region::).
>
>
Thanks Eli. I should have looked there first. Thanks for the pointer..
fixing it now :)

Re: Help setting nadvice for indent-region

[John Mastro]

Emanuel Berg <embe8573@student.uu.se> wrote:
> Macros and advices are absolutely at the next level
> compared to stapling defuns as another bricklayer, but
> just because it is more advanced doesn't make it
> better - it depends.
>
>> (defvar modi/region-or-whole-fns '(indent-region eval-region))
>>
>> (defun modi/region-or-whole-advice (orig &rest _)
>>   (interactive)
>>   (if (use-region-p)
>>       (funcall orig (region-beginning) (region-end))
>>     (funcall orig (point-min) (point-max))))
>>
>> (dolist (fn modi/region-or-whole-fns)
>>   (advice-add fn :around #'modi/region-or-whole-advice))
>
> OK, if this is "simpler", I'd say my DWIM groyne is
> simpler still:
>
>     (defun indent-region-dwim ()
>       (interactive)
>       (if mark-active
>           (indent-region (mark) (point))
>         (indent-region (point-min) (point-max) )))

I don't have strong feelings about this one way or the other, but the
points I would make in defense of advice are:
  - It's "just" function composition, nothing scary
  - Your solution requires you to define a new `dwim' function every
    time you want overload a function like this, whereas with advice you
    can simply add it to the list of functions to be adviced (assuming
    it will be adviced in the same way).

> It it also more natural and "human-ish" to read
> without all the computer lingo (`funcall' etc.).

Avoid funcall because it has a fun-y (haha) name? This part I can't get
behind at all.

-- 
john

Re: Help setting nadvice for indent-region

[Emanuel Berg]

John Mastro <john.b.mastro@gmail.com> writes:

> Your solution requires you to define a new `dwim'
> function every time you want overload a function
> like this

Yes, which is very fast and accurate.

> whereas with advice you can simply add it to the
> list of functions to be adviced (assuming it will be
> adviced in the same way).

If you write a defun, you know for sure what it does
and that it will be used as intended.

If you start muck around with existing code you have
two places to look when you want to know what happens,
instead of one place. Only at one of those places is
there even an indication there is another place to
look (i.e., where the advice is, but not where the
original code is).

Also, with a defun, it is used explicitly, but with
the advice or self-modifying code, what make sense for
you can be completely incomprehensible/invisible to
another person or program that uses the same piece of
code, and this makes debugging much more difficult -
also for the reason above (two places), by the way.

Remember the whole monolithic kernel vs.
microkernel thing.

>> It it also more natural and "human-ish" to read
>> without all the computer lingo (`funcall' etc.).
>
> Avoid funcall because it has a fun-y (haha) name?
> This part I can't get behind at all.

Just because people program computer doesn't mean
anyone is benefited from the people themselves turning
into, or acting like, computers. If the code is all
computer lingo this is a bad sign.

Compare the old-school C code which was constantly
pointers, references, bitmasks, all computer stuff,
enough to make your head spin - what does all this
stuff *do*? /* You are not expected to understand this */
To get the size of a vector ("array" in the C lingo),
simple! do

    int n = sizeof(a)/sizeof(a[0]);

compare this to the Lisp `length'.

-- 
underground experts united
http://user.it.uu.se/~embe8573

Re: Help setting nadvice for indent-region

[Emanuel Berg]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> I am sorry, I did not follow that. The link I pasted
> was to a particular commit in my config,
> highlighting only 46 lines pertaining to this
> advice definition.

One should never expect anyone to follow links.
They are provided to say "this piece of code exists in
a file, it is compiled or otherwise in effect on some
system somewhere on the planet, if you against all
odds would like to use it or study it on your terms
I'm making this as easy for you as possible..." It is
just like scientific papers that have hundreds of
references no one ever bothers to read up on. So you
should do it, but always yank the code into the
message as well.

> On a PC, clicking that link should show you that
> highlighted section of 46 lines in a browser like
> Chrome/Firefox.

OK, drop them GUI browsers for Emacs-w3m, and stop
clicking on stuff - instead hit RET!

But it is OK to use a PC :)

> I simply find it convenient to read code on github
> with monospace fonts and syntax highlighting.

Mails/posts should always be written/read in
a monospace font. Try Emacs Gnus. As for syntax
highlighting (we call int font-lock) it is possible to
have snippets like that inline - not really necessary
(for messages) IMHO.

> I like the message telling me exactly what happened
> i.e. I indented the whole buffer or I eval'ed the
> whole buffer. But I can understand that that does
> not give much value. My initial purpose to use macro
> here was to learn how to use a macro.

Probably better to wait for a sharp situation to
arrive and do other stuff meanwhile. Because, if you
make up a solution to a made-up problem chances are
something won't work or won't work as intended and you
will then not be able to tell if it is the solution,
the method or the "made-up"-ness that failed (or
a combination).

> Just one important thing I'd like to point out in my
> code is the necessity to modify the orig-fn args
> ONLY when args is nil. This is to protect from
> corrupting the args when the advised fn is called by
> a wrapper fn. E.g. we do not want to override all 4
> args to eval-region (set by eval-defun) with just 2
> args when eval-region is being called by eval-defun.

You shouldn't focus on the technology. If a problem
that is very straightforward ends up in a complicated
discussion where everything is about technology and
nothing is about the problem, then the problem has not
been solved in a good way.

During the stone-age there were problems-solvers that
could solve all problems in the caves and around the
fireplace and even between the women in the
cave-society.

You can carry out a thought experiment to assess your
solution. If you can explain it to such
a problem-solving cave-dweller, then it is a good
solution. If he doesn't understand because of all the
advices, macros, arguments, and funcalls, it is
a bad solution.

-- 
underground experts united
http://user.it.uu.se/~embe8573

Re: Help setting nadvice for indent-region

[Stefan Monnier]

> (defun modi/region-or-whole-advice (orig &rest _)
>   (interactive)
>   (if (use-region-p)
>       (funcall orig (region-beginning) (region-end))
>     (funcall orig (point-min) (point-max))))
> (dolist (fn modi/region-or-whole-fns)
>   (advice-add fn :around #'modi/region-or-whole-advice))

This will affect all calls to these functions and is hence sure to cause
some breakage somewhere.  You want something more like:

    (defun modi/region-or-whole-advice (&rest _)
      (interactive
       (if (use-region-p)
           (list (region-beginning) (region-end))
        (list (point-min) (point-max))))
      nil)
    (dolist (fn modi/region-or-whole-fns)
      (advice-add fn :before #'modi/region-or-whole-advice))

So you only modify the interactive spec (i.e. change the *command*'s
behavior) without changing the *function*'s behavior.


-- Stefan

Re: Help setting nadvice for indent-region

[Kaushal Modi]

Thanks for the reply Stefan.

I have just one concern.. that this will not work for advising functions
like eval-defun.

I tried it out and when I do C-M-x while a point is in a function (without
any region selected), it evaluates the whole buffer instead of just the
defun.

So I have to use the :around advice so that I can access the args and then
do a (null args) check to control when to modify the args. Does that look
like a fair way to implement this? Or is it possible to somehow check the
value of provided args inside the (interactive ..) form?

I now have something that's a result of a mix of suggestions given by
everyone over here :)
- avoided using macro as it's not quite needed
- Using this-command to print the current command name as that works for
this case (where I intend to advice the functions when when called directly
as commands).

(defun modi/advice-region-or-whole (orig-fn &rest args)
  (interactive) ; Required to override the "r" argument of `interactive'
                                        ; in functions like `indent-region'
                                        ; so that that function can be
called
                                        ; without an active region.
  (let (msg)
    ;; Execute the original SYMBOL function if it is called indirectly.
    ;; Example: We do not want to modify the ARGS if `eval-region'
    ;;          is called via `eval-defun', because in that case, the
    ;;          ARGS are set by the wrapper function `eval-defun'.
    (when (null args)
      (if (use-region-p) ; when region is selected
          (setq args (list (region-beginning) (region-end)))
        (progn
          (setq args (list (point-min) (point-max)))
          (setq msg (format "Executed %s on the whole buffer."
                            (propertize (symbol-name this-command)
                                        'face
                                        'font-lock-function-name-face))))))
    (apply orig-fn args)
    (when msg
      (message msg))))

(dolist (fn modi/region-or-whole-fns)
  (advice-add fn :around #'modi/advice-region-or-whole))




On Thu, Feb 11, 2016 at 9:05 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > (defun modi/region-or-whole-advice (orig &rest _)
> >   (interactive)
> >   (if (use-region-p)
> >       (funcall orig (region-beginning) (region-end))
> >     (funcall orig (point-min) (point-max))))
> > (dolist (fn modi/region-or-whole-fns)
> >   (advice-add fn :around #'modi/region-or-whole-advice))
>
> This will affect all calls to these functions and is hence sure to cause
> some breakage somewhere.  You want something more like:
>
>     (defun modi/region-or-whole-advice (&rest _)
>       (interactive
>        (if (use-region-p)
>            (list (region-beginning) (region-end))
>         (list (point-min) (point-max))))
>       nil)
>     (dolist (fn modi/region-or-whole-fns)
>       (advice-add fn :before #'modi/region-or-whole-advice))
>
> So you only modify the interactive spec (i.e. change the *command*'s
> behavior) without changing the *function*'s behavior.
>
>
> -- Stefan
>
>
>

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> I have just one concern.. that this will not work for advising
> functions like eval-defun.

What exactly did you try to do? - `eval-defun' has only one argument
that is not related to the region, so the advises that were discussed
yet are not applicable here.  In what way do you want to change the
behavior of `eval-defun'?

But in general, what Stefan pointed out was important: changing the
semantics of the functions is not a good idea.

> (defun modi/advice-region-or-whole (orig-fn &rest args)
>   (interactive) ; Required to override the "r" argument of `interactive'
>                                         ; in functions like `indent-region'
>                                         ; so that that function can be
> called
>                                         ; without an active region.
>   (let (msg)
>     ;; Execute the original SYMBOL function if it is called indirectly.
>     ;; Example: We do not want to modify the ARGS if `eval-region'
>     ;;          is called via `eval-defun', because in that case, the
>     ;;          ARGS are set by the wrapper function `eval-defun'.
>     (when (null args)
>       (if (use-region-p) ; when region is selected
>           (setq args (list (region-beginning) (region-end)))
>         (progn
>           (setq args (list (point-min) (point-max)))
>           (setq msg (format "Executed %s on the whole buffer."
>                             (propertize (symbol-name this-command)
>                                         'face
>                                         'font-lock-function-name-face))))))
>     (apply orig-fn args)
>     (when msg
>       (message msg))))

Be careful: this changes the return value of the advised function to the
value returned by `when' -- this is not `defadvice'!

And BTW, (just a hint) you also don't need to `setq' the ARGS variable
(of course you can), just do

  (apply orig-fun (calculate-new-args-somehow-here-using-the args))

In advice.el, you had some weird mechanism to modify function arguments,
but nadvice is different, as it works by simply composing functions as
described in the doc of `add-function'.


Regards,

Michael.

Re: Help setting nadvice for indent-region

[Kaushal Modi]

On Thu, Feb 11, 2016 at 1:10 PM Michael Heerdegen <michael_heerdegen@web.de>
wrote:

>
> > I have just one concern.. that this will not work for advising
> > functions like eval-defun.
>
> What exactly did you try to do? - `eval-defun' has only one argument
> that is not related to the region, so the advises that were discussed
> yet are not applicable here.  In what way do you want to change the
> behavior of `eval-defun'?
>
>
Sorry, let me rephrase that. I meant to say that using Stefan's suggested
method to advise eval-region also affected eval-defun.  Without a region
selected, if I did C-M-x with the point in a defun, this advice begin
applied to eval-region would cause the whole buffer to be evaluated (not
just that defun as I would expect C-M-x to do). That would adversely affect
instrumenting edebug too (C-u C-M-x).


> But in general, what Stefan pointed out was important: changing the
> semantics of the functions is not a good idea.
>
> I agree but I did not find an alternative solution by which,
- I apply the advice to eval-region when called interactively.
- But not when it is called by a wrapper fn like eval-defun that presets
the args for eval-region.
I am open to adopt a cleaner, canonical solution.

>
> Be careful: this changes the return value of the advised function to the
> value returned by `when' -- this is not `defadvice'!
>
> Point taken, I will fix that.


> And BTW, (just a hint) you also don't need to `setq' the ARGS variable
> (of course you can), just do
>
>   (apply orig-fun (calculate-new-args-somehow-here-using-the args))
>
> Agreed. As I need to set the let-bound variable msg's value too, based on
(region-active-p), I decided to have just one (if ..) form and modify args
and msg in there as appropriate.

I really appreciate this feedback.

Re: Help setting nadvice for indent-region

[Kaushal Modi]

>> Michael Heerdegen
> Be careful: this changes the return value of the advised function to the
> value returned by `when' -- this is not `defadvice'!

After my earlier reply, I realized I did not know about the return values
of the advice functions when using :around. I just did not care what their
return values should be till now (for :around) and it hasn't yet hurt
anything.

I looked up the elisp Info ( (elisp) Advising Named Functions ) for rules
regarding the return values of the advice functions but couldn't find
anything. I understand that the return values of the advice functions when
using a combinator like :before-until are important.

But what should be the return value of an :around advice fn?

Thanks.

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Sorry, let me rephrase that. I meant to say that using Stefan's
> suggested method to advise eval-region also affected eval-defun.
> Without a region selected, if I did C-M-x with the point in a defun,
> this advice begin applied to eval-region would cause the whole buffer
> to be evaluated (not just that defun as I would expect C-M-x to
> do). That would adversely affect instrumenting edebug too (C-u C-M-x).

I tried it, and can't reproduce what you describe.  How did you try it
(recipe)?


Michael.

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> But what should be the return value of an :around advice fn?

The around advice function should return the value you want the adviced
function to return.  This will very often be the value gotten by
applying the original function (aka "don't change the return value"), or
a value derived from it.  Or something completely different.  The
advised function will return just this value as well.  The around advice
works like this:

  (lambda (&rest r) (apply FUNCTION OLDFUN r))

where FUNCTION is the advice defined.  As you see, the combined call has
the call of FUNCTION at the outermost level.

When several advices are in effect, the order is significant.

But note that this is not the case for all advice types.

Michael.

Re: Help setting nadvice for indent-region

[Kaushal Modi]

> I tried it, and can't reproduce what you describe.  How did you try it
(recipe)?

You are right. I am not able to recreate that. I was actually getting
confused because of this:

(defun modi/advice-region-or-whole (&rest args)
  "Advice function that applies the ORIG-FN function to the whole buffer if
a region is not selected."
  (interactive (if (use-region-p) ; when region is selected
                   (list (region-beginning) (region-end))
                 (list (point-min) (point-max))))
  (message "Args: %S use-region-p: %S" args (use-region-p))
  (when (not (use-region-p))
    (message "Executing %s on the whole buffer."
             (propertize (symbol-name this-command)
                         'face
                         'font-lock-function-name-face)))
  nil)

Of course when I did C-M-x, (use-region-p) was nil and so it printed:
"Executing eval-defun on the whole buffer."

Things became clear after I added this debug statement:

 (message "Args: %S use-region-p: %S" args (use-region-p))

In any case, I will be going the right way of advising this as Stefan and
you advised. Now I only need to figure out how not to print that message
when doing eval-defun.


> The around advice function should return the value you want the advised function
to return.

Thank you! So with a let-bound variable 'ret', I can have something like

(setq ret (apply orig-fn args))

and return 'ret' at the end of the :around advice function definition.
Right?

Re: Help setting nadvice for indent-region

[Kaushal Modi]

OK, so this is what I have now.

(defun modi/advice-region-or-whole (&rest args)
  "Advice function that sets the region boundaries to that of the whole
buffer
if no region is selected."
  (interactive (if (use-region-p)
                   (list (region-beginning) (region-end))
                 (list (point-min) (point-max))))
  ;; (message "Args: %S R: %S I: %S"
  ;;          args (use-region-p) (called-interactively-p 'interactive))
  (when (and (eq (first args) (point-min))
             (eq (second args) (point-max)))
    (message "Executing %s on the whole buffer."
             (propertize (symbol-name this-command)
                         'face 'font-lock-function-name-face)))
  nil)

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Things became clear after I added this debug statement:
>
>  (message "Args: %S use-region-p: %S" args (use-region-p))
>
> In any case, I will be going the right way of advising this as Stefan
> and you advised. Now I only need to figure out how not to print that
> message when doing eval-defun.

If it's only for debugging anyway, and you want to message that only
when your advice comes into play, I suggest to move that call to the
interactive part as well.

> > The around advice function should return the value you want the
> > advised function
> to return.
>
> Thank you! So with a let-bound variable 'ret', I can have something like
>
> (setq ret (apply orig-fn args))
>
> and return 'ret' at the end of the :around advice function definition.
> Right?

Sure.

It's just a matter of style (i.e. a more functional vs. a more
imperative programming style).


Regards,

Michael.

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> OK, so this is what I have now.
>
> (defun modi/advice-region-or-whole (&rest args)
>   "Advice function that sets the region boundaries to that of the whole
> buffer
> if no region is selected."
>   (interactive (if (use-region-p)
>                    (list (region-beginning) (region-end))
>                  (list (point-min) (point-max))))
>   ;; (message "Args: %S R: %S I: %S"
>   ;;          args (use-region-p) (called-interactively-p 'interactive))
>   (when (and (eq (first args) (point-min))
>              (eq (second args) (point-max)))
>     (message "Executing %s on the whole buffer."
>              (propertize (symbol-name this-command)
>                          'face 'font-lock-function-name-face)))
>   nil)

Two more comments:

1.  If you want to show the name of the adviced function instead of the
current command: you know it when you install the advice (in your
dolist) - use it when defining the advice.  Then you can't use the same
constantpiece of advice for all functions, of course.

But if it's only for debugging, you don't need this anymore, I think.
If you need to debug, I recommend to use trace.el instead:

   M-x trace-function your-adviced-function-here

this will show you which arguments the function received (from the
interactive call, or when it had been called from Lisp, the arguments it
received from there).


2.  The nil return value is insignificant.  Stefan used it only to avoid
an empty function body in his advice.  Actually, as you see from the
semantics:

‘:before’	(lambda (&rest r) (apply FUNCTION r) (apply OLDFUN r))

the return value of calling the advice FUNCTION is not used.


Regards,

Michael.

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Kaushal Modi <kaushal.modi@gmail.com> writes:
>
> > OK, so this is what I have now.
> >
> > (defun modi/advice-region-or-whole (&rest args)
> >   "Advice function that sets the region boundaries to that of the whole
> > buffer
> > if no region is selected."
> >   (interactive (if (use-region-p)
> >                    (list (region-beginning) (region-end))
> >                  (list (point-min) (point-max))))
> >   ;; (message "Args: %S R: %S I: %S"
> >   ;;          args (use-region-p) (called-interactively-p 'interactive))
> >   (when (and (eq (first args) (point-min))
> >              (eq (second args) (point-max)))
> >     (message "Executing %s on the whole buffer."
> >              (propertize (symbol-name this-command)
> >                          'face 'font-lock-function-name-face)))
> >   nil)
>

Oh, and `first', `second' are defined in the old cl.el.  It's
recommended to use `cl-lib' now, and thus `cl-first', `cl-second'.
Or simply `car' and `cadr'.

Michael.

Re: Help setting nadvice for indent-region

[Kaushal Modi]

Hi Michael,

>> So with a let-bound variable 'ret', I can have something like
>> (setq ret (apply orig-fn args))
>> and return 'ret' at the end of the :around advice function
definition. Right?
> Sure.

I now use prog1! :)

> If you want to show the name of the adviced function instead of the
> current command: you know it when you install the advice (in your
> dolist) - use it when defining the advice.  Then you can't use the same
> constantpiece of advice for all functions, of course.

Right, that's why my first approach was to use macro to generate an
individual advice fn for each fn I was advising. But for my purpose,
this-command works well.

> But if it's only for debugging, you don't need this anymore, I think.

I'd love to have that message always shown (not just for debug) for a piece
of mind and a good feedback that something got applied on the whole buffer
without me selecting the whole buffer. eval-region, especially provides no
feedback. So it is good to see that message when eval-region happens on the
whole buffer.

> If you need to debug, I recommend to use trace.el instead:
   M-x trace-function your-adviced-function-here

Thanks. I will be using that for future debugs.

> The nil return value is insignificant.  Stefan used it only to avoid
an empty function body in his advice.

Thanks for that info.

> Oh, and `first', `second' are defined in the old cl.el.  It's
> recommended to use `cl-lib' now, and thus `cl-first', `cl-second'.
> Or simply `car' and `cadr'.

Noted, thanks! :)

With that, I now use the below as the solution which helps achieve these
goals:
(1) Apply indent-region/eval-region over the whole buffer if a region is
not selected (and even if (mark) returns nil).
(2) Print an assuring message that the function was applied over the whole
buffer AFTER applying the orig-fun. This is because `indent-region` prints
out an "Indenting region .." message, and if I want to display my "Executed
.. on the whole buffer." message, I need to do it after ORIG-FUN is
applied. So I cannot do that in the (interactive ..) form.

==== Current solution =====

(defvar modi/region-or-whole-fns '(indent-region
                                   eval-region)
  "List of functions to act on the whole buffer if no region is selected.")

(defun modi/advice-region-or-whole (orig-fun &rest args)
  "Advice function that applies ORIG-FUN to the whole buffer if no region is
selected.
http://thread.gmane.org/gmane.emacs.help/109025/focus=109102 "
  ;; Required to override the "r" argument of `interactive' in functions
like
  ;; `indent-region' so that they can be called without an active region.
  (interactive (if (use-region-p)
                   (list (region-beginning) (region-end))
                 (list (point-min) (point-max))))
  (prog1 ; Return value of the advising fn needs to be the same as ORIG-FUN
      (apply orig-fun args)
    (when (and (called-interactively-p 'interactive)
               (not (use-region-p)))
      (message "Executed %s on the whole buffer."
               (propertize (symbol-name this-command)
                           'face 'font-lock-function-name-face)))))

(dolist (fn modi/region-or-whole-fns)
  (advice-add fn :around #'modi/advice-region-or-whole))

=====

It's good to see my initial buggy overly complicated macro-based solution
boil down to this more robust (, apparently bug-free), canonical and a
relatively simpler one :)

Re: Help setting nadvice for indent-region

[Michael Heerdegen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Right, that's why my first approach was to use macro to generate an
> individual advice fn for each fn I was advising. But for my purpose,
> this-command works well.

FWIW, you can achieve this also without using a macro.

> With that, I now use the below as the solution which helps achieve these
> goals:
> (1) Apply indent-region/eval-region over the whole buffer if a region is
> not selected (and even if (mark) returns nil).
> (2) Print an assuring message that the function was applied over the whole
> buffer AFTER applying the orig-fun. This is because `indent-region` prints
> out an "Indenting region .." message, and if I want to display my "Executed
> .. on the whole buffer." message, I need to do it after ORIG-FUN is
> applied. So I cannot do that in the (interactive ..) form.
>
> ==== Current solution =====
>
> (defvar modi/region-or-whole-fns '(indent-region
>                                    eval-region)
>   "List of functions to act on the whole buffer if no region is selected.")
>
> (defun modi/advice-region-or-whole (orig-fun &rest args)
>   "Advice function that applies ORIG-FUN to the whole buffer if no region is
> selected.
> http://thread.gmane.org/gmane.emacs.help/109025/focus=109102 "
>   ;; Required to override the "r" argument of `interactive' in functions
> like
>   ;; `indent-region' so that they can be called without an active region.
>   (interactive (if (use-region-p)
>                    (list (region-beginning) (region-end))
>                  (list (point-min) (point-max))))
>   (prog1 ; Return value of the advising fn needs to be the same as ORIG-FUN
>       (apply orig-fun args)
>     (when (and (called-interactively-p 'interactive)
>                (not (use-region-p)))
>       (message "Executed %s on the whole buffer."
>                (propertize (symbol-name this-command)
>                            'face 'font-lock-function-name-face)))))
>
> (dolist (fn modi/region-or-whole-fns)
>   (advice-add fn :around #'modi/advice-region-or-whole))

That looks good.

> It's good to see my initial buggy overly complicated macro-based
> solution boil down to this more robust (, apparently bug-free),
> canonical and a relatively simpler one :)

The approach was ok, though I would have tried to avoid using a macro.
But well, it also changed the functions, not the interactive specs...


Michael.