comment-kill and the state of the world

comment-kill and the state of the world

[Ian Zimmerman]

Why in the world does comment-kill reindent the affected line?  Do other
people also feel this is a bug?  Maybe it cannot be changed for
compatibility reasons, but then another command should be created with
the desired behaviour.

Look at newcomment.el, one gets the general feeling of unfinished
business :-)

GNU Emacs 21.2.1

-- 
Wer Schoenheit angeschaut mit Augen, hat dem Tode schon Anheim gegeben.
Von Platen.

Re: comment-kill and the state of the world

[Stefan Monnier]

> Why in the world does comment-kill reindent the affected line?

No idea.  You're the first person I know who uses this command.

> Do other people also feel this is a bug?  Maybe it cannot be changed for
> compatibility reasons, but then another command should be created with the
> desired behaviour.

I don't think compatibility is a big problem.  When I re-wrote it in
newcomment.el, I just preserved the old behavior to reduce the risk of
people complaining that "the new code is broken".

> Look at newcomment.el, one gets the general feeling of unfinished
> business :-)

Thanks for the support.  But please make it more specific, so I get
a chance of fixing or at least defending my code.


        Stefan

Re: comment-kill and the state of the world

[Ian Zimmerman]

Ian> Why in the world does comment-kill reindent the affected line?

Stefan> No idea.  You're the first person I know who uses this command.

Ah, but you get the same thing with comment-dwim with an argument on a
line with an existing comment.  Is that any better?

Ian> Look at newcomment.el, one gets the general feeling of unfinished
Ian> business :-)

Stefan> Thanks for the support.  But please make it more specific, so I
Stefan> get a chance of fixing or at least defending my code.

It comes down to comment-dwim.  It really tries to do too much.  The
different situations should be separated, and then common patterns will
emerge to make into subroutines.

Right now, comment-dwim calls comment-indent if not a blank line, but
inserts a comment itself on a blank one.  The two braches are quite
different, use different customization variables, etc.  

And, let me repeat myself, the reindent in comment-kill is a bug.

-- 
Wer Schoenheit angeschaut mit Augen, hat dem Tode schon Anheim gegeben.
Von Platen.

Re: comment-kill and the state of the world

[Stefan Monnier]

> Ah, but you get the same thing with comment-dwim with an argument on a
> line with an existing comment.  Is that any better?

The command throuh which you reach the code is not very relevant.

> It comes down to comment-dwim.  It really tries to do too much.  The
> different situations should be separated, and then common patterns will
> emerge to make into subroutines.

Huh?  They are separate.  You can call comment-indent or comment-region
or uncomment-region or comment-kill directly.  It's just that after many
years of using those commands (they were there in Emacs-18 already), it
appeared that most of their combined functionality could be provided with
a single key binding.  Feel free not to use it or to criticize
it constructively.

> Right now, comment-dwim calls comment-indent if not a blank line, but
> inserts a comment itself on a blank one.  The two braches are quite
> different, use different customization variables, etc.

They don't use the same settings because experience indicates that they
should behave differently.  For example, in Lisp, comment-indent should use
a single `;' whereas when inserting a comment on a blank line to be
indented at the same level as code, it should use `;;'.  Also in one
circumstance a space might be desired but not in the other, ...

