Fix info display of images

Fix info display of images

[David Kastrup]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]


Hi,

the info images in the Lilypond documentation have a rather messed-up
display.  That's more or less due to Emacs putting "invisible"
properties over image code indiscriminately.

I find that the following does help.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 550 bytes --]

diff --git a/lisp/info.el b/lisp/info.el
index 1bd82ef..0d7686d 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -1446,7 +1446,8 @@ any double quotes or backslashes must be escaped (\\\",\\\\)."
 	      "\\(\0[\0-\37][[][^\0]*\0[\0-\37][]]\n?\\)"
 	      nil t)
 	(let* ((start (match-beginning 1)))
-	  (if (not (get-text-property start 'invisible))
+	  (if (and (not (get-text-property start 'invisible))
+		   (not (get-text-property start 'display)))
 	      (put-text-property start (point) 'invisible t)))))
     (set-buffer-modified-p nil)))
 

[-- Attachment #3: Type: text/plain, Size: 20 bytes --]



-- 
David Kastrup

Re: Fix info display of images

[Juri Linkov]

> the info images in the Lilypond documentation have a rather messed-up
> display.  That's more or less due to Emacs putting "invisible"
> properties over image code indiscriminately.
>
> I find that the following does help.

If you provided a test case, I would be able to confirm whether it works
or not.  But now it seems strange to me that you fix this in
`Info-hide-cookies-node' instead of `Info-display-images-node'.

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

Re: Fix info display of images

[David Kastrup]

Juri Linkov <juri@jurta.org> writes:

>> the info images in the Lilypond documentation have a rather messed-up
>> display.  That's more or less due to Emacs putting "invisible"
>> properties over image code indiscriminately.
>>
>> I find that the following does help.
>
> If you provided a test case, I would be able to confirm whether it
> works or not.

I can't easily provide a useful test case since that requires the
corresponding image files as well, and those are in one rather large
directory.

*I* can confirm that the patch works as intended.

> But now it seems strange to me that you fix this in
> `Info-hide-cookies-node' instead of `Info-display-images-node'.

The problem is that `Info-hide-cookies-node' rehides the images.  The
combination of invisible and display properties wreaks havoc on the
resulting display and cursor movement.  This gets even worse because the
images are also displayed using the "rear-nonsticky (display)" property,
which implies that all other properties (including invisible) become
rear-sticky.

So Info-hide-cookies-node should simply refrain from tampering with
cookies that have already been handled.

The extended test in my patch does that.

-- 
David Kastrup

Re: Fix info display of images

[David Kastrup]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

David Kastrup <dak@gnu.org> writes:

> Juri Linkov <juri@jurta.org> writes:
>
>>> the info images in the Lilypond documentation have a rather messed-up
>>> display.  That's more or less due to Emacs putting "invisible"
>>> properties over image code indiscriminately.
>>>
>>> I find that the following does help.
>>
>> If you provided a test case, I would be able to confirm whether it
>> works or not.
>
> I can't easily provide a useful test case since that requires the
> corresponding image files as well, and those are in one rather large
> directory.
>
> *I* can confirm that the patch works as intended.
>
>> But now it seems strange to me that you fix this in
>> `Info-hide-cookies-node' instead of `Info-display-images-node'.
>
> The problem is that `Info-hide-cookies-node' rehides the images.  The
> combination of invisible and display properties wreaks havoc on the
> resulting display and cursor movement.  This gets even worse because the
> images are also displayed using the "rear-nonsticky (display)" property,
> which implies that all other properties (including invisible) become
> rear-sticky.
>
> So Info-hide-cookies-node should simply refrain from tampering with
> cookies that have already been handled.
>
> The extended test in my patch does that.

Since there does not appear to be interest to apply this patch, let me
illustrate the difference using two screen shots, the first without the
patch, the second with the patch.

I think that the improvement is rather obvious.


[-- Attachment #2: Screenshot-a.png --]
[-- Type: image/png, Size: 15491 bytes --]

[-- Attachment #3: Screenshot-after.png --]
[-- Type: image/png, Size: 12532 bytes --]

[-- Attachment #4: Type: text/plain, Size: 20 bytes --]



-- 
David Kastrup

Re: Fix info display of images

[Juri Linkov]

> Since there does not appear to be interest to apply this patch, let me
> illustrate the difference using two screen shots, the first without the
> patch, the second with the patch.

Thanks, it's clear now what is the reason of this bug.

`Info-hide-cookies-node' hides a cookie, including one optional
*trailing newline*.

The image cookies in Lilypond's Cheat sheet end with trailing newlines,
so when `Info-hide-cookies-node' hides the newline, images are displayed
in one long line.

It seems hiding the newline was necessary only for index cookies,
that after hiding would leave an empty line.

OTOH, the docstring of `Info-hide-cookies-node' says:

  Hide unrecognized cookies in current node.

So your patch uses `(not (get-text-property start 'display))' to tell
`Info-hide-cookies-node' that this cookie was already recognized and
processed by `Info-display-images-node'.

> I think that the improvement is rather obvious.

This is not just improvement, but a plain bug fix.  I suggest to install it.

Since IIUC currently CVS is read-only, you could create a bug report in
the bug database to guarantee your bug fix is not lost and fixed in the
next release.

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

Re: Fix info display of images

[Chong Yidong]

Juri Linkov <juri@jurta.org> writes:

> This is not just improvement, but a plain bug fix.  I suggest to
> install it.
>
> Since IIUC currently CVS is read-only, you could create a bug report
> in the bug database to guarantee your bug fix is not lost and fixed in
> the next release.

It is not read-only yet, I think.  Please go ahead and install the
patch.

Ediff-patch-file with wrong default buffer (was: Fix info display of images)

[Juri Linkov]

>> This is not just improvement, but a plain bug fix.  I suggest to
>> install it.
>>
>> Since IIUC currently CVS is read-only, you could create a bug report
>> in the bug database to guarantee your bug fix is not lost and fixed in
>> the next release.
>
> It is not read-only yet, I think.  Please go ahead and install the
> patch.

Installed.

BTW, there is some regression in Ediff.

Running `ediff-patch-file' in a buffer with the patch used to propose
the name of the current buffer by default, but now it proposes some
irrelevant buffer name.

This bug can be reproduced by opening the initial David's message with the
patch in this thread and typing `M-x ediff-patch-file'.  To the question
"Is the patch already in a buffer" type `y'.  After that in the prompt
"Buffer that holds the patch" it provides some other buffer by default.
It used to provide the current buffer by default.

Maybe the reason is that Ediff recognizes buffers with patches only
in the CVS format?  In this case Ediff should be fixed to support
other formats since we are switching from CVS now.

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

Re: Ediff-patch-file with wrong default buffer (was: Fix info display of images)

[Michael Kifer]

On Mon, 14 Dec 2009 07:14:12 +0200
Juri Linkov <juri@jurta.org> wrote:

> >> This is not just improvement, but a plain bug fix.  I suggest to
> >> install it.
> >>
> >> Since IIUC currently CVS is read-only, you could create a bug report
> >> in the bug database to guarantee your bug fix is not lost and fixed in
> >> the next release.
> >
> > It is not read-only yet, I think.  Please go ahead and install the
> > patch.
> 
> Installed.
> 
> BTW, there is some regression in Ediff.
> 
> Running `ediff-patch-file' in a buffer with the patch used to propose
> the name of the current buffer by default, but now it proposes some
> irrelevant buffer name.
> 
> This bug can be reproduced by opening the initial David's message with the
> patch in this thread and typing `M-x ediff-patch-file'.  To the question
> "Is the patch already in a buffer" type `y'.  After that in the prompt
> "Buffer that holds the patch" it provides some other buffer by default.
> It used to provide the current buffer by default.
> 
> Maybe the reason is that Ediff recognizes buffers with patches only
> in the CVS format?  In this case Ediff should be fixed to support
> other formats since we are switching from CVS now.


I haven't looked at the ediff patch, which you are referring to, but, FYI,
Ediff only looks at the current buffer, the next, and the previous, window, and
the other-buffer. It does not attempt to search for the best possible buffer.
In those buffers that it looks at, it tries to see if it has a diff or a context
diff. This is controlled by ediff-context-diff-label-regexp.

So, either the patch that you are referring to broke the above behavior or the
current buffer does not match ediff-context-diff-label-regexp.

Re: Ediff-patch-file with wrong default buffer

[Juri Linkov]

> So, either the patch that you are referring to broke the above behavior or
> the current buffer does not match ediff-context-diff-label-regexp.

The problem is that the current buffer with a non-CVS patch does not
match ediff-context-diff-label-regexp.  The GNU unified format diff 2-liner
requires whitespace (and a date) after the file name:

(defcustom ediff-context-diff-label-regexp
  (concat "\\(" 	; context diff 2-liner
	  "^\\*\\*\\* +\\([^ \t]+\\)[^*]+[\t ]*\n--- +\\([^ \t]+\\)"
	  "\\|" 	; GNU unified format diff 2-liner
	  "^--- +\\([^ \t]+\\)[\t ]+.*\n\\+\\+\\+ +\\([^ \t]+\\)"
Problematic part underlined   ======

but Git produces patches in the format without whitespace and a date after
the name of the first file (on the line that begins with ---), e.g.

--- a/lisp/info.el
+++ b/lisp/info.el

The following patch fixes this problem:

Index: lisp/ediff-ptch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/ediff-ptch.el,v
retrieving revision 1.47
diff -u -r1.47 ediff-ptch.el
--- lisp/ediff-ptch.el	31 Jan 2009 22:10:29 -0000	1.47
+++ lisp/ediff-ptch.el	14 Dec 2009 05:47:01 -0000
@@ -127,7 +127,7 @@
   (concat "\\(" 	; context diff 2-liner
 	  "^\\*\\*\\* +\\([^ \t]+\\)[^*]+[\t ]*\n--- +\\([^ \t]+\\)"
 	  "\\|" 	; GNU unified format diff 2-liner
-	  "^--- +\\([^ \t]+\\)[\t ]+.*\n\\+\\+\\+ +\\([^ \t]+\\)"
+	  "^--- +\\([^ \t]+\\)[\t ]*.*\n\\+\\+\\+ +\\([^ \t]+\\)"
 	  "\\)")
   "Regexp matching filename 2-liners at the start of each context diff.
 You probably don't want to change that, unless you are using an obscure patch

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

Re: Ediff-patch-file with wrong default buffer

[Michael Kifer]

Sounds good. Are you going to install it while you are at it?

On Mon, 14 Dec 2009 07:47:36 +0200
Juri Linkov <juri@jurta.org> wrote:

> > So, either the patch that you are referring to broke the above behavior or
> > the current buffer does not match ediff-context-diff-label-regexp.
> 
> The problem is that the current buffer with a non-CVS patch does not
> match ediff-context-diff-label-regexp.  The GNU unified format diff 2-liner
> requires whitespace (and a date) after the file name:
> 
> (defcustom ediff-context-diff-label-regexp
>   (concat "\\(" 	; context diff 2-liner
> 	  "^\\*\\*\\* +\\([^ \t]+\\)[^*]+[\t ]*\n--- +\\([^ \t]+\\)"
> 	  "\\|" 	; GNU unified format diff 2-liner
> 	  "^--- +\\([^ \t]+\\)[\t ]+.*\n\\+\\+\\+ +\\([^ \t]+\\)"
> Problematic part underlined   ======
> 
> but Git produces patches in the format without whitespace and a date after
> the name of the first file (on the line that begins with ---), e.g.
> 
> --- a/lisp/info.el
> +++ b/lisp/info.el
> 
> The following patch fixes this problem:
> 
> Index: lisp/ediff-ptch.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/ediff-ptch.el,v
> retrieving revision 1.47
> diff -u -r1.47 ediff-ptch.el
> --- lisp/ediff-ptch.el	31 Jan 2009 22:10:29 -0000	1.47
> +++ lisp/ediff-ptch.el	14 Dec 2009 05:47:01 -0000
> @@ -127,7 +127,7 @@
>    (concat "\\(" 	; context diff 2-liner
>  	  "^\\*\\*\\* +\\([^ \t]+\\)[^*]+[\t ]*\n--- +\\([^ \t]+\\)"
>  	  "\\|" 	; GNU unified format diff 2-liner
> -	  "^--- +\\([^ \t]+\\)[\t ]+.*\n\\+\\+\\+ +\\([^ \t]+\\)"
> +	  "^--- +\\([^ \t]+\\)[\t ]*.*\n\\+\\+\\+ +\\([^ \t]+\\)"
>  	  "\\)")
>    "Regexp matching filename 2-liners at the start of each context diff.
>  You probably don't want to change that, unless you are using an obscure patch
> 

Re: Ediff-patch-file with wrong default buffer

[Juri Linkov]

> Sounds good. Are you going to install it while you are at it?

The GNU Diff manual defines the unified format which looks like this:

     --- FROM-FILE FROM-FILE-MODIFICATION-TIME
     +++ TO-FILE TO-FILE-MODIFICATION-TIME

but `git-diff' outputs in a format *similar* to the two-line traditional
unified diff format.

However, I see no problem with this patch for ediff-ptch.el.
So I installed it.  (Maybe diff-mode has the same problem because
it requires a tab after the file name, but currently I can't find
a test case that would reproduce the same problem for diff-mode).

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

Re: Ediff-patch-file with wrong default buffer

[Andreas Schwab]

Juri Linkov <juri@jurta.org> writes:

> Index: lisp/ediff-ptch.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/ediff-ptch.el,v
> retrieving revision 1.47
> diff -u -r1.47 ediff-ptch.el
> --- lisp/ediff-ptch.el	31 Jan 2009 22:10:29 -0000	1.47
> +++ lisp/ediff-ptch.el	14 Dec 2009 05:47:01 -0000
> @@ -127,7 +127,7 @@
>    (concat "\\(" 	; context diff 2-liner
>  	  "^\\*\\*\\* +\\([^ \t]+\\)[^*]+[\t ]*\n--- +\\([^ \t]+\\)"
>  	  "\\|" 	; GNU unified format diff 2-liner
> -	  "^--- +\\([^ \t]+\\)[\t ]+.*\n\\+\\+\\+ +\\([^ \t]+\\)"
> +	  "^--- +\\([^ \t]+\\)[\t ]*.*\n\\+\\+\\+ +\\([^ \t]+\\)"

Of course, "[\t ]*.*" is equivalent to ".*".

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Ediff-patch-file with wrong default buffer

[Juri Linkov]

There is another problem with ediff-patch-file and bzr.

Applying a patch to a bzr tree using ediff-patch-file
asks a question:

  File is under version control.  Check it out? (y or n)

that makes no sense.

Maybe "Bzr" should be added to a list of backends
in `ediff-file-checked-in-p'?

=== modified file 'lisp/ediff-util.el'
--- lisp/ediff-util.el	2010-01-13 08:35:10 +0000
+++ lisp/ediff-util.el	2010-02-02 00:29:43 +0000
@@ -1127,7 +1127,7 @@ (defun ediff-file-checked-out-p (file)
 (defun ediff-file-checked-in-p (file)
   (and (featurep 'vc-hooks)
        ;; CVS files are considered not checked in
-       (not (memq (vc-backend file) '(nil CVS)))
+       (not (memq (vc-backend file) '(nil CVS Bzr)))
        (if (fboundp 'vc-state)
 	   (and
 	    (not (memq (vc-state file) '(edited needs-merge)))

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

Re: Ediff-patch-file with wrong default buffer

[Juri Linkov]

> The GNU Diff manual defines the unified format which looks like this:
>
>      --- FROM-FILE FROM-FILE-MODIFICATION-TIME
>      +++ TO-FILE TO-FILE-MODIFICATION-TIME

Yet another problem with bzr and diff formats:

For Lisp files it's useful to set vc-bzr-diff-switches to "-F ^("
or diff --diff-options '-F ^(' aka `--show-function-line'
to show a function/variable name that precedes the hunk
(like "@@ (defvar ispell-message-text-end" in the patch below).

But in this case, with (add-hook 'message-send-hook 'ispell-message)
`ispell-message' checks the spelling of the whole patch that is annoying.

`ispell-message-text-end' defines the limit of spell-checking
for unidiff/context diff formats.  But it is too strict.

This patch removes the final newline to allow more text after @@:

=== modified file 'lisp/textmodes/ispell.el'
--- lisp/textmodes/ispell.el	2010-01-14 18:59:31 +0000
+++ lisp/textmodes/ispell.el	2010-02-02 00:32:03 +0000
@@ -3459,7 +3459,7 @@ (defvar ispell-message-text-end
 	       ;; Matches context difference listing
 	       "\\(\\(^cd .*\n\\)?diff -c .*\\)?\n\\*\\*\\* .*\n--- .*\n\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*"
 	       ;; Matches unidiff difference listing
-	       "\\(diff -u .*\\)?\n--- .*\n\\+\\+\\+ .*\n@@ [-+][0-9]+,[0-9]+ [-+][0-9]+,[0-9]+ @@\n"
+	       "\\(diff -u .*\\)?\n--- .*\n\\+\\+\\+ .*\n@@ [-+][0-9]+,[0-9]+ [-+][0-9]+,[0-9]+ @@"
 	       ;; Matches reporter.el bug report
 	       "^current state:\n==============\n"
 	       ;; Matches commonly used "cut" boundaries

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

Re: Ediff-patch-file with wrong default buffer

[Stefan Monnier]

> There is another problem with ediff-patch-file and bzr.
> Applying a patch to a bzr tree using ediff-patch-file
> asks a question:

>   File is under version control.  Check it out? (y or n)

> that makes no sense.

> Maybe "Bzr" should be added to a list of backends
> in `ediff-file-checked-in-p'?

> === modified file 'lisp/ediff-util.el'
> --- lisp/ediff-util.el	2010-01-13 08:35:10 +0000
> +++ lisp/ediff-util.el	2010-02-02 00:29:43 +0000
> @@ -1127,7 +1127,7 @@ (defun ediff-file-checked-out-p (file)
>  (defun ediff-file-checked-in-p (file)
>    (and (featurep 'vc-hooks)
>         ;; CVS files are considered not checked in
> -       (not (memq (vc-backend file) '(nil CVS)))
> +       (not (memq (vc-backend file) '(nil CVS Bzr)))

Isn't that backwards?  Shouldn't (not (memq (vc-backend file) '(nil CVS)))
be replaced by (memq (vc-backend file) '(RCS SCCS))?


        Stefan

Re: Ediff-patch-file with wrong default buffer

[Juri Linkov]

>>  (defun ediff-file-checked-in-p (file)
>>    (and (featurep 'vc-hooks)
>>         ;; CVS files are considered not checked in
>> -       (not (memq (vc-backend file) '(nil CVS)))
>> +       (not (memq (vc-backend file) '(nil CVS Bzr)))
>
> Isn't that backwards?  Shouldn't (not (memq (vc-backend file) '(nil CVS)))
> be replaced by (memq (vc-backend file) '(RCS SCCS))?

Done.

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