bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Reiner Steib]

Hi,

> Please describe exactly what actions triggered the bug
> and the precise symptoms of the bug:

- C-x C-f lisp/gnus/gnus.el RET

- M-x vc-annotate RET

- D

Instead of showing diff of revision at line (as in Emacs 22), I get:

,----[ *Messages* ]
| vc-annotate-show-changeset-diff-revision-at-line:
| The CVS backend does not support changeset diffs
`----

Is this change intended?  I could understand that `D' should now work
on change sets, but please at least mention it in NEWS and tell the
user to try `d' instead or better just display the warning and than
call `vc-annotate-show-diff-revision-at-line' if no change set
operation is available.

- L

Instead of showing log of revision at line (as in Emacs 22), nothing
happens.

If there's no useful binding for `L', why not bind it to
`vc-annotate-show-log-revision-at-line' or at least tell the user to
use `l' (lowercase L) and also document it in NEWS.

I finally found something in NEWS (first I search for "vc-annotate"
without success):

,----[ NEWS ]
| *** In VC Annotate mode, for VC systems that support changesets, you can
| see the diff for the whole changeset (not only for the current file)
| by typing the D key.  Using the "Show changeset diff of revision at
| line" menu entry does the same thing.
`----

> In GNU Emacs 23.0.91.1 (i686-pc-linux-gnu, GTK+ Version 2.12.9)
>  of 2009-03-08 on primula
> Windowing system distributor `The X.Org Foundation', version 11.0.10400090

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Dan Nicolaescu]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

  > Hi,
  > 
  > > Please describe exactly what actions triggered the bug
  > > and the precise symptoms of the bug:
  > 
  > - C-x C-f lisp/gnus/gnus.el RET
  > 
  > - M-x vc-annotate RET
  > 
  > - D
  > 
  > Instead of showing diff of revision at line (as in Emacs 22), I get:
  > 
  > ,----[ *Messages* ]
  > | vc-annotate-show-changeset-diff-revision-at-line:
  > | The CVS backend does not support changeset diffs
  > `----
  > 
  > Is this change intended?  

It is. 

  > I could understand that `D' should now work
  > on change sets, but please at least mention it in NEWS and tell the
  > user to try `d' instead or better just display the warning and than
  > call `vc-annotate-show-diff-revision-at-line' if no change set
  > operation is available.

I am not sure that is better.  But if you can suggest a better wording
for the error message, go for it.

  > - L
  > 
  > Instead of showing log of revision at line (as in Emacs 22), nothing
  > happens.
  > 
  > If there's no useful binding for `L', why not bind it to
  > `vc-annotate-show-log-revision-at-line' or at least tell the user to
  > use `l' (lowercase L) and also document it in NEWS.

That would be a bad idea.  All the bindings for vc-annotate were upper
case.  And that was only because vc-annotate was derived from
`view-mode' which was binding almost all lower case keys.
With more keys to bind we can add better functionality to vc-annotate
(like the 'd' and 'D' bindings), so it would be a bad idea to 
Yes, it would take a tiny bit of effort for the users to get used to the
lower case bindings, but given that they are easier to type it should
not be too bad.
I'll make a note in NEWS about the changed bindings.

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Dan Nicolaescu]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

  > Hi,
  > 
  > > Please describe exactly what actions triggered the bug
  > > and the precise symptoms of the bug:
  > 
  > - C-x C-f lisp/gnus/gnus.el RET
  > 
  > - M-x vc-annotate RET
  > 
  > - D
  > 
  > Instead of showing diff of revision at line (as in Emacs 22), I get:
  > 
  > ,----[ *Messages* ]
  > | vc-annotate-show-changeset-diff-revision-at-line:
  > | The CVS backend does not support changeset diffs
  > `----
  > 
  > Is this change intended?  

It is. 

  > I could understand that `D' should now work
  > on change sets, but please at least mention it in NEWS and tell the
  > user to try `d' instead or better just display the warning and than
  > call `vc-annotate-show-diff-revision-at-line' if no change set
  > operation is available.

I am not sure that is better.  But if you can suggest a better wording
for the error message, go for it.

  > - L
  > 
  > Instead of showing log of revision at line (as in Emacs 22), nothing
  > happens.
  > 
  > If there's no useful binding for `L', why not bind it to
  > `vc-annotate-show-log-revision-at-line' or at least tell the user to
  > use `l' (lowercase L) and also document it in NEWS.