The way to customize those could be improved.  It is currently mostly due
to historical baggage.  For example, the number of spaces to put after
the comment marker in comment-indent can only be specified directly in
`comment-start', whereas comments on their own line specify it with
`comment-add' (which incidentally cannot remove space from
`comment-start').

Suggestions are welcome, but don't forget that supporting people's current
settings (embedded in packages written in 1997, for example) is important.

> And, let me repeat myself, the reindent in comment-kill is a bug.

You can repeat it all you want here, but it will only get heard by the
powers that be if you post it via M-x report-emacs-bug.
I'd tend to agree, BTW, after thinking about it a bit more.


        Stefan

Re: comment-kill and the state of the world

[Ian Zimmerman]

Ian> Ah, but you get the same thing with comment-dwim with an argument
Ian> on a line with an existing comment.  Is that any better?

Stefan> The command throuh which you reach the code is not very
Stefan> relevant.

I don't understand.  Are you saying people delete comments by hand with,
say, M-; C-p C-p C-k ?  Or is there some other way I don't know about?

Ian> It comes down to comment-dwim.  It really tries to do too much.
Ian> The different situations should be separated, and then common
Ian> patterns will emerge to make into subroutines.

Stefan> Huh?  They are separate.  You can call comment-indent or
Stefan> comment-region or uncomment-region or comment-kill directly.

Yes.  But comment-dwim does more than just dispatch to them.  If you
call them directly, you lose the benefit of that additional code,
whatever it is.

Stefan> It's just that after many years of using those commands (they
Stefan> were there in Emacs-18 already), it appeared that most of their
Stefan> combined functionality could be provided with a single key
Stefan> binding.  Feel free not to use it or to criticize it
Stefan> constructively.

How am I not being constructive?  I am driven by desire to improve the
code.  I feel that comment-dwim, unless changed into a dispatch routine
and nothing else, is a bad idea and getting rid of it would be an
improvement. 

Maybe I should clarify: my point of view is not an end user's one, but a
Lisp programmer's one, specifically one writing a major mode for a
programing language.  I'd like to use the existing generic comment code,
but I find its complexity gets in the way.

Ian> Right now, comment-dwim calls comment-indent if not a blank line,
Ian> but inserts a comment itself on a blank one.  The two braches are
Ian> quite different, use different customization variables, etc.

Stefan> They don't use the same settings because experience indicates
Stefan> that they should behave differently.  For example, in Lisp,
Stefan> comment-indent should use a single `;' whereas when inserting a
Stefan> comment on a blank line to be indented at the same level as
Stefan> code, it should use `;;'.  Also in one circumstance a space
Stefan> might be desired but not in the other, ...

First, comment-indent already tests for an empty line.  Why not add the
special code there, rather than in comment-dwim?

Second, do you have any advice for me when I _want_ them to behave the same?
(Perhaps with the exception of indentation, but comment-indent-function
should take care of that).  Just ignoring comment-dwim doesn't solve it
because comment-region also uses the "other" settings.

Stefan> The way to customize those could be improved.  It is currently
Stefan> mostly due to historical baggage.  For example, the number of
Stefan> spaces to put after the comment marker in comment-indent can
Stefan> only be specified directly in `comment-start', whereas comments
Stefan> on their own line specify it with `comment-add' (which
Stefan> incidentally cannot remove space from `comment-start').

Stefan> Suggestions are welcome, but don't forget that supporting
Stefan> people's current settings (embedded in packages written in 1997,
Stefan> for example) is important.

Isn't that exactly what I wrote in my first post?  And you seemed to
indicate it wans't as important as I thought.

Ian> And, let me repeat myself, the reindent in comment-kill is a bug.

Stefan> You can repeat it all you want here, but it will only get heard
Stefan> by the powers that be if you post it via M-x report-emacs-bug.
Stefan> I'd tend to agree, BTW, after thinking about it a bit more.

OK.  But please, let's stop the acrimonious tone.  Again, all I want is
to improve Emacs.

BTW, another bug is that comment-indent uses (indent-according-to-mode)
to position the comment when comment-indent-function returns nil.  That
makes no sense when there's preceding code on the line (which is always,
now that comment-dwim handles the empty case itself).  Fortunately, this
is easy to work around, I just make sure my comment-indent-function
_never_ returns nil.

-- 
Wer Schoenheit angeschaut mit Augen, hat dem Tode schon Anheim gegeben.
Von Platen.

Re: comment-kill and the state of the world

[Stefan Monnier]

Ian> Ah, but you get the same thing with comment-dwim with an argument
Ian> on a line with an existing comment.  Is that any better?
Stefan> The command throuh which you reach the code is not very
Stefan> relevant.
> I don't understand.  Are you saying people delete comments by hand with,
> say, M-; C-p C-p C-k ?  Or is there some other way I don't know about?

