VC Development

VC Development

[Nick Roberts]

Dan Nicolaescu writes:

> It's even more strange that version control is so important nowadays,
> and just about everyone seems to have a very strong opinion about it,
> but not enough to want to contribute to VC...

One reason that makes it unattractive to me is the fact that you revert changes
without discussion:

2009-01-31  Dan Nicolaescu  <dann@ics.uci.edu>

	* vc-dir.el (vc-dir-mouse-map):
	(vc-default-dir-printer):
	(vc-dir-find-file-other-window): Undo previous change, it breaks
	the behavior of mouse-2.

This change fixed bug#1098 which I reported in October of last year where I
say:

   In vc-directory, mouse-2 on a [file] toggles the mark.  This is unintuitive
   and inconsistent with with PCL-CVS and Emacs general use of mouse-2 to link
   to something else.  It should visit the file at that point in another
   buffer.

I'm sure that it would have been easy to fix this without reverting the change
that I had already made.  Exactly what behaviour was broken?  I took care to
keep the existing keymap.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: VC Development

[Chong Yidong]

Nick Roberts <nickrob@snap.net.nz> writes:

>> It's even more strange that version control is so important nowadays,
>> and just about everyone seems to have a very strong opinion about it,
>> but not enough to want to contribute to VC...
>
> One reason that makes it unattractive to me is the fact that you
> revert changes without discussion:
>
> 2009-01-31  Dan Nicolaescu  <dann@ics.uci.edu>
>
> 	* vc-dir.el (vc-dir-mouse-map):
> 	(vc-default-dir-printer):
> 	(vc-dir-find-file-other-window): Undo previous change, it breaks
> 	the behavior of mouse-2.

Indeed: please, at minimum, send a message to emacs-devel before
reverting another developer's changes, even if those changes appear to
be "clearly wrong".  (I am making no judgement about the changes in
question.)

Re: VC Development

[Chong Yidong]

Nick Roberts <nickrob@snap.net.nz> writes:

> This change fixed bug#1098 which I reported in October of last year where I
> say:
>
>    In vc-directory, mouse-2 on a [file] toggles the mark.  This is
>    unintuitive and inconsistent with with PCL-CVS and Emacs general
>    use of mouse-2 to link to something else.  It should visit the file
>    at that point in another buffer.

Aha, I remember this bug.

The problem, I think, is that selecting files is so important in vc-dir
that there should be an easy way to do it with the mouse.  However, it's
true that mouse-2 ought to visit files.  The proper approach is probably
to tweak the vc-directory interace slightly, adding a check-box on each
line that toggles whenever you click on it.

Re: VC Development

[Dan Nicolaescu]

Chong Yidong <cyd@stupidchicken.com> writes:

  > Nick Roberts <nickrob@snap.net.nz> writes:
  > 
  > >> It's even more strange that version control is so important nowadays,
  > >> and just about everyone seems to have a very strong opinion about it,
  > >> but not enough to want to contribute to VC...
  > >
  > > One reason that makes it unattractive to me is the fact that you
  > > revert changes without discussion:
  > >
  > > 2009-01-31  Dan Nicolaescu  <dann@ics.uci.edu>
  > >
  > > 	* vc-dir.el (vc-dir-mouse-map):
  > > 	(vc-default-dir-printer):
  > > 	(vc-dir-find-file-other-window): Undo previous change, it breaks
  > > 	the behavior of mouse-2.
  > 
  > Indeed: please, at minimum, send a message to emacs-devel before

Sorry, I gave up on trying to talk to that person a while ago when he
became the founding member of my killfile due to getting tired of his
incessant personal attacks.
I welcome patches, but it's highly inappropriate to check in an
obviously untested change 2 days before the pretest into a well
maintained package without any kind of heads up.  

