More about blink-cursor-mode

More about blink-cursor-mode

[Luc Teirlinck]

The following patch contains two proposed changes to frame.el.  I can
install either one or both changes.

The first change is, I believe, necessary.  The docstring  of
`blink-cursor-mode' has to tell that one should not try to set the
variable directly.

I do not know about the other change.  If setting blink-cursor-mode
buffer locally would make any sense, the code in front of the
defcustom should use default-boundp and setq-default. But
`blink-cursor-mode' never should have a buffer local value.  If we use
default-setq in front of the defcustom, then it would be consequent to
use it in the function `blink-cursor-mode' too, for consistency with
the code in startup.el.  I do not know whether any of this really
matters anyway.  It is a purely stylistic issue for this particular
variable.  Maybe we could just keep the current code.

There still is the issue with the effect of the earlier `blink-cursor'
change on Custom, which I pointed out in a separate posting.  That
problem is more general, however.

===File ~/frame.el-diff=====================================
*** frame.el	12 Feb 2005 11:14:04 -0600	1.216
--- frame.el	19 Feb 2005 19:33:14 -0600	
***************
*** 1266,1278 ****
  ;; function.  The correct evaluated standard value will be installed
  ;; in startup.el using exactly the same expression as in the defcustom.
  (defvar blink-cursor-mode)