Looks like I don't understand either.  You complained about a behavior of
comment-kill and I replied that I had no idea why the behavior was like
that and that I didn't know anybody who used comment-kill.
Then you said "but the same applies
to comment-dwim", at which point I tried to clear up the fact that
my earlier answer applied to the comment-kill function as much as
the command.

Ian> It comes down to comment-dwim.  It really tries to do too much.
Ian> The different situations should be separated, and then common
Ian> patterns will emerge to make into subroutines.

Stefan> Huh?  They are separate.  You can call comment-indent or
Stefan> comment-region or uncomment-region or comment-kill directly.

> Yes.  But comment-dwim does more than just dispatch to them.  If you
> call them directly, you lose the benefit of that additional code,
> whatever it is.

The only non-dispatch part of the code is the part that inserts
a comment on an empty line.  Are you saying that we should move
that code out into its own function?  That'd be fine by me.

> How am I not being constructive?  I am driven by desire to improve the
> code.  I feel that comment-dwim, unless changed into a dispatch routine
> and nothing else, is a bad idea and getting rid of it would be an
> improvement. 

I'm sorry, I felt you were not being constructive, whereas what is going on
is just that I have difficulty understand what you want.
`comment-dwim' is meant to be just a dispatch function, so if there's
a part that isn't and that you need it to be, it should be easy to fix.

> Maybe I should clarify: my point of view is not an end user's one, but a
> Lisp programmer's one, specifically one writing a major mode for a
> programing language.  I'd like to use the existing generic comment code,
> but I find its complexity gets in the way.

Can you say precisely which part?

> First, comment-indent already tests for an empty line.  Why not add the
> special code there, rather than in comment-dwim?

History.  `comment-indent' (or more specifically indent-for-comment)
in Emacs-20 inserted a comment at `comment-column' when called on an
empty line.  I did not dare to change this behavior.

> Second, do you have any advice for me when I _want_ them to behave the same?

Which difference are you referring to ?
Since I have trouble understanding your problems, please try to be
very precise and concrete.

> (Perhaps with the exception of indentation, but comment-indent-function
> should take care of that).  Just ignoring comment-dwim doesn't solve it
> because comment-region also uses the "other" settings.

Stefan> The way to customize those could be improved.  It is currently
Stefan> mostly due to historical baggage.  For example, the number of
Stefan> spaces to put after the comment marker in comment-indent can
Stefan> only be specified directly in `comment-start', whereas comments
Stefan> on their own line specify it with `comment-add' (which
Stefan> incidentally cannot remove space from `comment-start').

Stefan> Suggestions are welcome, but don't forget that supporting
Stefan> people's current settings (embedded in packages written in 1997,
Stefan> for example) is important.

> Isn't that exactly what I wrote in my first post?  And you seemed to
> indicate it wans't as important as I thought.

It's important for things that are used.  I don't think the behavior of
comment-kill is that important, I expect that we can convince RMS to
change it.  OTOH, changing the way comment-indent and comment-region
are configured by elisp packages is a lot more difficult because
the elisp packages might not be maintained any more and those
functions are used heavily, so backward compatibility is a lot
more constraining.

> BTW, another bug is that comment-indent uses (indent-according-to-mode)
> to position the comment when comment-indent-function returns nil.  That
> makes no sense when there's preceding code on the line

Indeed, that's a clear bug.  Does the patch below fix it for you ?

> (which is always, now that comment-dwim handles the empty case itself).

It's not always: comment-indent can still be called directly by
the user, and (more importantly), it is called by comment-indent-new-line,
i.e. by the auto-fill code.

> Fortunately, this is easy to work around, I just make sure my
> comment-indent-function _never_ returns nil.

The nil return value is new in Emacs-21, and obviously not
well-tested yet.  Thanks for your report,


        Stefan


Index: newcomment.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/newcomment.el,v
retrieving revision 1.68
diff -u -u -b -r1.68 newcomment.el
--- newcomment.el	1 Sep 2003 15:45:13 -0000	1.68
+++ newcomment.el	19 Oct 2003 23:07:20 -0000
@@ -501,6 +501,10 @@
       (goto-char begpos)
       ;; Compute desired indent.
       (setq indent (save-excursion (funcall comment-indent-function)))