That would be a bad idea.  All the bindings for vc-annotate were upper
case.  And that was only because vc-annotate was derived from
`view-mode' which was binding almost all lower case keys.
With more keys to bind we can add better functionality to vc-annotate
(like the 'd' and 'D' bindings), so it would be a bad idea to 
Yes, it would take a tiny bit of effort for the users to get used to the
lower case bindings, but given that they are easier to type it should
not be too bad.
I'll make a note in NEWS about the changed bindings.

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Reiner Steib]

On Mon, Mar 09 2009, Dan Nicolaescu wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
>   > - D
>   > 
>   > Instead of showing diff of revision at line (as in Emacs 22), I get:
>   > 
>   > ,----[ *Messages* ]
>   > | vc-annotate-show-changeset-diff-revision-at-line:
>   > | The CVS backend does not support changeset diffs
>   > `----
>   > 
>   > Is this change intended?  
>
> It is. 
>
>   > I could understand that `D' should now work
>   > on change sets, but please at least mention it in NEWS and tell the
>   > user to try `d' instead or better just display the warning and than
>   > call `vc-annotate-show-diff-revision-at-line' if no change set
>   > operation is available.
>
> I am not sure that is better.  But if you can suggest a better wording
> for the error message, go for it.

How about this?  (Same in `log-view-diff-changeset', probably.)

--8<---------------cut here---------------start------------->8---
--- vc-annotate.el	09 Jan 2009 09:52:59 +0100	1.7
+++ vc-annotate.el	09 Mar 2009 20:35:28 +0100	
@@ -506,7 +506,10 @@
   "Visit the diff of the revision at line from its previous revision for all files in the changeset."
   (interactive)
   (when (eq 'file (vc-call-backend vc-annotate-backend 'revision-granularity))
