Replace selective-display in Gnus (was: obsolete selective-display ?)

Replace selective-display in Gnus (was: obsolete selective-display ?)

[Reiner Steib]

On Sat, Aug 29 2009, Dan Nicolaescu wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>   > > From: Dan Nicolaescu <dann@ics.uci.edu>
>   > > Is it time for selective-display be declared obsolete?
>   > > AFAICT there's a single active use in the tree, in gnus.
>   > 
>   > What are the alternatives?
>
> 'invisible property on overlays

Gnus uses selective-display to hide threads in the summary buffer [1].
AFAICS, the relevant code is in `gnus-sum.el'.

In `gnus-summary-hide-thread' ...

      (prog1
	  (if (and (> (point) start)
		   (search-backward "\n" start t))
	      (progn
		(subst-char-in-region start (point) ?\n ?\^M)
		(gnus-summary-goto-subject article))
	    (goto-char start)
	    nil))

... and `gnus-summary-show-thread' ...

    (prog1
	;; Any hidden lines here?
	(search-forward "\r" end t)
      (subst-char-in-region beg end ?\^M ?\n t)
      (goto-char orig)
      (gnus-summary-position-point))

Could someone propose a patch how to use invisible overlays here?  It
should be compatible with Emacs 21 (and up) and XEmacs 21.4 (and up)
if possible.

There's also `gnus-summary-mode' where probably these lines should be
removed:

  (setq selective-display t)
  (setq selective-display-ellipses t)	;Display `...'

Bye, Reiner.

[1] Examples:

    show threads (`T S', `gnus-summary-show-all-threads'):

      3.6k Dan Nicolaescu       ┌─▶  obsolete selective-display ?
      3.8k Eli Zaretskii        ╰┬─►  
      4.2k Dan Nicolaescu        ╰┬─►  
       11k Ted Zlatanov         ┌─▶  Re: Emacs 23.1 flushes stdin on startup
      4.0k Stefan Monnier       ╰┬─►  
       10k Ted Zlatanov          ╰┬─►  
      5.5k Tom Tromey           ┌─▶  advice needed for multi-threading patch
      6.0k Giuseppe Scrivano    ╰┬─►  

    hide threads (`T H', `gnus-summary-hide-all-threads'):

      3.6k Dan Nicolaescu       ┌─▶  obsolete selective-display ?...
       11k Ted Zlatanov         ┌─▶  Re: Emacs 23.1 flushes stdin on startup...
      5.5k Tom Tromey           ┌─▶  advice needed for multi-threading patch...
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Replace selective-display in Gnus

[Stefan Monnier]

> Could someone propose a patch how to use invisible overlays here?  It
> should be compatible with Emacs 21 (and up) and XEmacs 21.4 (and up)
> if possible.

I thought I had a few years back, but Google can't find it, so maybe
I just dreamt it.  Here is the gnus-sum.el code I'm using nowadays.
IIRC it's not 100% correct (the behavior is not quite the same as the
current one).  This includes unrelated changes, and uses remove-overlays
which was new in Emacs-22.


        Stefan



Using submit branch file:///home/monnier/tmp/bzr/trunk/
=== modified file 'lisp/gnus/gnus-sum.el'
--- lisp/gnus/gnus-sum.el	2009-08-29 09:03:23 +0000
+++ lisp/gnus/gnus-sum.el	2009-08-29 14:14:56 +0000
@@ -3069,8 +3069,7 @@
   (setq buffer-read-only t		;Disable modification
 	show-trailing-whitespace nil)
   (setq truncate-lines t)