+      ;; If `indent' is nil and there's code before the comment, we can't
+      ;; use `indent-according-to-mode', so we default to comment-column.
+      (unless (or indent (save-excursion (skip-chars-backward " \t") (bolp)))
+	(setq indent comment-column))
       (if (not indent)
 	  ;; comment-indent-function refuses: delegate to line-indent.
 	  (indent-according-to-mode)

Re: comment-kill and the state of the world

[Ian Zimmerman]

Ian> Ah, but you get the same thing with comment-dwim with an argument
Ian> on a line with an existing comment.  Is that any better?

Stefan> The command throuh which you reach the code is not very
Stefan> relevant.

Ian> I don't understand.  Are you saying people delete comments by hand
Ian> with, say, M-; C-p C-p C-k ?  Or is there some other way I don't
Ian> know about?

Stefan> Looks like I don't understand either.  You complained about a
Stefan> behavior of comment-kill and I replied that I had no idea why
Stefan> the behavior was like that and that I didn't know anybody who
Stefan> used comment-kill.  Then you said "but the same applies to
Stefan> comment-dwim", at which point I tried to clear up the fact that
Stefan> my earlier answer applied to the comment-kill function as much
Stefan> as the command.

So, how _do_ people kill their comments?

Ian> It comes down to comment-dwim.  It really tries to do too much.
Ian> The different situations should be separated, and then common
Ian> patterns will emerge to make into subroutines.

Stefan> Huh?  They are separate.  You can call comment-indent or
Stefan> comment-region or uncomment-region or comment-kill directly.

Ian> Yes.  But comment-dwim does more than just dispatch to them.  If
Ian> you call them directly, you lose the benefit of that additional
Ian> code, whatever it is.

Stefan> The only non-dispatch part of the code is the part that inserts
Stefan> a comment on an empty line.  Are you saying that we should move
Stefan> that code out into its own function?  That'd be fine by me.

I think it should be moved to comment-indent.

Ian> Second, do you have any advice for me when I _want_ them to behave
Ian> the same?

Stefan> Which difference are you referring to ?  Since I have trouble
Stefan> understanding your problems, please try to be very precise and
Stefan> concrete.

Let's say the language's comment delimiters look like this: {- -} .  I
can configure comment-dwim to leave appropriate amount of whitespace and
put the point in the right place, so the situation after an empty
comment is inserted looks like this: {- | -} .  But that won't affect
non-empty lines, because that is handled by comment-indent.  And vice
versa.

Ian> BTW, another bug is that comment-indent uses
Ian> (indent-according-to-mode) to position the comment when
Ian> comment-indent-function returns nil.  That makes no sense when
Ian> there's preceding code on the line

Stefan> Indeed, that's a clear bug.  Does the patch below fix it for you
Stefan> ?

Yes.

Ian> (which is always, now that comment-dwim handles the empty case
Ian> itself).

Stefan> It's not always: comment-indent can still be called directly by
Stefan> the user, and (more importantly), it is called by
Stefan> comment-indent-new-line, i.e. by the auto-fill code.

Ok, good point.

-- 
Wer Schoenheit angeschaut mit Augen, hat dem Tode schon Anheim gegeben.
Von Platen.

Re: comment-kill and the state of the world

[Stefan Monnier]

> So, how _do_ people kill their comments?

Using standard editing commands, I guess.
I myself tend to use C-k, I think.

Stefan> The only non-dispatch part of the code is the part that inserts
Stefan> a comment on an empty line.  Are you saying that we should move
Stefan> that code out into its own function?  That'd be fine by me.

> I think it should be moved to comment-indent.

There are two parts:
1 - move the code out of comment-dwim so that comment-dwim is really just
    a dispatch function.
2 - move the code into comment-indent.

Part 1 can be done safely and easily without any objection.
Part 2 means changing the behavior of the comment-indent command (which does
not bother me but might bother RMS or other users, so you'd have to report
it as a bug or feature request).  `comment-indent' is also used as
a function called from comment-indent-new-line and that code (and its
interaction with comment-indent) is pretty delicate, so the patch would have
to be written carefully a thoroughly tested, which I have no intent to do
unless there's a good reason for it (but feel free to do it).  If the merge
into comment-indent is well done it could definitely be a good thing (and
who knows, it might clean things up).

Could you explain why you'd want the code to be moved into comment-indent?

Ian> Second, do you have any advice for me when I _want_ them to behave
Ian> the same?
Stefan> Which difference are you referring to ?  Since I have trouble
Stefan> understanding your problems, please try to be very precise and
Stefan> concrete.
> Let's say the language's comment delimiters look like this: {- -} .  I
> can configure comment-dwim to leave appropriate amount of whitespace and
> put the point in the right place, so the situation after an empty
> comment is inserted looks like this: {- | -} .  But that won't affect
> non-empty lines, because that is handled by comment-indent.  And vice
> versa.

By "configure comment-dwim", I assume you mean "configure comment-padding".

Note that if you "configure comment-dwim" by changing comment-end and
comment-start instead (by setting them to "{-  " and "  -}" for example),
it will work the same way for comment-indent.

Sometimes you want it to behave the same in both case, sometimes you want it
to behave differently, which is why there are two ways to configure the
spacing.  Admittedly, the `comment-padding' way is easier and very different
from the other, so it's pretty ugly, but that's the way it worked in
Emacs-20, so I had to support it in Emacs-21.  RMS would probably accept
a patch for a new config option something like `comment-padding-foo' which
would be used for comments on non-empty lines and could be set to nil to mean
use the same value as `comment-padding'.  You'd have to come up with a good
name for the variable, tho.  It's just that the need hasn't come up yet
because configuring comment-start and comment-end works as well in all
the actual cases I've come across.

Ian> BTW, another bug is that comment-indent uses
Ian> (indent-according-to-mode) to position the comment when
Ian> comment-indent-function returns nil.  That makes no sense when
Ian> there's preceding code on the line
Stefan> Indeed, that's a clear bug.  Does the patch below fix it for you
Stefan> ?
> Yes.

Thank you.


        Stefan

Re: comment-kill and the state of the world

[Ian Zimmerman]

Stefan> Could you explain why you'd want the code to be moved into
Stefan> comment-indent?

The next part of the thread/post.

Ian> Second, do you have any advice for me when I _want_ them to behave
Ian> the same?

Stefan> Which difference are you referring to ?  Since I have trouble
Stefan> understanding your problems, please try to be very precise and
Stefan> concrete.

Ian> Let's say the language's comment delimiters look like this: {- -} .
Ian> I can configure comment-dwim to leave appropriate amount of
Ian> whitespace and put the point in the right place, so the situation
Ian> after an empty comment is inserted looks like this: {- | -} .  But
Ian> that won't affect non-empty lines, because that is handled by
Ian> comment-indent.  And vice versa.

Stefan> By "configure comment-dwim", I assume you mean "configure
Stefan> comment-padding".

Stefan> Note that if you "configure comment-dwim" by changing
Stefan> comment-end and comment-start instead (by setting them to "{- "
Stefan> and " -}" for example), it will work the same way for
Stefan> comment-indent.

I'd tried that.  comment-indent then leaves point in the wrong place
(right before the second hyphen).

Stefan> Sometimes you want it to behave the same in both case, sometimes
Stefan> you want it to behave differently, which is why there are two
Stefan> ways to configure the spacing.  Admittedly, the
Stefan> `comment-padding' way is easier and very different from the
Stefan> other, so it's pretty ugly, but that's the way it worked in
Stefan> Emacs-20, so I had to support it in Emacs-21.  RMS would
Stefan> probably accept a patch for a new config option something like
Stefan> `comment-padding-foo' which would be used for comments on
Stefan> non-empty lines and could be set to nil to mean use the same
Stefan> value as `comment-padding'.  You'd have to come up with a good
Stefan> name for the variable, tho.  It's just that the need hasn't come
Stefan> up yet because configuring comment-start and comment-end works
Stefan> as well in all the actual cases I've come across.

I think it's just sound engineering.  Even if the two branches can be
made to behave the same now, the more they change the harder it will be
to achieve that.

I'll see what I can do regarding the name :-)

Re: comment-kill and the state of the world

[Stefan Monnier]

Stefan> Note that if you "configure comment-dwim" by changing
Stefan> comment-end and comment-start instead (by setting them to "{- "
Stefan> and " -}" for example), it will work the same way for
Stefan> comment-indent.

> I'd tried that.  comment-indent then leaves point in the wrong place
> (right before the second hyphen).

But that's the case as well if you call `comment-indent' with a pre-existing
comment, right?  So it's an unrelated bug which comment-padding-foo would
not fix (one I remember fixing a while back, but probably only in
Emacs-CVS).  What was happening is that comment-indent skips the comment
starter and the leading spaces to get to the actual content of the comment,
but with an empty comment, that tends to leave you right before the
commend-end which is not what you want when there's padding.

Stefan> Sometimes you want it to behave the same in both case, sometimes
Stefan> you want it to behave differently, which is why there are two
Stefan> ways to configure the spacing.  Admittedly, the
Stefan> `comment-padding' way is easier and very different from the
Stefan> other, so it's pretty ugly, but that's the way it worked in
Stefan> Emacs-20, so I had to support it in Emacs-21.  RMS would
Stefan> probably accept a patch for a new config option something like
Stefan> `comment-padding-foo' which would be used for comments on
Stefan> non-empty lines and could be set to nil to mean use the same
Stefan> value as `comment-padding'.  You'd have to come up with a good
Stefan> name for the variable, tho.  It's just that the need hasn't come
Stefan> up yet because configuring comment-start and comment-end works
Stefan> as well in all the actual cases I've come across.
> I think it's just sound engineering.

Believe me I haven't met many people who hate redundancy as much as I do.

> Even if the two branches can be made to behave the same now, the more they
> change the harder it will be to achieve that.

Feel free to write the patch for it.  It already took significant effort
to get things as they are, such that it all works the way all the major
modes expect it, including using various combinations of comment-start,
comment-end, block-comment-start, block-comment-end, comment-multi-line,
various ways to call auto-fill, ...

And don't forget to come up with a good argument to convince RMS that it's
worth it to change the behavior of comment-indent (he happens to like
code duplication, as odd as it sounds to most of us).

A new pair of eyes will surely help to simplify the code, tho.


        Stefan

Re: comment-kill and the state of the world

[Ian Zimmerman]

Ian> I'd tried that.  comment-indent then leaves point in the wrong
Ian> place (right before the second hyphen).

Stefan> But that's the case as well if you call `comment-indent' with a
Stefan> pre-existing comment, right?  So it's an unrelated bug which
Stefan> comment-padding-foo would not fix (one I remember fixing a while
Stefan> back, but probably only in Emacs-CVS).  What was happening is
Stefan> that comment-indent skips the comment starter and the leading
Stefan> spaces to get to the actual content of the comment, but with an
Stefan> empty comment, that tends to leave you right before the
Stefan> commend-end which is not what you want when there's padding.

Ian> Even if the two branches can be made to behave the same now, the
Ian> more they change the harder it will be to achieve that.

Stefan> Feel free to write the patch for it.  It already took
Stefan> significant effort to get things as they are, such that it all
Stefan> works the way all the major modes expect it, including using
Stefan> various combinations of comment-start, comment-end,
Stefan> block-comment-start, block-comment-end, comment-multi-line,
Stefan> various ways to call auto-fill, ...

Stefan> And don't forget to come up with a good argument to convince RMS
Stefan> that it's worth it to change the behavior of comment-indent (he
Stefan> happens to like code duplication, as odd as it sounds to most of
Stefan> us).

Stefan> A new pair of eyes will surely help to simplify the code, tho.

OK, looks like there's hope :-)

-- 
"Rap music is our punishment for neglecting music education."
An anonymous teacher