bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

emacs -Q

(defadvice isearch-forward (before isearch+-doc activate)
  "
Isearch Plus
============
...")

`C-h f isearch-forward' does not show this addition to the doc string
at all.  In Emacs 24.3 (and prior releases) there is no such bug.

Furthermore, in the dev snapshot, you see this from `C-h f':

 :around advice: `ad-Advice-isearch-forward'

That should NOT be shown.  That is internal, implementation info.  It is
also incorrect.  The advice is :before, not :around.

And besides missing the added doc string text, this text that introduces
it is also missing (present in Emacs 24.3 and prior, where the first
line is bright red and bold):

 This function is advised.

 Before-advice `isearch+-doc':

That is user-friendly info, and it is correct (:before, not :around).

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-06-20 on ODIEONE
Bzr revision: 113100 eliz@gnu.org-20130620173624-w9v620tog4yacftk
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 CFLAGS=-O0 -g3 LDFLAGS=-Lc:/Devel/emacs/lib
 CPPFLAGS=-Ic:/Devel/emacs/include'

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

tags 14734 wontfix
thanks

>  :around advice: `ad-Advice-isearch-forward'
[...]
> It is also incorrect.  The advice is :before, not :around.

It is correct: the ad-Advice-isearch-forward advice is an around advice
(and it is not your advice, it's internal as you say).
If you click on it, you'll see your before advice, properly labeled.

If you don't want to see the internal thingy, then use the new
advice-add rather than the old defadvice.


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> tags 14734 wontfix
> thanks
> 
> >  :around advice: `ad-Advice-isearch-forward'
> > It is also incorrect.  The advice is :before, not :around.
> 
> It is correct: the ad-Advice-isearch-forward advice is an around advice
> (and it is not your advice, it's internal as you say).
> If you click on it, you'll see your before advice, properly labeled.
> 
> If you don't want to see the internal thingy, then use the new
> advice-add rather than the old defadvice.

How can you close this bug?  Did you read the whole report?

The addition to the doc string, which is the only point of this particular advice, is MISSING from the output of C-h f.  The added text should appear at the end of the original doc string, seamlessly.

Please read the whole report and try to control your knee jerks.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> > If you don't want to see the internal thingy, then use the new
> > advice-add rather than the old defadvice.

So you introduce a regression in user-visible behavior that
degrades the user interface, with the idea that that will
encourage use of your new advice replacement over traditional
defadvice?  Is that it?

Using traditional advice to add to a doc string has always worked
seamlessly: a user saw a single display of help text that included
both the original doc and any doc added by advice, seamlessly.

The change introduced is horrible for users.  Now they see only
the original doc string, plus a link that will be incomprehensible
to many (so skipped over by them):

  :around advice: `ad-Advice-isearch-forward'

And if they do happen to click that uninviting link then they
get an even more incomprehensible page of internal,
advice-oriented code and unintelligible text:

  ad-Advice-isearch-forward is a Lisp function.

  (ad-Advice-isearch-forward AD--ADDOIT-FUNCTION &optional REGEXP-P
  NO-RECURSIVE-EDIT)

  Advice function assembled by advice.el.

  Before-advice `isearch+-doc':
   
  Isearch Plus
  ============
  ...

What they should see for `C-h f isearch-forward', and what they have
always seen in the past, is the original `isearch-forward' doc together
with the added doc from advising - it is just appended:

  Isearch Plus
  ============
  ...

Emacs did that by design.  You have broken that.

Not only can Emacs do better, it always HAS.  This is a real step
backward for users.  Intentional or not.  I'm reopening the bug.
I hope you will seriously consider reverting the misguided changes
that introduced this regression.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Dmitry Gutov]

(Not Stefan.)

Drew Adams <drew.adams@oracle.com> writes:

> The change introduced is horrible for users.  Now they see only
> the original doc string, plus a link that will be incomprehensible
> to many (so skipped over by them):

You're overdramatizing. It's less convenient, but far from "horrible".

> Not only can Emacs do better, it always HAS.  This is a real step
> backward for users.  Intentional or not.  I'm reopening the bug.
> I hope you will seriously consider reverting the misguided changes
> that introduced this regression.

It's obviously the result of advice.el being re-implemented using
nadvice.el, for backward compatibility. Having a minor regression in
functionality in this kind of situation is fairly normal.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> > The change introduced is horrible for users.  Now they see only
> > the original doc string, plus a link that will be incomprehensible
> > to many (so skipped over by them):
> 
> You're overdramatizing. It's less convenient, but far from "horrible".
> 
> > Not only can Emacs do better, it always HAS.  This is a real step
> > backward for users.  Intentional or not.  I'm reopening the bug.
> > I hope you will seriously consider reverting the misguided changes
> > that introduced this regression.
> 
> It's obviously the result of advice.el being re-implemented using
> nadvice.el, for backward compatibility. Having a minor regression in
> functionality in this kind of situation is fairly normal.

If your point of view is only that of an implementor, you see only
"less convenient", "minor regression", and "fairly normal".  You see
the regression as just "a result of ... being reimplemented", as if
design and the user experience do not matter.  Implementation leads.

If your point of view is that of a user, the result is a real step
backward and, yes, pretty horrible.  The design should lead, for users.

One opinion, of course.

And just why did something already implemented (and stable for years)
need to be REimplemented "for backward compatibility"?  Why did adding
something new and different and presumably better require ALSO
reimplementing something that was already, by definition, backward
compatible?

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> And just why did something already implemented (and stable for years)
> need to be REimplemented "for backward compatibility"?  Why did adding
> something new and different and presumably better require ALSO
> reimplementing something that was already, by definition, backward
> compatible?

I had the same question - since reimplementing should have been a bit of
work.  But maintaining the reimplementation is probably much easier than
doing so for the old package, so I can understand this decision.


Michael.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> >  :around advice: `ad-Advice-isearch-forward'
> [...]
> > It is also incorrect.  The advice is :before, not :around.
>
> It is correct: the ad-Advice-isearch-forward advice is an around advice
> (and it is not your advice, it's internal as you say).
> If you click on it, you'll see your before advice, properly labeled.
>
> If you don't want to see the internal thingy, then use the new
> advice-add rather than the old defadvice.

Let me add that advice.el is not yet obsolete, and the manual still
explains the old advice.el.  We should try reach a consistent state,
i.e. C-h f should report information according to the package the user
is told to use.


Michael.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

> Let me add that advice.el is not yet obsolete,

Indeed, but it's only a question of time.

> and the manual still explains the old advice.el.

That's a bug that needs fixing, yes.


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> > Let me add that advice.el is not yet obsolete,
> 
> Indeed, but it's only a question of time.

So much for the argument about "backward compatibility".

N'importe quoi.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

>> > Let me add that advice.el is not yet obsolete,
>> Indeed, but it's only a question of time.
> So much for the argument about "backward compatibility".

What kind of ass comment is that?  You know full well that "obsolete"
does not mean "not backward compatible".


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> >> > Let me add that advice.el is not yet obsolete,
> >> Indeed, but it's only a question of time.
> > So much for the argument about "backward compatibility".
> 
> What kind of ass comment is that?

?

> You know full well that "obsolete" does not mean "not backward
> compatible".

An argument was made that the REimplementation of something that
worked well was done (was necessary?) to allow for backward
compatibility.  I was speaking to that argument.

If that is really the reason for reimplementing the old, and
the old is slated for obsolescence, then that extra "backward
compatibility" work, which degrades the user experience, would
not seem to be worth much, would it?

I have my doubts that providing "backward compatibility" was
the reason for the reimplementation, but I have no special
insight into the reason.  Clearly, someone went to some trouble
to code the old anew, for some reason.

The old code was already backward compatible, by definition.
What was the reason that the `C-h f' behavior needed to change
so negatively?  Please don't say that it is a gift of backward
compatibility.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

> An argument was made that the REimplementation of something that
> worked well was done (was necessary?) to allow for backward
> compatibility.  I was speaking to that argument.

There has been no reimplementation (that would have been a waste of
time, since it's a package planned to become obsolete).  There's only
been some adjustments to make it so that the two advice packages don't
step on each other's feet.  More specifically, the old advice.el was
made to work on top of the new nadvice.el (the total size of the the new
advice.el and nadvice.el is about the same as the old advice.el).

The old advice.el could be reimplemented to use nadvice more directly
(e.g. so that each defadvice turns into a single advice-add, rather
than bundling all defadvice into a single advice-add), which would solve
some of your complaints, but it's a lot of work and would probably break
more backward compatibility because it'd be difficult to preserve some
details of advice.el's semantics.


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Juanma Barranquero]

On Sat, Jun 29, 2013 at 5:24 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> The old advice.el could be reimplemented to use nadvice more directly
> (e.g. so that each defadvice turns into a single advice-add, rather
> than bundling all defadvice into a single advice-add), which would solve
> some of your complaints, but it's a lot of work and would probably break
> more backward compatibility because it'd be difficult to preserve some
> details of advice.el's semantics.

FWIW, I like a lot the new advice system and I understand why does not
make sense to spend much time with the old advices (I'm quite happy to
see the end of ad-do-it and other clumsy macros).

But I also agree with Drew that the current doctrings for adviced
functions are horrible and a big step backwards for users.

    J

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

> But I also agree with Drew that the current doctrings for adviced
> functions are horrible and a big step backwards for users.

Patches welcome,


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> > But I also agree with Drew that the current doctrings for adviced
> > functions are horrible and a big step backwards for users.
> 
> Patches welcome,

The problem is not doc strings but the regression in user interaction.
Perhaps that is what Juanma meant: what the user sees via `C-h f'.

The patch for that is to revert the changes that caused the regression.

There is nothing wrong with providing a new advice mechanism, and a
better one is certainly welcome.  Thank you for working on that.
There is something very wrong, however, if doing that ruins the user
interface.

Advising functions provides a Lisp feature that is usable by Lisp users.
But ALL users make use of `C-h f', and often, and the changes made to
facilitate introducing the new advice have (apparently) broken the user
experience with `C-h f'.

This is a serious regression that deserves to be taken seriously.  The
code that introduced the regression should be backed out immediately, if
fixing it now would be too onerous.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Juanma Barranquero]

On Fri, Jul 5, 2013 at 4:50 PM, Drew Adams <drew.adams@oracle.com> wrote:

> Perhaps that is what Juanma meant: what the user sees via `C-h f'.

Yes.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

> The patch for that is to revert the changes that caused the regression.

If it's so easy, please send it along (but do test it first),


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> > The patch for that is to revert the changes that caused the regression.
> 
> If it's so easy, please send it along (but do test it first),

Oh come on.  No one said it was easy.  Someone implemented the
regressive code and should revert the regression s?he introduced.

(It should have been obvious while implementing it that from a user
point of view it would be a mistake.)

The regression is not in the release (24.3) or prior.  That's the good news.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Stefan Monnier]

> (It should have been obvious while implementing it that from a user
> point of view it would be a mistake.)

Yes, I'm simply an idiot,


        Stefan

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> > (It should have been obvious while implementing it that from
> > a user point of view it would be a mistake.)
> 
> Yes, I'm simply an idiot

Please calm down.  No one questions your intelligence (as your
sarcasm indicates you know full well).

It's about effect on users; it's not only about implementation.

And it is certainly not about you or about intelligence.  Being
brilliant does not mean that one never makes mistakes or always
exercises the best judgment.  "I'm smart, therefore I must be
right" is not always a smart argument.

bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Drew Adams]

> the old advice.el was made to work on top of the new nadvice.el (the
> total size of the new advice.el and nadvice.el is about the same as
> the old advice.el).

But the new advice, and now the old advice too, since it is on top of
the new, does NOT work.  Specifically, there is this regression wrt
doc strings.

This should not be a `wont-fix' bug.  It should be fixed.

It should be fixed in the advice system that is documented in the
manuals, which for now is still the "old" system (there is nothing
about the nadvice stuff in the manuals).

If the "new" system will at some point replace the old and itself be
documented differently, instead of being only, as you say now,
internal, then it too will need to be fixed wrt its handling of the
doc string.

bug#13581: bug#14734: 24.3.50; REGRESSION: defadvice broken wrt doc strings (C-h f)

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> It's obviously the result of advice.el being re-implemented using
> nadvice.el, for backward compatibility. Having a minor regression in
> functionality in this kind of situation is fairly normal.

This was marked as a "wontfix" five+ years ago, and the rough consensus
seems to be that the current look is OK, so I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no