Seeing obviously wrong stuff like this:
+;; Keep existing map.
+(defvar vc-dir-mouse-map
+  (let ((map vc-dir-mode-map))
+    (define-key map [mouse-2] 'vc-dir-find-file-other-window)
+    map)
+  "Local keymap for visiting a file.")
I don't have time to fix it properly now, so the best thing to do is to
revert it.  I don't have time to waist on an email discussion for such
a trivial issue.

Re: VC Development

[Nick Roberts]

 > Seeing obviously wrong stuff like this:
 > +;; Keep existing map.
 > +(defvar vc-dir-mouse-map
 > +  (let ((map vc-dir-mode-map))
 > +    (define-key map [mouse-2] 'vc-dir-find-file-other-window)
 > +    map)
 > +  "Local keymap for visiting a file.")
 > I don't have time to fix it properly now, so the best thing to do is to
 > revert it.  I don't have time to waist on an email discussion for such
 > a trivial issue.

OK.  I see now that I should have used copy-keymap:

(defvar vc-dir-mouse-map
  (let ((map (copy-keymap vc-dir-mode-map)))
   ...

I would like to see this change go in before the pretest.  Since I'm evidently
in Dan's killfile this must be a request to the maintainers.  In case there
is any confusion, with my proposed change, mouse-2 still toggles the mark when
clicked over the status (first mouse highlight region) but over the filename
(second mouse highlight region) now visits that file in another buffer.

I would also like to see stay-local behaviour working.  I can look at making
vc-svn-registered not use "svn status -v DIR", so that vc-stay-local-p is not
so slow, but clearly it is not worthwhile if Dan is going to revert all my
changes and refuse to discuss the reasons why.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: VC Development

[Chong Yidong]

Nick Roberts <nickrob@snap.net.nz> writes:

> OK.  I see now that I should have used copy-keymap:
>
> (defvar vc-dir-mouse-map
>   (let ((map (copy-keymap vc-dir-mode-map)))
>    ...
>
> I would like to see this change go in before the pretest.  Since I'm
> evidently in Dan's killfile this must be a request to the maintainers.
> In case there is any confusion, with my proposed change, mouse-2 still
> toggles the mark when clicked over the status (first mouse highlight
> region) but over the filename (second mouse highlight region) now
> visits that file in another buffer.

Unfortunately, I am not familiar enough with the VC code to make a good
technical judgement call in this case.  IMO, your change is not an
obvious improvement---the check-box interface I mentioned seems to be a
more intuitive way to toggle the mark with the mouse.  So, I prefer to
stick with the current state of affairs (i.e., how vc-dir has been
behaving since it was written), until that interface can be implemented.

> I would also like to see stay-local behaviour working.  I can look at
> making vc-svn-registered not use "svn status -v DIR", so that
> vc-stay-local-p is not so slow, but clearly it is not worthwhile if
> Dan is going to revert all my changes and refuse to discuss the
> reasons why.

I think using the local directory tree is the correct thing to do.  It
is clearly not feasible to implement and test it before the pretest, but
please try working on it.  (Stefan, any thoughts?)

Re: VC Development

[Nick Roberts]

 > Unfortunately, I am not familiar enough with the VC code to make a good
 > technical judgement call in this case.  IMO, your change is not an
 > obvious improvement---the check-box interface I mentioned seems to be a
 > more intuitive way to toggle the mark with the mouse.

I suspect you've not used vc-dir.  Currently an asterisk displays in the status
area when clicked on with mouse-2 (vc-dir-toggle-mark) and disappears when
clicked on again - just like a check-box.  I'm not suggesting changing this
behaviour.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: VC Development

[Stefan Monnier]

> Seeing obviously wrong stuff like this:
> +;; Keep existing map.
> +(defvar vc-dir-mouse-map
> +  (let ((map vc-dir-mode-map))
> +    (define-key map [mouse-2] 'vc-dir-find-file-other-window)
> +    map)
> +  "Local keymap for visiting a file.")
> I don't have time to fix it properly now, so the best thing to do is to
> revert it.

No, the best thing to do is to report it here (or on emacsbugs).


        Stefan

Re: VC Development

[Stefan Monnier]

>> +  (let ((map vc-dir-mode-map))

> OK.  I see now that I should have used copy-keymap:

>   (let ((map (copy-keymap vc-dir-mode-map)))

Actually, it should inherit instead:

   (let ((map (make-sparse-keymap)))
     (set-keymap-parent map vc-dir-mode-map)

Better yet: use the `keymap' property rather than the `local-map'
property, so the keymap can just contains the `mouse-2' binding without
having to explicitly copy and/or inherit from the local map.

> I would like to see this change go in before the pretest.  Since I'm
> evidently in Dan's killfile this must be a request to the maintainers.
> In case there is any confusion, with my proposed change, mouse-2 still
> toggles the mark when clicked over the status (first mouse highlight
> region) but over the filename (second mouse highlight region) now
> visits that file in another buffer.

Yes, it's a change we want to have installed.

> I would also like to see stay-local behaviour working.  I can look at
> making vc-svn-registered not use "svn status -v DIR", so that
> vc-stay-local-p is not so slow, but clearly it is not worthwhile if
> Dan is going to revert all my changes and refuse to discuss the
> reasons why.

Just send your patches here for approval.


        Stefan

Re: VC Development

[Dan Nicolaescu]

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

  > > Seeing obviously wrong stuff like this:
  > > +;; Keep existing map.
  > > +(defvar vc-dir-mouse-map
  > > +  (let ((map vc-dir-mode-map))
  > > +    (define-key map [mouse-2] 'vc-dir-find-file-other-window)
  > > +    map)
  > > +  "Local keymap for visiting a file.")
  > > I don't have time to fix it properly now, so the best thing to do is to
  > > revert it.
  > 
  > No, the best thing to do is to report it here (or on emacsbugs).

In general, maybe.  But absolutely NOT in this particular case:
obviously broken and clearly untested stuff 2 days before the prestest
and I have no time to properly fix it.  And the person making the change
has shown time and again that is not interested in anything else but
personal attacks.  Sorry, I don't have any intention on wasting time on that.

Re: VC Development

[Stefan Monnier]

>> > Seeing obviously wrong stuff like this:
>> > +;; Keep existing map.
>> > +(defvar vc-dir-mouse-map
>> > +  (let ((map vc-dir-mode-map))
>> > +    (define-key map [mouse-2] 'vc-dir-find-file-other-window)
>> > +    map)
>> > +  "Local keymap for visiting a file.")
>> > I don't have time to fix it properly now, so the best thing to do is to
>> > revert it.
>> 
>> No, the best thing to do is to report it here (or on emacsbugs).

> In general, maybe.  But absolutely NOT in this particular case:
> obviously broken and clearly untested stuff 2 days before the prestest
> and I have no time to properly fix it.  And the person making the change
> has shown time and again that is not interested in anything else but
> personal attacks.  Sorry, I don't have any intention on wasting time on that.

It was a 30s fix.  Don't *ever* undo someone's change without first
reporting the problem here.


        Stefan

Re: VC Development

[Nick Roberts]

 >   > No, the best thing to do is to report it here (or on emacsbugs).
 > 
 > In general, maybe.  But absolutely NOT in this particular case:
 > obviously broken and clearly untested stuff 2 days before the prestest
 > and I have no time to properly fix it.  And the person making the change
 > has shown time and again that is not interested in anything else but
 > personal attacks.

I find such an extreme belief rather disturbing.  It's the opposite, actually:
I have no interest in making personal attacks.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: VC Development

[Nick Roberts]

 > Better yet: use the `keymap' property rather than the `local-map'
 > property, so the keymap can just contains the `mouse-2' binding without
 > having to explicitly copy and/or inherit from the local map.

Like below?

-- 
Nick                                           http://www.inet.net.nz/~nickrob



2009-02-04  Nick Roberts  <nickrob@snap.net.nz>

	* vc-dir.el (vc-dir-find-file-other-window): Allow mouse events.
	(vc-dir-mouse-map): New keymap.
	(vc-default-dir-printer): Use it locally.
	


--- vc-dir.el	01 Feb 2009 12:10:10 +1300	1.34
+++ vc-dir.el	04 Feb 2009 22:31:53 +1300	
@@ -689,9 +689,10 @@ that share the same state."
   (interactive)
   (find-file (vc-dir-current-file)))
 
-(defun vc-dir-find-file-other-window ()
+(defun vc-dir-find-file-other-window (&optional event)
   "Find the file on the current line, in another window."
-  (interactive)
+  (interactive (list last-input-event))
+  (if event (posn-set-point (event-end event)))
   (find-file-other-window (vc-dir-current-file)))
 
 (defun vc-dir-isearch ()
@@ -1145,6 +1146,12 @@ Interactively, a prefix argument means t
    (propertize "Please add backend specific headers here.  It's easy!"
 	       'face 'font-lock-warning-face)))
 
+(defvar vc-dir-mouse-map
+   (let ((map (make-sparse-keymap)))
+     (define-key map [mouse-2] 'vc-dir-find-file-other-window)
+    map)
+  "Local keymap for visiting a file.")
+
 (defun vc-default-dir-printer (backend fileentry)
   "Pretty print FILEENTRY."
   ;; If you change the layout here, change vc-dir-move-to-goal-column.
@@ -1171,7 +1178,8 @@ Interactively, a prefix argument means t
       (if isdir
 	  "Directory\nVC operations can be applied to it\nmouse-3: Pop-up menu"
 	"File\nmouse-3: Pop-up menu")
-      'mouse-face 'highlight))))
+      'mouse-face 'highlight
+      'keymap vc-dir-mouse-map))))
 
 (defun vc-default-extra-status-menu (backend)
   nil)

Re: VC Development

[Stefan Monnier]

>> Better yet: use the `keymap' property rather than the `local-map'
>> property, so the keymap can just contains the `mouse-2' binding without
>> having to explicitly copy and/or inherit from the local map.

> Like below?

That looks OK, yes.
You may want to wrap the code of vc-dir-find-file-other-window inside
a save-selected-window since posn-set-point may change the window.
Also I'd use last-nonmenu-event since that's what is used by
interactive's "e" code.  IIUC that would allow it work if the command is
put on a contextual pop-up menu.


        Stefan


> 2009-02-04  Nick Roberts  <nickrob@snap.net.nz>

> 	* vc-dir.el (vc-dir-find-file-other-window): Allow mouse events.
> 	(vc-dir-mouse-map): New keymap.
> 	(vc-default-dir-printer): Use it locally.
	


> --- vc-dir.el	01 Feb 2009 12:10:10 +1300	1.34
> +++ vc-dir.el	04 Feb 2009 22:31:53 +1300	
> @@ -689,9 +689,10 @@ that share the same state."
>    (interactive)
>    (find-file (vc-dir-current-file)))
 
> -(defun vc-dir-find-file-other-window ()
> +(defun vc-dir-find-file-other-window (&optional event)
>    "Find the file on the current line, in another window."
> -  (interactive)
> +  (interactive (list last-input-event))
> +  (if event (posn-set-point (event-end event)))
>    (find-file-other-window (vc-dir-current-file)))
 
>  (defun vc-dir-isearch ()
> @@ -1145,6 +1146,12 @@ Interactively, a prefix argument means t
>     (propertize "Please add backend specific headers here.  It's easy!"
>  	       'face 'font-lock-warning-face)))
 
> +(defvar vc-dir-mouse-map
> +   (let ((map (make-sparse-keymap)))
> +     (define-key map [mouse-2] 'vc-dir-find-file-other-window)
> +    map)
> +  "Local keymap for visiting a file.")
> +
>  (defun vc-default-dir-printer (backend fileentry)
>    "Pretty print FILEENTRY."
>    ;; If you change the layout here, change vc-dir-move-to-goal-column.
> @@ -1171,7 +1178,8 @@ Interactively, a prefix argument means t
>        (if isdir
>  	  "Directory\nVC operations can be applied to it\nmouse-3: Pop-up menu"
>  	"File\nmouse-3: Pop-up menu")
> -      'mouse-face 'highlight))))
> +      'mouse-face 'highlight
> +      'keymap vc-dir-mouse-map))))
 
>  (defun vc-default-extra-status-menu (backend)
>    nil)

Re: VC Development

[Nick Roberts]

 > That looks OK, yes.
 > You may want to wrap the code of vc-dir-find-file-other-window inside
 > a save-selected-window since posn-set-point may change the window.
 > Also I'd use last-nonmenu-event since that's what is used by
 > interactive's "e" code.  IIUC that would allow it work if the command is
 > put on a contextual pop-up menu.
 
I've used last-nonmenu-event but trying save-selected-window put the file in
the *vc-dir* buffer if that was not already selected - so I gave up.  I think
the window holding the visited file should become the selected window because
that's presumably where the user wants to go.  I was trying to get it to work
like dired-mouse-find-file-other-window, which doesn't move the cursor in the
dired window, but I couldn't find a way to get it to work for all cases.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: VC Development

[Stefan Monnier]

> I've used last-nonmenu-event

Good.

> but trying save-selected-window put the file in the *vc-dir* buffer if
> that was not already selected - so I gave up.  I think the window
> holding the visited file should become the selected window because
> that's presumably where the user wants to go.  I was trying to get it
> to work like dired-mouse-find-file-other-window, which doesn't move
> the cursor in the dired window, but I couldn't find a way to get it to
> work for all cases.

Yes, it's difficult to get it to work really well.  It's OK for now.


        Stefan