Diff faces

Diff faces

[Juri Linkov]

Wouldn't it be better to use the same colors for equivalent faces of
ediff, smerge and diff-mode?  So that ediff-fine-diff-B, diff-fine-change
and smerge-refined-change will be the same and so on.

PS: While looking at ediff faces I discovered a bug: trying to get the
description of ediff faces from the Help buffer created by `C-u C-x ='
fails with (wrong-type-argument symbolp "ediff-fine-diff-B").

Ediff puts face names as strings on overlays, but describe-face accepts
only symbols.  The patch below fixes describe-face to not fail on face
name strings.  As for fixing ediff, I don't know whether this is necessary,
since face names as strings are mostly interchangeable with face symbols.
There are only two places in ediff*.el files that call `face-name' to
convert face symbols to strings, so these calls could be removed.

Index: lisp/faces.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/faces.el,v
retrieving revision 1.377
diff -c -r1.377 faces.el
*** lisp/faces.el	21 Sep 2007 07:23:03 -0000	1.377
--- lisp/faces.el	11 Oct 2007 23:59:05 -0000
***************
*** 1285,1290 ****
--- 1285,1291 ----
        (save-excursion
  	(set-buffer standard-output)
  	(dolist (f face)
+ 	  (if (stringp f) (setq f (intern f)))
  	  (insert "Face: " (symbol-name f))
  	  (if (not (facep f))
  	      (insert "   undefined face.\n")

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff faces

[Stefan Monnier]

> Wouldn't it be better to use the same colors for equivalent faces of
> ediff, smerge and diff-mode?  So that ediff-fine-diff-B, diff-fine-change
> and smerge-refined-change will be the same and so on.

Could be.  My use of faces (i.e. "mostly no colors, only font/size/darkness
changes") is rather atypical and I simply cannot imagine how people live
with the default angry fruit-salad, so I prefer to let other people deal
with it.


        Stefan

Re: Diff faces

[Juri Linkov]

>> Wouldn't it be better to use the same colors for equivalent faces of
>> ediff, smerge and diff-mode?  So that ediff-fine-diff-B, diff-fine-change
>> and smerge-refined-change will be the same and so on.
>
> Could be.  My use of faces (i.e. "mostly no colors, only font/size/darkness
> changes") is rather atypical and I simply cannot imagine how people live
> with the default angry fruit-salad, so I prefer to let other people deal
> with it.

I like the current shadows-of-gray color scheme used in diff-mode.
And even ediff uses similar gray colors to highlight inactive hunks.
The only complaint I currently have is that the yellow color of the new
face `diff-fine-change' doesn't fit into this scheme.  What do you think
about changing it to a shadow of gray?  Alternatively, you could use
fruit-salad colors only for the refined hunk in diff-mode (similar to the
colors of the active hunk in ediff).

BTW, I think the current character-wise refinement in diff-mode and in
smerge-mode is not very useful.  More useful would be to refine it on the
word or symbol basis.  I have an experimental patch that produces good
results.  It splits original hunks symbol-by-symbol (though word-by-word
works as well) and compares them ignoring whitespace differences.

Would you like to improve diff-mode and smerge-mode with something like this?

Index: lisp/smerge-mode.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/smerge-mode.el,v
retrieving revision 1.53
diff -u -r1.53 smerge-mode.el
--- lisp/smerge-mode.el	9 Oct 2007 03:38:56 -0000	1.53
+++ lisp/smerge-mode.el	14 Oct 2007 21:30:03 -0000
@@ -667,22 +667,33 @@
       (insert-buffer-substring buf beg end)
       (when preproc (goto-char (point-min)) (funcall preproc))
       (goto-char (point-min))
+      (while (search-forward "\n" nil t)
+	(replace-match "" nil nil))
+      (goto-char (point-min))
       (while (not (eobp))
-        (forward-char 1)
+        (save-match-data (forward-symbol 1))
         ;; We add \n after each char except after \n, so we get one line per
         ;; text char, where each line contains just one char, except for \n
         ;; chars which are represented by the empty line.
-        (unless (eq (char-before) ?\n) (insert ?\n)))
+	(insert ?\n))
       (let ((coding-system-for-write 'emacs-mule))
         (write-region (point-min) (point-max) file nil 'nomessage)))))
 
 (defun smerge-refine-highlight-change (buf beg match-num1 match-num2 props)
   (let* ((startline (string-to-number (match-string match-num1)))
+	 (len1 (1- startline))
+	 (len2 (if (match-end match-num2)
+		   (string-to-number (match-string match-num2))
+		 startline))
          (ol (make-overlay
-              (+ beg startline -1)
-              (+ beg (if (match-end match-num2)
-                         (string-to-number (match-string match-num2))
-                       startline))
+	      (with-current-buffer buf
+		(goto-char beg)
+		(save-match-data (forward-symbol len1))
+		(point))
+	      (with-current-buffer buf
+		(goto-char beg)
+		(save-match-data (forward-symbol len2))
+		(point))
               buf
               ;; Make them tend to shrink rather than spread when editing.
               'front-advance nil)))
@@ -710,7 +721,7 @@
           (let ((coding-system-for-read 'emacs-mule))
             ;; Don't forget -a to make sure diff treats it as a text file
             ;; even if it contains \0 and such.
-            (call-process diff-command nil t nil "-a" file1 file2))
+            (call-process diff-command nil t nil "-awb" file1 file2))
           ;; Process diff's output.
           (goto-char (point-min))
           (while (not (eobp))

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff faces

[Stefan Monnier]

> I like the current shadows-of-gray color scheme used in diff-mode.
> And even ediff uses similar gray colors to highlight inactive hunks.
> The only complaint I currently have is that the yellow color of the new
> face `diff-fine-change' doesn't fit into this scheme.  What do you think
> about changing it to a shadow of gray?

Sounds fine to me, that's basically what I use.

> Alternatively, you could use fruit-salad colors only for the refined hunk
> in diff-mode (similar to the colors of the active hunk in ediff).

diff-mode doesn't have a notion of active hunk right now and I'm not
convinced I want to add such a notion.

> BTW, I think the current character-wise refinement in diff-mode and in
> smerge-mode is not very useful.  More useful would be to refine it on the
> word or symbol basis.  I have an experimental patch that produces good
> results.  It splits original hunks symbol-by-symbol (though word-by-word
> works as well) and compares them ignoring whitespace differences.

I like it to be finer than that.  I agree that chars are a bit too fine, but
it's rarely a problem.  Basically, I was thinking of refining enough so as
to treat "FooBar64" as 3 entities.

Also I often like the fact that it does not ignore whitespace differences
but I often would like it to ignore them, so I'd like to be able to choose.

> +      (while (search-forward "\n" nil t)
> +	(replace-match "" nil nil))
> +      (goto-char (point-min))

That should be " " rather than "", right?  If so, subst-char-in-region might
be a better choice.


        Stefan

Re: Diff faces

[Juri Linkov]

> I like it to be finer than that.  I agree that chars are a bit too fine, but
> it's rarely a problem.  Basically, I was thinking of refining enough so as
> to treat "FooBar64" as 3 entities.
>
> Also I often like the fact that it does not ignore whitespace differences
> but I often would like it to ignore them, so I'd like to be able to choose.

Perhaps two new options will help selecting the necessary granularity and
whitespace to ignore.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff faces

[Stefan Monnier]

>> I like it to be finer than that.  I agree that chars are a bit too fine, but
>> it's rarely a problem.  Basically, I was thinking of refining enough so as
>> to treat "FooBar64" as 3 entities.
>> 
>> Also I often like the fact that it does not ignore whitespace differences
>> but I often would like it to ignore them, so I'd like to be able to choose.

> Perhaps two new options will help selecting the necessary granularity and
> whitespace to ignore.

My guess is that the "FooBar64" granularity should be both fine enough for
me and large enough to avoid the kind of bogus "correlation" between
unrelated chars.  So we'd only need a choice for whitespace.


        Stefan

Re: Diff faces

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I like the current shadows-of-gray color scheme used in diff-mode.
>> And even ediff uses similar gray colors to highlight inactive hunks.
>> The only complaint I currently have is that the yellow color of the new
>> face `diff-fine-change' doesn't fit into this scheme.  What do you think
>> about changing it to a shadow of gray?
>
> Sounds fine to me, that's basically what I use.

Another consideration is that the current face (bg "yellow") is almost
unusable with a dark background (because there's almost no contrast
between "yellow" and the usual white foreground), and _completely_
unusable on a dark-background tty -- ttys can't use shades of grey, so
diff faces on ttys are fruit-salady, with a yellow _foreground_ for
changed text!

A grey background doesn't really work well either on dark-background
ttys either, as sometimes (but not always...), "grey" is a fairly light
grey color.

-Miles

-- 
自らを空にして、心を開く時、道は開かれる

Re: Diff faces

[Stefan Monnier]

> BTW, I think the current character-wise refinement in diff-mode and in
> smerge-mode is not very useful.  More useful would be to refine it on the
> word or symbol basis.  I have an experimental patch that produces good
> results.  It splits original hunks symbol-by-symbol (though word-by-word
> works as well) and compares them ignoring whitespace differences.

I've installed a similar (tho significantly more complex) patch.
Can you tell me if you like the new behavior?


        Stefan

Re: Diff faces

[Juri Linkov]

>> BTW, I think the current character-wise refinement in diff-mode and in
>> smerge-mode is not very useful.  More useful would be to refine it on the
>> word or symbol basis.  I have an experimental patch that produces good
>> results.  It splits original hunks symbol-by-symbol (though word-by-word
>> works as well) and compares them ignoring whitespace differences.
>
> I've installed a similar (tho significantly more complex) patch.
> Can you tell me if you like the new behavior?

The new behavior is excellent.  Thanks.  I think word granularity is
now more useful even when it highlights whole words that differ only
in one character.

BTW, smerge has a key binding `C-c ^ R' to refine differences, but
diff-mode has `C-c C-b'.  What about using more similar keys like
`C-c R' for diff-mode?  And maybe also add a special key to refine
without ignoring whitespace (by setting smerge-refine-ignore-whitespace
temporarily to nil, or vice versa depending on its customized value)
and to bind it to e.g. `C-c W' and `C-c ^ W'?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff faces

[Andreas Schwab]

Juri Linkov <juri@jurta.org> writes:

> What about using more similar keys like `C-c R' for diff-mode?

C-c <letter> is reserved.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Diff faces

[Juri Linkov]

>> What about using more similar keys like `C-c R' for diff-mode?
>
> C-c <letter> is reserved.

Ediff uses `*' to refine the current region, so what about `C-c *'?
Is it reserved?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Diff faces

[Stefan Monnier]

>>> What about using more similar keys like `C-c R' for diff-mode?
>> C-c <letter> is reserved.

For user bindings, indeed.

> Ediff uses `*' to refine the current region, so what about `C-c *'?

This one is reserved for minor modes.
Check the Coding Convention chapter.


        Stefan

Re: Diff faces

[Stefan Monnier]

> BTW, smerge has a key binding `C-c ^ R' to refine differences, but
> diff-mode has `C-c C-b'.  What about using more similar keys like
> `C-c R' for diff-mode?  And maybe also add a special key to refine
> without ignoring whitespace (by setting smerge-refine-ignore-whitespace
> temporarily to nil, or vice versa depending on its customized value)
> and to bind it to e.g. `C-c W' and `C-c ^ W'?

I never use C-c ^ R nor C-c C-b so I have no opinion.
For smerge, I use smerge-auto-refine (not yet installed) which refines
conflicts when you get to them with C-c ^ n.

For diff, I currently use M-x diff-fin TAB RET (yup: I can never get this
C-c C-b into my head) but I'd like to do diff-auto-refine as well.


        Stefan

Re: Diff faces

[Richard Stallman]

    Ediff puts face names as strings on overlays, but describe-face accepts
    only symbols.  The patch below fixes describe-face to not fail on face
    name strings.  As for fixing ediff, I don't know whether this is necessary,
    since face names as strings are mostly interchangeable with face symbols.
    There are only two places in ediff*.el files that call `face-name' to
    convert face symbols to strings, so these calls could be removed.

I agree describe-face should handle a string as the face property,
since that's a documented feature.  So please install that patch.

However, is there a good reason for ediff to use strings
instead of symbols to specify the faces?

Re: Diff faces

[Michael Kifer]

>     Ediff puts face names as strings on overlays, but describe-face accepts
>     only symbols.  The patch below fixes describe-face to not fail on face
>     name strings.  As for fixing ediff, I don't know whether this is necessary,
>     since face names as strings are mostly interchangeable with face symbols.
>     There are only two places in ediff*.el files that call `face-name' to
>     convert face symbols to strings, so these calls could be removed.
> 
> I agree describe-face should handle a string as the face property,
> since that's a documented feature.  So please install that patch.
> 
> However, is there a good reason for ediff to use strings
> instead of symbols to specify the faces?

I think that in the old days this is what was required.
Or, maybe for compatibility with XEmacs -- do not remember (was 10 years
back or so).

Re: Diff faces

[Richard Stallman]

    > However, is there a good reason for ediff to use strings
    > instead of symbols to specify the faces?

    I think that in the old days this is what was required.

How about changing it and see what happens?

Re: Diff faces

[Michael Kifer]

>     > However, is there a good reason for ediff to use strings
>     > instead of symbols to specify the faces?
> 
>     I think that in the old days this is what was required.
> 
> How about changing it and see what happens?
> 



seems ok. I'll change it