! (unless (boundp 'blink-cursor-mode) (setq blink-cursor-mode nil))
  (defcustom blink-cursor-mode
    (not (or noninteractive
  	   emacs-quick-startup
  	   (eq system-type 'ms-dos)
  	   (not (memq window-system '(x w32)))))
!   "*Non-nil means Blinking Cursor mode is active."
    :group 'cursor
    :tag "Blinking cursor"
    :type 'boolean
--- 1266,1281 ----
  ;; function.  The correct evaluated standard value will be installed
  ;; in startup.el using exactly the same expression as in the defcustom.
  (defvar blink-cursor-mode)
! (unless (default-boundp 'blink-cursor-mode)
!   (setq-default blink-cursor-mode nil))
  (defcustom blink-cursor-mode
    (not (or noninteractive
  	   emacs-quick-startup
  	   (eq system-type 'ms-dos)
  	   (not (memq window-system '(x w32)))))
!   "*Non-nil means Blinking Cursor mode is active.
! Do not set this variable directly.  It would confuse Emacs.
! Either set it through Custom or use the command `blink-cursor-mode'."
    :group 'cursor
    :tag "Blinking cursor"
    :type 'boolean
***************
*** 1302,1318 ****
      (if blink-cursor-timer
  	(cancel-timer blink-cursor-timer))
      (setq blink-cursor-idle-timer nil
! 	  blink-cursor-timer nil
! 	  blink-cursor-mode nil)
      (if on-p
  	(progn
  	  ;; Hide the cursor.
! 	  ;(internal-show-cursor nil nil)
  	  (setq blink-cursor-idle-timer
  		(run-with-idle-timer blink-cursor-delay
  				     blink-cursor-delay
  				     'blink-cursor-start))
! 	  (setq blink-cursor-mode t))
        (internal-show-cursor nil t))))
  
  (defun blink-cursor-start ()
--- 1305,1321 ----
      (if blink-cursor-timer
  	(cancel-timer blink-cursor-timer))
      (setq blink-cursor-idle-timer nil
! 	  blink-cursor-timer nil)
!     (setq-default blink-cursor-mode nil)
      (if on-p
  	(progn
  	  ;; Hide the cursor.
! 	  ;; (internal-show-cursor nil nil)
  	  (setq blink-cursor-idle-timer
  		(run-with-idle-timer blink-cursor-delay
  				     blink-cursor-delay
  				     'blink-cursor-start))
! 	  (setq-default blink-cursor-mode t))
        (internal-show-cursor nil t))))
  
  (defun blink-cursor-start ()
============================================================

Re: More about blink-cursor-mode

[Stefan]

> The first change is, I believe, necessary.  The docstring  of
> `blink-cursor-mode' has to tell that one should not try to set the
> variable directly.

The change below which uses define-minor-mode fixes this part as
a side effect.

> I do not know about the other change.  If setting blink-cursor-mode
> buffer locally would make any sense, the code in front of the
> defcustom should use default-boundp and setq-default.

This code is run before any .emacs can ever affect the result, so it
doesn't matter.  In any case all this round-about hack seems completely
unnecessary (based on my tests) and if really the standard expression
signals an error if it is evaluated too early, then we can just wrap it with
a (condition-case nil ... (error nil)) or some such.  Seems like a much
better option than this nasty unexplained stuff we have now.

Any objection?


        Stefan


--- orig/lisp/frame.el
+++ mod/lisp/frame.el
@@ -1256,35 +1256,11 @@
 This timer calls `blink-cursor-timer-function' every
 `blink-cursor-interval' seconds.")
 
-;; The strange sequence below is meant to set both the right temporary
-;; value and the right "standard expression" , according to Custom,
-;; for blink-cursor-mode.  We do not know the standard _evaluated_
-;; value yet, because the standard expression uses values that are not
-;; yet set.  Evaluating it now would yield an error, but we make sure
-;; that it is not evaluated, by ensuring that blink-cursor-mode is set
-;; before the defcustom is evaluated and by using the right :initialize
-;; function.  The correct evaluated standard value will be installed
-;; in startup.el using exactly the same expression as in the defcustom.
-(defvar blink-cursor-mode)
-(unless (boundp 'blink-cursor-mode) (setq blink-cursor-mode nil))
-(defcustom blink-cursor-mode
-  (not (or noninteractive
-	   emacs-quick-startup
-	   (eq system-type 'ms-dos)
-	   (not (memq window-system '(x w32)))))
-  "*Non-nil means Blinking Cursor mode is active."
-  :group 'cursor
-  :tag "Blinking cursor"
-  :type 'boolean
-  :initialize 'custom-initialize-set
-  :set #'(lambda (symbol value)
-	   (set-default symbol value)
-	   (blink-cursor-mode (or value 0))))
-
-(defvaralias 'blink-cursor 'blink-cursor-mode)
-(make-obsolete-variable 'blink-cursor 'blink-cursor-mode "22.1")
-
-(defun blink-cursor-mode (arg)
+;; We do not know the standard _evaluated_ value yet, because the standard
+;; expression uses values that are not yet set.  The correct evaluated
+;; standard value will be installed in startup.el using exactly the same
+;; expression as in the defcustom.
+(define-minor-mode blink-cursor-mode
   "Toggle blinking cursor mode.
 With a numeric argument, turn blinking cursor mode on iff ARG is positive.
 When blinking cursor mode is enabled, the cursor of the selected
@@ -1293,27 +1293,29 @@
 Note that this command is effective only when Emacs
 displays through a window system, because then Emacs does its own
 cursor display.  On a text-only terminal, this is not implemented."
-  (interactive "P")
-  (let ((on-p (if (null arg)
-		  (not blink-cursor-mode)
-		(> (prefix-numeric-value arg) 0))))
+  :init-value (not (or noninteractive
+		       emacs-quick-startup
+		       (eq system-type 'ms-dos)
+		       (not (memq window-system '(x w32)))))
+  :global t
     (if blink-cursor-idle-timer
 	(cancel-timer blink-cursor-idle-timer))
     (if blink-cursor-timer
 	(cancel-timer blink-cursor-timer))
     (setq blink-cursor-idle-timer nil
-	  blink-cursor-timer nil
-	  blink-cursor-mode nil)
-    (if on-p
+	blink-cursor-timer nil)
+  (if blink-cursor-mode
 	(progn
 	  ;; Hide the cursor.
-	  ;(internal-show-cursor nil nil)
+	;;(internal-show-cursor nil nil)
 	  (setq blink-cursor-idle-timer
 		(run-with-idle-timer blink-cursor-delay
 				     blink-cursor-delay
-				     'blink-cursor-start))
-	  (setq blink-cursor-mode t))
-      (internal-show-cursor nil t))))
+				   'blink-cursor-start)))
+    (internal-show-cursor nil t)))
+
+(defvaralias 'blink-cursor 'blink-cursor-mode)
+(make-obsolete-variable 'blink-cursor 'blink-cursor-mode "22.1")
 
 (defun blink-cursor-start ()
   "Timer function called from the timer `blink-cursor-idle-timer'.

Re: More about blink-cursor-mode

[Luc Teirlinck]

Stefan Monnier wrote:

   Any objection?

I have no time to look at it closely or to try it out right now.  I
will try to do so tomorrow.  Even if that expression gives no error on
your operating system (or mine), I guess that does not guarantee that
it works on all settings.

Sincerely,

Luc.

Re: More about blink-cursor-mode

[Luc Teirlinck]

Stefan Monnier wrote:

   Any objection?

Well, you already installed it, but there are some problems.  I
replied earlier that I would look at it today.

You removed:

(defvar blink-cursor-mode)
(unless (boundp 'blink-cursor-mode) (setq blink-cursor-mode nil))

which ensured that the value when evaluated before startup.el was
loaded would be nil.

Now it turns out that removing this actually works, unlike the person
who wrote the initial code, and later me, thought.  There are several
variables in the form that are not yet defined.  However, at the time
the code is executed, noninteractive appears to be always t.  Because
this is the first expression in the `or' form, the `or' evalutes to t,
without evaluating the other, not yet defined, variables in the `or'
form, thereby avoiding the error.  Because the `or' is preceded by
not, the temporary value that `blink-cursor-mode' gets is nil, as it
should be.  All of this is not necessarily less tricky than the two
lines of code above.  We must make sure that anybody trying to change
the expression knows about the importance of the noniteractive trick
and about the fact that he should also change the identical expression
in startup.el, where noninteractive may be nil (unless running in
batch mode) and the real value is established.

Also, after your change, the user options `blink-cursor-mode' and
`blink-cursor-mode-hook' wind up in a special group, blink-cursor,
which has no parent group and which browsing Custom will hence never
find.  They should be in the cursor group, together with other options
related to cursor blinking.

   In any case all this round-about hack seems completely unnecessary

It will be necessary for other options that still need to be given a
correct Custom standard expression and for which the noninteractive
coincidence will not happen.

   and if really the standard expression signals an error if it is
   evaluated too early, then we can just wrap it with a
   (condition-case nil ... (error nil)) or some such.

One _definitely_ does not want to rap a Custom standard expression in
a condition-case.

   Seems like a much better option than this nasty unexplained stuff
   we have now.

The comment tried to explain it.

The comment left after your change does not make a lot of sense any
more.

Maybe it is better to put the:

(defvar blink-cursor-mode)
(unless (default-boundp 'blink-cursor-mode)
   (setq-default blink-cursor-mode nil))

with something like the original comment back in, to protect against
future code changes, that might undo the current lucky coincidence.

If not, I believe that the comment in the patch below should be added.
The :group _definitely_ should be changed back to 'cursor, as the
patch below does.

===File ~/frame.el-newdiff==================================
*** frame.el	21 Feb 2005 14:18:13 -0600	1.217
--- frame.el	21 Feb 2005 18:35:09 -0600	
***************
*** 1256,1265 ****
  This timer calls `blink-cursor-timer-function' every
  `blink-cursor-interval' seconds.")
  
! ;; We do not know the standard _evaluated_ value yet, because the standard
! ;; expression uses values that are not yet set.  The correct evaluated
! ;; standard value will be installed in startup.el using exactly the same
! ;; expression as in the defcustom.
  (define-minor-mode blink-cursor-mode
    "Toggle blinking cursor mode.
  With a numeric argument, turn blinking cursor mode on iff ARG is positive.
--- 1256,1271 ----
  This timer calls `blink-cursor-timer-function' every
  `blink-cursor-interval' seconds.")
  
! ;; The :init-value given below is evaluated at startup, before
! ;; startup.el is loaded.  At that time, the variables it uses do not
! ;; yet have their correct values.  Because noninteractive is t at the
! ;; time, the :init-value evaluates to nil.  The code in the
! ;; :init-value will be evaluated again in startup.el, at which time
! ;; all values will be correct.  If you change the :init-value below,
! ;; you should also change the expression used in startup.el.  The two
! ;; expressions need to be identical or Custom will be very confused.
! ;; You should also make sure that the new expression still evaluates
! ;; to nil before startup.el is loaded.
  (define-minor-mode blink-cursor-mode
    "Toggle blinking cursor mode.
  With a numeric argument, turn blinking cursor mode on iff ARG is positive.
***************
*** 1273,1278 ****
--- 1279,1285 ----
  		       emacs-quick-startup
  		       (eq system-type 'ms-dos)
  		       (not (memq window-system '(x w32)))))
+   :group 'cursor
    :global t
    (if blink-cursor-idle-timer (cancel-timer blink-cursor-idle-timer))
    (if blink-cursor-timer (cancel-timer blink-cursor-timer))
============================================================
 LocalWords:  noninteractive evalutes noniteractive newdiff

Re: More about blink-cursor-mode

[Stefan Monnier]

> which ensured that the value when evaluated before startup.el was
> loaded would be nil.

Any evidence that it matters?
Really, it shouldn't matter, so if you know of a place where it matters,
maybe we should fix it.  The initial value is simply forcibly overridden in
startup.el, without paying *any* attention to the old value.  At least
that's how it should work.

>    In any case all this round-about hack seems completely unnecessary
> It will be necessary for other options that still need to be given a
> correct Custom standard expression and for which the noninteractive
> coincidence will not happen.

No, the same "blind override" should apply there as well.

>    and if really the standard expression signals an error if it is
>    evaluated too early, then we can just wrap it with a
>    (condition-case nil ... (error nil)) or some such.

> One _definitely_ does not want to wrap a Custom standard expression in
> a condition-case.

Why not, exactly?

>    Seems like a much better option than this nasty unexplained stuff
>    we have now.
> The comment tried to explain it.

It did not explain why this particular workaround was used.
E.g. why (defvar cursor-blink-mode)?  It seems 100% useless since it's only
a byte-compiler directive and only has the effect of declaring the variable
2 lines earlier, but since the variable is not *used* in those two lines, it
shouldn't matter.  Why not wrap the expression in condition-case instead?
Or place `boundp' checks instead?  That's what I mean by "lack of
explanation".

> The comment left after your change does not make a lot of sense any
> more.

> Maybe it is better to put the:

> (defvar blink-cursor-mode)
> (unless (default-boundp 'blink-cursor-mode)
>    (setq-default blink-cursor-mode nil))

> with something like the original comment back in, to protect against
> future code changes, that might undo the current lucky coincidence.

Please *concretely* justify the need for such ugly code, before
considering re-installing it.

> If not, I believe that the comment in the patch below should be added.

If you want to add such a comment, it should start by explaining why it's
important that the initial value be nil rather than t or `slipper'.
Otherwise it makes no sense.

> The :group _definitely_ should be changed back to 'cursor, as the
> patch below does.

Fair enough ;-)


        Stefan

Re: More about blink-cursor-mode

[Luc Teirlinck]

Stefan Monnier wrote:

   E.g. why (defvar cursor-blink-mode)?  It seems 100% useless since it's only
   a byte-compiler directive and only has the effect of declaring the variable
   2 lines earlier, but since the variable is not *used* in those two lines, it
   shouldn't matter.  Why not wrap the expression in condition-case instead?
   Or place `boundp' checks instead?  That's what I mean by "lack of
   explanation".

Well the variable _is_ used:

(unless (default-boundp 'blink-cursor-mode)
    (setq-default blink-cursor-mode nil))

I do not _exactly_ know _what_ prevents a compiler warning.  (Does
unless work just as well as if, does default-boundp work just as well
as boundp?)  I guess that one way or the other the line above can be
rewritten in such a way that it avoids the need for the defvar.

But the real question is whether the code is easier to understand for
somebody who reads it, without prior knowledge of the problems
involved, with making sure that the :init-form is never evaluated by
setting the default-value before the defcustom if not yet set (which
indeed can be done with just one line of code) or with actually
evaluating the :init-form, wrapped into a condition-case if necessary.

In the current situation, I believe that we need a comment making
clear that the temporary evaluated value should not be relied upon
even during the time it is in effect (to be robust against code
changes), that the `noninteractive' better be kept the first form in the
`or' to prevent the need for the condition-case, and that any change
in the :init-form should be mimicked in startup.el.

The comment in startup.el should also be changed, since now we _do_
execute the form in the defcustom, it just gives an incorrect value.

I propose:

===File ~/frame.el-diff=====================================
*** frame.el	21 Feb 2005 14:18:13 -0600	1.217
--- frame.el	22 Feb 2005 20:09:14 -0600	
***************
*** 1256,1265 ****
  This timer calls `blink-cursor-timer-function' every
  `blink-cursor-interval' seconds.")
  
! ;; We do not know the standard _evaluated_ value yet, because the standard
! ;; expression uses values that are not yet set.  The correct evaluated
! ;; standard value will be installed in startup.el using exactly the same
! ;; expression as in the defcustom.
  (define-minor-mode blink-cursor-mode
    "Toggle blinking cursor mode.
  With a numeric argument, turn blinking cursor mode on iff ARG is positive.
--- 1256,1272 ----
  This timer calls `blink-cursor-timer-function' every
  `blink-cursor-interval' seconds.")
  
! ;; The :init-value below is evaluated at startup, before startup.el is
! ;; loaded.  At that time, the variables it uses do not yet have their
! ;; correct values, or are not yet defined.  Because noninteractive is
! ;; t at the time, the :init-value evaluates to nil.  However, the only
! ;; relevant fact is that it does not throw an error.  The actual
! ;; temporary value is irrelevant and should not be relied upon.  The
! ;; code in the :init-value will be evaluated again in startup.el, at
! ;; which time all values will be correct.  If you change the
! ;; :init-value below, you should also change the expression used in
! ;; startup.el.  The two expressions need to be identical or Custom
! ;; will get very confused.
  (define-minor-mode blink-cursor-mode
    "Toggle blinking cursor mode.
  With a numeric argument, turn blinking cursor mode on iff ARG is positive.
***************
*** 1273,1278 ****
--- 1280,1286 ----
  		       emacs-quick-startup
  		       (eq system-type 'ms-dos)
  		       (not (memq window-system '(x w32)))))
+   :group 'cursor
    :global t
    (if blink-cursor-idle-timer (cancel-timer blink-cursor-idle-timer))
    (if blink-cursor-timer (cancel-timer blink-cursor-timer))
============================================================

===File ~/startup.el-diff===================================
*** startup.el	10 Feb 2005 17:18:09 -0600	1.338
--- startup.el	22 Feb 2005 16:21:58 -0600	
***************
*** 735,744 ****
                (<= (frame-parameter nil 'tool-bar-lines) 0))
      (tool-bar-mode 1))
  
!   ;; Can't do this init in defcustom because the relevant variables
!   ;; are not set.  If you make any changes to the `or' form below,
!   ;; you should also change the corresponding expression in the
!   ;; defcustom in frame.el, or Custom will be badly confused.
    (unless (or noninteractive
  	      emacs-quick-startup
                (eq system-type 'ms-dos)
--- 735,745 ----
                (<= (frame-parameter nil 'tool-bar-lines) 0))
      (tool-bar-mode 1))
  
!   ;; Can't do this init correctly in defcustom because the relevant
!   ;; variables have wrong values or are not yet set.  If you make any
!   ;; changes to the `or' form below, you should also change the
!   ;; corresponding expression in the defcustom in frame.el, or Custom
!   ;; will be badly confused.
    (unless (or noninteractive
  	      emacs-quick-startup
                (eq system-type 'ms-dos)
============================================================

Re: More about blink-cursor-mode

[Stefan Monnier]

> In the current situation, I believe that we need a comment making
> clear that the temporary evaluated value should not be relied upon
> even during the time it is in effect (to be robust against code
> changes),

There are many variables like that.  It's not like it's a big deal.

> that the `noninteractive' better be kept the first form in the
> `or' to prevent the need for the condition-case, and that any change
> in the :init-form should be mimicked in startup.el.

As mentioned elsewhere, instead of mimicking things we should provide
a customize-reset-value which tells custom to re-evaluate the expression
that defines the value.


        Stefan

Re: More about blink-cursor-mode

[Luc Teirlinck]

>From my previous reply:

  One _definitely_ does not want to rap a Custom standard expression in
  a condition-case.

Well, unless the `condition-case' still fully makes sense after
startup, when the user chooses "Erase Customization" or "Show initial
Lisp expression" in a Custom buffer.

Sincerely,

Luc.

Re: More about blink-cursor-mode

[Stefan Monnier]

>   One _definitely_ does not want to rap a Custom standard expression in
>   a condition-case.

> Well, unless the `condition-case' still fully makes sense after
> startup, when the user chooses "Erase Customization" or "Show initial
> Lisp expression" in a Custom buffer.

Of course.  That's a given.  And since the expression has worked well
unwrapped for a long time, wrapping it with condition-case would have *no
(nasty) effect* except maybe at startup (i.e. when we might want it).


        Stefan

Re: More about blink-cursor-mode

[Richard Stallman]

Your code is clean; if it works well, it is fine with me.

Re: More about blink-cursor-mode

[Richard Stallman]

These changes seem correct to me.