-  (setq selective-display t)
-  (setq selective-display-ellipses t)	;Display `...'
+  (add-to-invisibility-spec '(gnus-sum . t))
   (gnus-summary-set-display-table)
   (gnus-set-default-directory)
   (make-local-variable 'gnus-summary-line-format)
@@ -3282,7 +3281,10 @@
   '(progn
      (gnus-summary-skip-intangible)
      (or (get-text-property (point) 'gnus-number)
-	 (gnus-summary-last-subject))))
+	 (gnus-summary-last-subject)
+         ;; FIXME: WTF?
+         ;; Triggered once from editing a text saved in the drafts folder.
+	 (debug))))
 
 (defmacro gnus-summary-article-header (&optional number)
   "Return the header of article NUMBER."
@@ -4403,9 +4405,7 @@
 
 (defsubst gnus-remove-odd-characters (string)
   "Translate STRING into something that doesn't contain weird characters."
-  (mm-subst-char-in-string
-   ?\r ?\-
-   (mm-subst-char-in-string ?\n ?\- string t) t))
+  (replace-regexp-in-string "[\r\n]" "-" string))
 
 ;; This function has to be called with point after the article number
 ;; on the beginning of the line.
@@ -6101,8 +6101,7 @@
   "Look through all the headers and mark the Xrefs as read."
   (let ((virtual (gnus-virtual-group-p from-newsgroup))
 	name info xref-hashtb idlist method nth4)
-    (save-excursion
-      (set-buffer gnus-group-buffer)
+    (with-current-buffer gnus-group-buffer
       (when (setq xref-hashtb
 		  (gnus-create-xref-hashtb from-newsgroup headers unreads))
 	(mapatoms
@@ -7530,6 +7529,8 @@
     (gnus-article-setup-buffer))
   (gnus-set-global-variables)
   (with-current-buffer gnus-article-buffer
+    ;; It seems I added this at some point, not sure why.  --Stef
+    ;; (let ((inhibit-read-only t)) (erase-buffer))
     (setq gnus-article-charset gnus-newsgroup-charset)
     (setq gnus-article-ignored-charsets gnus-newsgroup-ignored-charsets)
     (mm-enable-multibyte))
@@ -9184,6 +9185,9 @@
 	(func `(lambda (h) (,(intern (concat "mail-header-" header)) h)))
 	(case-fold-search t))
     (dolist (header gnus-newsgroup-headers)
+      ;; FIXME: when called from gnus-summary-limit-include-thread via
+      ;; gnus-summary-limit-include-matching-articles, `regexp' is a decoded
+      ;; string whereas the header isn't decoded.
       (when (string-match regexp (funcall func header))
 	(push (mail-header-number header) articles)))
     (nreverse articles)))
@@ -9845,7 +9849,7 @@
 			      to-newsgroup
 			      select-method))
 
-	;;;!!!Why is this necessary?
+        ;;!!!Why is this necessary?
 	(set-buffer gnus-summary-buffer)
 
 	(gnus-summary-goto-subject article)
@@ -10009,6 +10013,8 @@
 (defun gnus-summary-expire-articles (&optional now)
   "Expire all articles that are marked as expirable in the current group."
   (interactive)
+  ;; (unless (gnus-check-server (gnus-find-method-for-group gnus-newsgroup-name))
+  ;;   (error "Couldn't open server"))
   (when (and (not gnus-group-is-exiting-without-update-p)
 	     (gnus-check-backend-function
 	      'request-expire-articles gnus-newsgroup-name))
@@ -10287,8 +10293,8 @@
 	;; Prettify the article buffer again.
 	(unless no-highlight
 	  (with-current-buffer gnus-article-buffer
-	    ;;;!!! Fix this -- article should be rehighlighted.
-	    ;;;(gnus-run-hooks 'gnus-article-display-hook)
+	    ;;!!! Fix this -- article should be rehighlighted.
+            ;;(gnus-run-hooks 'gnus-article-display-hook)
 	    (set-buffer gnus-original-article-buffer)
 	    (gnus-request-article
 	     (cdr gnus-article-current)
@@ -10450,8 +10456,8 @@
       (set var (cons article (symbol-value var)))
       (if (memq type '(processable cached replied forwarded recent saved))
 	  (gnus-summary-update-secondary-mark article)
-	;;; !!! This is bogus.  We should find out what primary
-	;;; !!! mark we want to set.
+	;; !!! This is bogus.  We should find out what primary
+	;; !!! mark we want to set.
 	(gnus-summary-update-mark gnus-del-mark 'unread)))))
 
 (defun gnus-summary-mark-as-expirable (n)
@@ -11281,26 +11287,25 @@
 (defun gnus-summary-show-all-threads ()
   "Show all threads."
   (interactive)
-  (save-excursion
-    (let ((buffer-read-only nil))
-      (subst-char-in-region (point-min) (point-max) ?\^M ?\n t)))
+  (remove-overlays (point-min) (point-max) 'invisible 'gnus-sum)
   (gnus-summary-position-point))
 
 (defun gnus-summary-show-thread ()
   "Show thread subtrees.
 Returns nil if no thread was there to be shown."
   (interactive)
-  (let ((buffer-read-only nil)
-	(orig (point))
+  (let* ((orig (point))
 	(end (point-at-eol))
 	;; Leave point at bol
-	(beg (progn (beginning-of-line) (point))))
-    (prog1
-	;; Any hidden lines here?
-	(search-forward "\r" end t)
-      (subst-char-in-region beg end ?\^M ?\n t)
+         (beg (progn (beginning-of-line) (if (bobp) (point) (1- (point)))))
+         (eoi (when (eq (get-char-property end 'invisible) 'gnus-sum)
+                (or (next-single-char-property-change end 'invisible)
+                    (point-max)))))
+    (when eoi
+      (remove-overlays beg eoi 'invisible 'gnus-sum)
       (goto-char orig)
-      (gnus-summary-position-point))))
+      (gnus-summary-position-point)
+      eoi)))
 
 (defun gnus-summary-maybe-hide-threads ()
   "If requested, hide the threads that should be hidden."
@@ -11349,22 +11354,26 @@
 will not be hidden.
 Returns nil if no threads were there to be hidden."
   (interactive)
-  (let ((buffer-read-only nil)
-	(start (point))
+  (let ((start (point))
+        (starteol (line-end-position))
 	(article (gnus-summary-article-number)))
     (goto-char start)
     ;; Go forward until either the buffer ends or the subthread ends.
     (when (and (not (eobp))
 	       (or (zerop (gnus-summary-next-thread 1 t))
 		   (goto-char (point-max))))
-      (prog1
 	  (if (and (> (point) start)
+               ;; FIXME: this should actually search for a non-invisible \n.
 		   (search-backward "\n" start t))
 	      (progn
-		(subst-char-in-region start (point) ?\n ?\^M)
+            (when (> (point) starteol)
+              (remove-overlays starteol (point) 'invisible 'gnus-sum)
+              (let ((ol (make-overlay starteol (point) nil t nil)))
+                (overlay-put ol 'invisible 'gnus-sum)
+                (overlay-put ol 'evaporate t)))
 		(gnus-summary-goto-subject article))
 	    (goto-char start)
-	    nil)))))
+        nil))))
 
 (defun gnus-summary-go-to-next-thread (&optional previous)
   "Go to the same level (or less) next thread.
@@ -11854,8 +11863,12 @@
   (and (boundp group)
        (symbol-name group)
        (symbol-value group)
+       ;; It seems that we may get spurious old groups which do not
+       ;; correspond to any server any more and for which the returned method
+       ;; is invalid (like (nil "")) and causes an error in gnus-get-function.
+       (ignore-errors
        (gnus-get-function (gnus-find-method-for-group
-			   (symbol-name group)) 'request-accept-article t)))
+                             (symbol-name group)) 'request-accept-article t))))
 
 (defun gnus-read-move-group-name (prompt default articles prefix)
   "Read a group name."

Re: Replace selective-display in Gnus

[Reiner Steib]

On Sat, Aug 29 2009, Stefan Monnier wrote:

>> Could someone propose a patch how to use invisible overlays here?  It
>> should be compatible with Emacs 21 (and up) and XEmacs 21.4 (and up)
>> if possible.
>
> Here is the gnus-sum.el code I'm using nowadays.

Thanks.

> IIRC it's not 100% correct (the behavior is not quite the same as
> the current one).

What is the difference?  (I don't use this feature so I'll probably
won't notice it.)

> This includes unrelated changes, and uses remove-overlays which was
> new in Emacs-22.

As we need the old code for compatibility with older Emacsen, I think
I will install it with some (fboundp 'remove-overlays) conditionals.

Below are the relevant hunks (AFAICS) for review. Does anyone see a
problem with this?

--8<---------------cut here---------------start------------->8---
--- lisp/gnus/gnus-sum.el	2009-08-29 09:03:23 +0000
+++ lisp/gnus/gnus-sum.el	2009-08-29 14:14:56 +0000
@@ -3069,8 +3069,7 @@
   (setq buffer-read-only t		;Disable modification
 	show-trailing-whitespace nil)
   (setq truncate-lines t)
-  (setq selective-display t)
-  (setq selective-display-ellipses t)	;Display `...'
+  (add-to-invisibility-spec '(gnus-sum . t))
   (gnus-summary-set-display-table)
   (gnus-set-default-directory)
   (make-local-variable 'gnus-summary-line-format)
@@ -11281,26 +11287,25 @@
 (defun gnus-summary-show-all-threads ()
   "Show all threads."
   (interactive)
-  (save-excursion
-    (let ((buffer-read-only nil))
-      (subst-char-in-region (point-min) (point-max) ?\^M ?\n t)))
+  (remove-overlays (point-min) (point-max) 'invisible 'gnus-sum)
   (gnus-summary-position-point))
 
 (defun gnus-summary-show-thread ()
   "Show thread subtrees.
 Returns nil if no thread was there to be shown."
   (interactive)
-  (let ((buffer-read-only nil)
-	(orig (point))
+  (let* ((orig (point))
 	(end (point-at-eol))
 	;; Leave point at bol
-	(beg (progn (beginning-of-line) (point))))
-    (prog1
-	;; Any hidden lines here?
-	(search-forward "\r" end t)
-      (subst-char-in-region beg end ?\^M ?\n t)
+         (beg (progn (beginning-of-line) (if (bobp) (point) (1- (point)))))
+         (eoi (when (eq (get-char-property end 'invisible) 'gnus-sum)
+                (or (next-single-char-property-change end 'invisible)
+                    (point-max)))))
+    (when eoi
+      (remove-overlays beg eoi 'invisible 'gnus-sum)
       (goto-char orig)
-      (gnus-summary-position-point))))
+      (gnus-summary-position-point)
+      eoi)))
 
 (defun gnus-summary-maybe-hide-threads ()
   "If requested, hide the threads that should be hidden."

@@ -11349,22 +11354,26 @@
 will not be hidden.
 Returns nil if no threads were there to be hidden."
   (interactive)
-  (let ((buffer-read-only nil)
-	(start (point))
+  (let ((start (point))
+        (starteol (line-end-position))
 	(article (gnus-summary-article-number)))
     (goto-char start)
     ;; Go forward until either the buffer ends or the subthread ends.
     (when (and (not (eobp))
 	       (or (zerop (gnus-summary-next-thread 1 t))
 		   (goto-char (point-max))))
-      (prog1
 	  (if (and (> (point) start)
+               ;; FIXME: this should actually search for a non-invisible \n.
 		   (search-backward "\n" start t))
 	      (progn
-		(subst-char-in-region start (point) ?\n ?\^M)
+            (when (> (point) starteol)
+              (remove-overlays starteol (point) 'invisible 'gnus-sum)
+              (let ((ol (make-overlay starteol (point) nil t nil)))
+                (overlay-put ol 'invisible 'gnus-sum)
+                (overlay-put ol 'evaporate t)))
 		(gnus-summary-goto-subject article))
 	    (goto-char start)
-	    nil)))))
+        nil))))
 
 (defun gnus-summary-go-to-next-thread (&optional previous)
   "Go to the same level (or less) next thread.
--8<---------------cut here---------------end--------------->8---

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

Re: Replace selective-display in Gnus

[Dan Nicolaescu]

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

  > On Sat, Aug 29 2009, Stefan Monnier wrote:
  > 
  > >> Could someone propose a patch how to use invisible overlays here?  It
  > >> should be compatible with Emacs 21 (and up) and XEmacs 21.4 (and up)
  > >> if possible.
  > >
  > > Here is the gnus-sum.el code I'm using nowadays.
  > 
  > Thanks.
  > 
  > > IIRC it's not 100% correct (the behavior is not quite the same as
  > > the current one).
  > 
  > What is the difference?  (I don't use this feature so I'll probably
  > won't notice it.)
  > 
  > > This includes unrelated changes, and uses remove-overlays which was
  > > new in Emacs-22.
  > 
  > As we need the old code for compatibility with older Emacsen, I think
  > I will install it with some (fboundp 'remove-overlays) conditionals.

Instead of conditionals you could replace `remove-overlays' calls with
something like:

    (dolist (ov (overlays-in from to))
      (when (overlay-get ov 'gnus-sum)
        (delete-overlay ov)))))

which should work everywhere.

Re: Replace selective-display in Gnus

[Reiner Steib]

On Sat, Aug 29 2009, Dan Nicolaescu wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
>
>   > On Sat, Aug 29 2009, Stefan Monnier wrote:
>   > 
>   > >> Could someone propose a patch how to use invisible overlays here?  It
>   > >> should be compatible with Emacs 21 (and up) and XEmacs 21.4 (and up)
>   > >> if possible.
>   > >
>   > > Here is the gnus-sum.el code I'm using nowadays.
>   > 
>   > Thanks.
>   > 
>   > > IIRC it's not 100% correct (the behavior is not quite the same as
>   > > the current one).
>   > 
>   > What is the difference?  (I don't use this feature so I'll probably
>   > won't notice it.)

Unanswered, AFAICS?
 
>   > > This includes unrelated changes, and uses remove-overlays which was
>   > > new in Emacs-22.
>   > 
>   > As we need the old code for compatibility with older Emacsen, I think
>   > I will install it with some (fboundp 'remove-overlays) conditionals.
>
> Instead of conditionals you could replace `remove-overlays' calls with
> something like:
>
>     (dolist (ov (overlays-in from to))
>       (when (overlay-get ov 'gnus-sum)
>         (delete-overlay ov)))))
>
> which should work everywhere.

Thanks, Stefan and Dan.  I have committed this.

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

Re: Replace selective-display in Gnus

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:

[...]

>>     (dolist (ov (overlays-in from to))
>>       (when (overlay-get ov 'gnus-sum)
>>         (delete-overlay ov)))))
>>
>> which should work everywhere.

> Thanks, Stefan and Dan.  I have committed this.

`next-single-char-property-change' used in `gnus-summary-show-thread'
is not available in XEmacs 21.4 and SXEmacs.  Though it doesn't
seem to disturb those XEmacsen from running Gnus so far.  Could
it be replaced with a certain overlay function, like
`next-overlay-change' (that XEmacs provides for the compatibility)?

Regards,

Re: Replace selective-display in Gnus

[Stephen J. Turnbull]

Katsumi Yamaoka writes:

 > `next-single-char-property-change' used in `gnus-summary-show-thread'
 > is not available in XEmacs 21.4 and SXEmacs.

You could also file a feature request against fsf-compat[1],
especially in cases like this where the mainline has synched.  In
XEmacs 21.5, it's probably portable Lisp code, maybe even a macro,
anyway.


Footnotes: 
[1]  I'd rather apologize every time than change a name that is used
in a large number of 3rd party libraries.  Sorry!

Re: Replace selective-display in Gnus

[Katsumi Yamaoka]

>>>>> Katsumi Yamaoka wrote:
[...]
> `next-single-char-property-change' used in `gnus-summary-show-thread'
> is not available in XEmacs 21.4 and SXEmacs.  Though it doesn't
> seem to disturb those XEmacsen from running Gnus so far.

Not only in XEmacs 21.4 and SXEmacs but also in XEmacs 21.5 and
Emacs 21, `C-M-h' works but `C-M-s' doesn't.

> Could it be replaced with a certain overlay function, like
> `next-overlay-change' (that XEmacs provides for the compatibility)?

After all I used the pure `extent' functions of XEmacs (not
fsf-compat/overlay.el) to fix the functions that replace
the selective-display feature of Gnus.

NOTE:
XEmacs 21.4 and SXEmacs users need to have the `calendar' XEmacs
package installed, that provides the `add-to-invisibility-spec'
function in auto-autoloads.el.

Regards,

Re: Replace selective-display in Gnus

[Reiner Steib]

On Fri, Oct 16 2009, Katsumi Yamaoka wrote:

>>>>>> Katsumi Yamaoka wrote:
>> `next-single-char-property-change' used in `gnus-summary-show-thread'
>> is not available in XEmacs 21.4 and SXEmacs.  [...]
>
> Not only in XEmacs 21.4 and SXEmacs but also in XEmacs 21.5 and
> Emacs 21, `C-M-h' works but `C-M-s' doesn't.

Oh, that's bad.

>> Could it be replaced with a certain overlay function, like
>> `next-overlay-change' (that XEmacs provides for the compatibility)?
>
> After all I used the pure `extent' functions of XEmacs (not
> fsf-compat/overlay.el) to fix the functions that replace
> the selective-display feature of Gnus.
>
> NOTE:
> XEmacs 21.4 and SXEmacs users need to have the `calendar' XEmacs
> package installed, that provides the `add-to-invisibility-spec'
> function in auto-autoloads.el.

That is a strange dependency.  Why is such a (core) function defined
in the calendar package?  If we keep this dependency, please add it to
(info "(gnus)XEmacs") and add an appropriate error message at runtime.

But the whole point of these changes were to allow to declare
selective-display obsolete in the current Emacs version (Gnus is the
only use).  Adding so many compatibility hacks is ugly, IMHO.  I'd
rather keep the old selective-display code for Emacs 21 and XEmacs
(with appropriate feature tests for `remove-overlays'
`next-single-char-property-change'.  WDYT?

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

Re: Replace selective-display in Gnus

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:
> On Fri, Oct 16 2009, Katsumi Yamaoka wrote:
[...]
>> NOTE:
>> XEmacs 21.4 and SXEmacs users need to have the `calendar' XEmacs
>> package installed, that provides the `add-to-invisibility-spec'
>> function in auto-autoloads.el.

> That is a strange dependency.  Why is such a (core) function defined
> in the calendar package?

Actually that function is defined in cal-compat.el along with
the ";;;###autoload" directive.  There are also many Emacs compat
functions, `line-beginning-position', `match-string-no-properties',
`propertize', etc. defined in similar ways.  Maybe those were
introduced when importing calender from Emacs.

> If we keep this dependency, please add it to (info "(gnus)XEmacs")
> and add an appropriate error message at runtime.

Besides the calendar package, the c-support package and the semantic
package use `add-to-invisibility-spec', too; the former provides
the function definition by itself but the later doesn't.  It seems
to be beyond control of Gnus to guide users about it or tidy it up.

> But the whole point of these changes were to allow to declare
> selective-display obsolete in the current Emacs version (Gnus is the
> only use).  Adding so many compatibility hacks is ugly, IMHO.  I'd
> rather keep the old selective-display code for Emacs 21 and XEmacs
> (with appropriate feature tests for `remove-overlays'
> `next-single-char-property-change'.  WDYT?

In order to replace the selective-display feature, now the Gnus
head uses the overlay functions for Emacs and the extent functions
for XEmacs, and both work properly in all the Emacs versions that
Gnus supports.  The compat hacks I added (`gnus-overlay-get' and
`gnus-overlays-in') are not so many and they follow Gnus' way
(i.e., not using fsf-compat/overlay.el).  So, keeping the old
selective-display code for some Emacsen is not necessary, I believe.

Cf. http://article.gmane.org/gmane.emacs.gnus.commits/6239

Regards,