* More about blink-cursor-mode
@ 2005-02-20 1:55 Luc Teirlinck
2005-02-20 21:38 ` Stefan
2005-02-21 10:21 ` Richard Stallman
0 siblings, 2 replies; 11+ messages in thread
From: Luc Teirlinck @ 2005-02-20 1:55 UTC (permalink / raw)
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 ()
============================================================
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: More about blink-cursor-mode 2005-02-20 1:55 More about blink-cursor-mode Luc Teirlinck @ 2005-02-20 21:38 ` Stefan 2005-02-21 3:27 ` Luc Teirlinck ` (3 more replies) 2005-02-21 10:21 ` Richard Stallman 1 sibling, 4 replies; 11+ messages in thread From: Stefan @ 2005-02-20 21:38 UTC (permalink / raw) Cc: emacs-devel > 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'. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-20 21:38 ` Stefan @ 2005-02-21 3:27 ` Luc Teirlinck 2005-02-22 1:24 ` Luc Teirlinck ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Luc Teirlinck @ 2005-02-21 3:27 UTC (permalink / raw) Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-20 21:38 ` Stefan 2005-02-21 3:27 ` Luc Teirlinck @ 2005-02-22 1:24 ` Luc Teirlinck 2005-02-22 13:52 ` Stefan Monnier 2005-02-22 1:37 ` Luc Teirlinck 2005-02-22 8:42 ` Richard Stallman 3 siblings, 1 reply; 11+ messages in thread From: Luc Teirlinck @ 2005-02-22 1:24 UTC (permalink / raw) Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-22 1:24 ` Luc Teirlinck @ 2005-02-22 13:52 ` Stefan Monnier 2005-02-23 2:53 ` Luc Teirlinck 0 siblings, 1 reply; 11+ messages in thread From: Stefan Monnier @ 2005-02-22 13:52 UTC (permalink / raw) Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-22 13:52 ` Stefan Monnier @ 2005-02-23 2:53 ` Luc Teirlinck 2005-02-23 4:23 ` Stefan Monnier 0 siblings, 1 reply; 11+ messages in thread From: Luc Teirlinck @ 2005-02-23 2:53 UTC (permalink / raw) Cc: emacs-devel 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) ============================================================ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-23 2:53 ` Luc Teirlinck @ 2005-02-23 4:23 ` Stefan Monnier 0 siblings, 0 replies; 11+ messages in thread From: Stefan Monnier @ 2005-02-23 4:23 UTC (permalink / raw) Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-20 21:38 ` Stefan 2005-02-21 3:27 ` Luc Teirlinck 2005-02-22 1:24 ` Luc Teirlinck @ 2005-02-22 1:37 ` Luc Teirlinck 2005-02-22 13:54 ` Stefan Monnier 2005-02-22 8:42 ` Richard Stallman 3 siblings, 1 reply; 11+ messages in thread From: Luc Teirlinck @ 2005-02-22 1:37 UTC (permalink / raw) Cc: emacs-devel >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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-22 1:37 ` Luc Teirlinck @ 2005-02-22 13:54 ` Stefan Monnier 0 siblings, 0 replies; 11+ messages in thread From: Stefan Monnier @ 2005-02-22 13:54 UTC (permalink / raw) Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-20 21:38 ` Stefan ` (2 preceding siblings ...) 2005-02-22 1:37 ` Luc Teirlinck @ 2005-02-22 8:42 ` Richard Stallman 3 siblings, 0 replies; 11+ messages in thread From: Richard Stallman @ 2005-02-22 8:42 UTC (permalink / raw) Cc: teirllm, emacs-devel Your code is clean; if it works well, it is fine with me. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: More about blink-cursor-mode 2005-02-20 1:55 More about blink-cursor-mode Luc Teirlinck 2005-02-20 21:38 ` Stefan @ 2005-02-21 10:21 ` Richard Stallman 1 sibling, 0 replies; 11+ messages in thread From: Richard Stallman @ 2005-02-21 10:21 UTC (permalink / raw) Cc: emacs-devel These changes seem correct to me. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2005-02-23 4:23 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2005-02-20 1:55 More about blink-cursor-mode Luc Teirlinck 2005-02-20 21:38 ` Stefan 2005-02-21 3:27 ` Luc Teirlinck 2005-02-22 1:24 ` Luc Teirlinck 2005-02-22 13:52 ` Stefan Monnier 2005-02-23 2:53 ` Luc Teirlinck 2005-02-23 4:23 ` Stefan Monnier 2005-02-22 1:37 ` Luc Teirlinck 2005-02-22 13:54 ` Stefan Monnier 2005-02-22 8:42 ` Richard Stallman 2005-02-21 10:21 ` Richard Stallman
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).