-    (error "The %s backend does not support changeset diffs" vc-annotate-backend))
+    (error
+     (substitute-command-keys "The %s backend does not support changeset diffs.  \
+Use \\[vc-annotate-show-diff-revision-at-line] to diff this file.")
+     vc-annotate-backend))
   (vc-annotate-show-diff-revision-at-line-internal nil))
 
 (defun vc-annotate-warp-revision (revspec)
--8<---------------cut here---------------end--------------->8---

>   > - L
>   > 
>   > Instead of showing log of revision at line (as in Emacs 22), nothing
>   > happens.
>   > 
>   > If there's no useful binding for `L', why not bind it to
>   > `vc-annotate-show-log-revision-at-line' or at least tell the user to
>   > use `l' (lowercase L) and also document it in NEWS.
>
> That would be a bad idea.  

Do you also consider the following as bad?  If a future version
actually has a useful command for `L', this can be removed.

--8<---------------cut here---------------start------------->8---
--- vc-annotate.el.~1.7.~	2009-01-09 09:52:59.000000000 +0100
+++ vc-annotate.el	2009-03-09 20:45:47.000000000 +0100
@@ -124,6 +124,12 @@
     (define-key m "f" 'vc-annotate-find-revision-at-line)
     (define-key m "j" 'vc-annotate-revision-at-line)
     (define-key m "l" 'vc-annotate-show-log-revision-at-line)
+    (define-key m "L"
+      (lambda ()
+	(interactive)
+	(error
+	 (substitute-command-keys
+	  "Use \\[vc-annotate-show-log-revision-at-line] to view log."))))
     (define-key m "n" 'vc-annotate-next-revision)
     (define-key m "p" 'vc-annotate-prev-revision)
     (define-key m "w" 'vc-annotate-working-revision)
--8<---------------cut here---------------end--------------->8---

> All the bindings for vc-annotate were upper case.  And that was only
> because vc-annotate was derived from `view-mode' which was binding
> almost all lower case keys.  With more keys to bind we can add
> better functionality to vc-annotate (like the 'd' and 'D' bindings),
> so it would be a bad idea to 

I agree that the lower case binding are better, but the transition
should be made as smooth as possible.

> Yes, it would take a tiny bit of effort for the users to get used to
> the lower case bindings, but given that they are easier to type it
> should not be too bad.

My first reaction was that I really thought these feature won't work
anymore Emacs 23 because of the (partial) rewrite of Emacs' VC system.

> I'll make a note in NEWS about the changed bindings.

Thanks.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Dan Nicolaescu]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

  > On Mon, Mar 09 2009, Dan Nicolaescu wrote:
  > 
  > > Reiner Steib <reinersteib+gmane@imap.cc> writes:
  > >   > - D
  > >   > 
  > >   > Instead of showing diff of revision at line (as in Emacs 22), I get:
  > >   > 
  > >   > ,----[ *Messages* ]
  > >   > | vc-annotate-show-changeset-diff-revision-at-line:
  > >   > | The CVS backend does not support changeset diffs
  > >   > `----
  > >   > 
  > >   > Is this change intended?  
  > >
  > > It is. 
  > >
  > >   > I could understand that `D' should now work
  > >   > on change sets, but please at least mention it in NEWS and tell the
  > >   > user to try `d' instead or better just display the warning and than
  > >   > call `vc-annotate-show-diff-revision-at-line' if no change set
  > >   > operation is available.
  > >
  > > I am not sure that is better.  But if you can suggest a better wording
  > > for the error message, go for it.
  > 
  > How about this?  (Same in `log-view-diff-changeset', probably.)
  > 
  > --- vc-annotate.el	09 Jan 2009 09:52:59 +0100	1.7
  > +++ vc-annotate.el	09 Mar 2009 20:35:28 +0100	
  > @@ -506,7 +506,10 @@
  >    "Visit the diff of the revision at line from its previous revision for all files in the changeset."
  >    (interactive)
  >    (when (eq 'file (vc-call-backend vc-annotate-backend 'revision-granularity))
  > -    (error "The %s backend does not support changeset diffs" vc-annotate-backend))
  > +    (error
  > +     (substitute-command-keys "The %s backend does not support changeset diffs.  \
  > +Use \\[vc-annotate-show-diff-revision-at-line] to diff this file.")
  > +     vc-annotate-backend))
  >    (vc-annotate-show-diff-revision-at-line-internal nil))
  >  
  >  (defun vc-annotate-warp-revision (revspec)
  > 
  > >   > - L
  > >   > 
  > >   > Instead of showing log of revision at line (as in Emacs 22), nothing
  > >   > happens.
  > >   > 
  > >   > If there's no useful binding for `L', why not bind it to
  > >   > `vc-annotate-show-log-revision-at-line' or at least tell the user to
  > >   > use `l' (lowercase L) and also document it in NEWS.
  > >
  > > That would be a bad idea.  
  > 
  > Do you also consider the following as bad?  

Yup.  There are other keys too.  So the choice is not just do `L', it's
do all or none.  I think none is preferable in order to avoid
complications and future burden.

  > > Yes, it would take a tiny bit of effort for the users to get used to
  > > the lower case bindings, but given that they are easier to type it
  > > should not be too bad.
  > 
  > My first reaction was that I really thought these feature won't work
  > anymore Emacs 23 because of the (partial) rewrite of Emacs' VC system.

Users can look at the menus, and see the functions are still there.  For
experienced users that don't use menus, they can figure out in a number
of ways what the key bindings are.

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Stefan Monnier]

> --8<---------------cut here---------------start------------->8---
> --- vc-annotate.el	09 Jan 2009 09:52:59 +0100	1.7
> +++ vc-annotate.el	09 Mar 2009 20:35:28 +0100	
> @@ -506,7 +506,10 @@
>    "Visit the diff of the revision at line from its previous revision for all files in the changeset."
>    (interactive)
>    (when (eq 'file (vc-call-backend vc-annotate-backend 'revision-granularity))
> -    (error "The %s backend does not support changeset diffs" vc-annotate-backend))
> +    (error
> +     (substitute-command-keys "The %s backend does not support changeset diffs.  \
> +Use \\[vc-annotate-show-diff-revision-at-line] to diff this file.")
> +     vc-annotate-backend))
>    (vc-annotate-show-diff-revision-at-line-internal nil))
 
We could also fallback to the single-file diff code when the fileset is
a singleton.


        Stefan

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Dan Nicolaescu]

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

  > > --8<---------------cut here---------------start------------->8---
  > > --- vc-annotate.el	09 Jan 2009 09:52:59 +0100	1.7
  > > +++ vc-annotate.el	09 Mar 2009 20:35:28 +0100	
  > > @@ -506,7 +506,10 @@
  > >    "Visit the diff of the revision at line from its previous revision for all files in the changeset."
  > >    (interactive)
  > >    (when (eq 'file (vc-call-backend vc-annotate-backend 'revision-granularity))
  > > -    (error "The %s backend does not support changeset diffs" vc-annotate-backend))
  > > +    (error
  > > +     (substitute-command-keys "The %s backend does not support changeset diffs.  \
  > > +Use \\[vc-annotate-show-diff-revision-at-line] to diff this file.")
  > > +     vc-annotate-backend))
  > >    (vc-annotate-show-diff-revision-at-line-internal nil))
  >  
  > We could also fallback to the single-file diff code when the fileset is
  > a singleton.

This is vc-annotate, the fileset (if you are referring to the vc.el
notion of fileset) is always singleton.

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Stefan Monnier]

>> > --8<---------------cut here---------------start------------->8---
>> > --- vc-annotate.el	09 Jan 2009 09:52:59 +0100	1.7
>> > +++ vc-annotate.el	09 Mar 2009 20:35:28 +0100	
>> > @@ -506,7 +506,10 @@
>> >    "Visit the diff of the revision at line from its previous revision for all files in the changeset."
>> >    (interactive)
>> >    (when (eq 'file (vc-call-backend vc-annotate-backend 'revision-granularity))
>> > -    (error "The %s backend does not support changeset diffs" vc-annotate-backend))
>> > +    (error
>> > +     (substitute-command-keys "The %s backend does not support changeset diffs.  \
>> > +Use \\[vc-annotate-show-diff-revision-at-line] to diff this file.")
>> > +     vc-annotate-backend))
>> >    (vc-annotate-show-diff-revision-at-line-internal nil))
>> 
>> We could also fallback to the single-file diff code when the fileset is
>> a singleton.

> This is vc-annotate, the fileset (if you are referring to the vc.el
> notion of fileset) is always singleton.

I meant changeset.  But, yes, I now realize that we don't know the size
of the changeset, so we can't do any better than what we do now.


        Stefan

bug#2604: 23.0.91; key bindings in vc-annotate incompatible w/ Emacs 22

[Dan Nicolaescu]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

  > On Mon, Mar 09 2009, Dan Nicolaescu wrote:
  > 
  > > Reiner Steib <reinersteib+gmane@imap.cc> writes:
  > >   > - D
  > >   > 
  > >   > Instead of showing diff of revision at line (as in Emacs 22), I get:
  > >   > 
  > >   > ,----[ *Messages* ]
  > >   > | vc-annotate-show-changeset-diff-revision-at-line:
  > >   > | The CVS backend does not support changeset diffs
  > >   > `----
  > >   > 
  > >   > Is this change intended?  
  > >
  > > It is. 
  > >
  > >   > I could understand that `D' should now work
  > >   > on change sets, but please at least mention it in NEWS and tell the
  > >   > user to try `d' instead or better just display the warning and than
  > >   > call `vc-annotate-show-diff-revision-at-line' if no change set
  > >   > operation is available.
  > >
  > > I am not sure that is better.  But if you can suggest a better wording
  > > for the error message, go for it.
  > 
  > How about this?  (Same in `log-view-diff-changeset', probably.)
  > 
  > --- vc-annotate.el	09 Jan 2009 09:52:59 +0100	1.7
  > +++ vc-annotate.el	09 Mar 2009 20:35:28 +0100	
  > @@ -506,7 +506,10 @@
  >    "Visit the diff of the revision at line from its previous revision for all files in the changeset."
  >    (interactive)
  >    (when (eq 'file (vc-call-backend vc-annotate-backend 'revision-granularity))
  > -    (error "The %s backend does not support changeset diffs" vc-annotate-backend))
  > +    (error
  > +     (substitute-command-keys "The %s backend does not support changeset diffs.  \
  > +Use \\[vc-annotate-show-diff-revision-at-line] to diff this file.")
  > +     vc-annotate-backend))
  >    (vc-annotate-show-diff-revision-at-line-internal nil))
  >  
  >  (defun vc-annotate-warp-revision (revspec)

I am not too convinced that this is better, but if you think it's
better, then feel free to go ahead and change it.

I added a note in NEWS about the key bindings.