24.2.50; doc string of `text-scale-adjust'

DoneSubmitted by Drew Adams.
Details
4 participants
  • Bastien
  • Drew Adams
  • Stefan Monnier
  • Stefan Monnier
Owner
unassigned
Severity
minor
D
D
Drew Adams wrote on 4 Sep 2012 04:54
(address . bug-gnu-emacs@gnu.org)
703AC3C70837474CADDB99BA7E9E59A1@us.oracle.com
1. Describe the parameter, INC. 2. Describe the use of a prefix argument. In general, describe this as a user command, from a user point of view,and not just as a function to be called from code.
In GNU Emacs 24.2.50.1 (i386-mingw-nt5.1.2600) of 2012-09-02 on MARVINBzr revision: 109861 eggert@cs.ucla.edu-20120902171035-7mzihil3xd6bjfiyWindowing system distributor `Microsoft Corp.', version 5.1.2600Configured using: `configure --with-gcc (4.6) --no-opt --enable-checking --cflags -ID:/devel/emacs/libs/libXpm-3.5.8/include -ID:/devel/emacs/libs/libXpm-3.5.8/src -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include -ID:/devel/emacs/libs/giflib-4.1.4-1/include -ID:/devel/emacs/libs/jpeg-6b-4/include -ID:/devel/emacs/libs/tiff-3.8.2-1/include -ID:/devel/emacs/libs/gnutls-3.0.9/include -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2'
B
B
Bastien wrote on 11 Sep 2012 15:37
(name . Drew Adams)(address . drew.adams@oracle.com)(address . 12345@debbugs.gnu.org)
87vcfkr884.fsf@altern.org
"Drew Adams" <drew.adams@oracle.com> writes:
Toggle quote (7 lines)> 1. Describe the parameter, INC.> > 2. Describe the use of a prefix argument.> > In general, describe this as a user command, from a user point of view,> and not just as a function to be called from code.
See attached patch.
I enhanced the docstring and rewrote the command so that it doesnot use a temporary keymap. When I tried `C-x C-+' I was left with the `+' and `-' keys not usable anymore.
The patch also removes the redundant keybinding `C-x C-='.
And C-x C-0 does not allow adjusting further. My feeling is thatit is not useful.
I'll apply this patch within a week if nobody is against it.
=== modified file 'lisp/face-remap.el'--- lisp/face-remap.el 2012-07-10 11:51:54 +0000+++ lisp/face-remap.el 2012-09-11 13:18:59 +0000@@ -281,22 +281,26 @@ ;;;###autoload (define-key ctl-x-map [(control ?+)] 'text-scale-adjust) ;;;###autoload (define-key ctl-x-map [(control ?-)] 'text-scale-adjust)-;;;###autoload (define-key ctl-x-map [(control ?=)] 'text-scale-adjust) ;;;###autoload (define-key ctl-x-map [(control ?0)] 'text-scale-adjust) ;;;###autoload-(defun text-scale-adjust (inc)- "Increase or decrease the height of the default face in the current buffer.+(defun text-scale-adjust (inc &optional mod)+ "Adjust the height of the default face by INC.++INC may be passed as a numeric prefix argument.++The optional argument MOD is used internally to pass the previous+modifier to the command, when used repeatedly. The actual adjustment made depends on the final component of the-key-binding used to invoke the command, with all modifiers removed:+keybinding used to invoke the command. - +, = Increase the default face height by one step+ + Increase the default face height by one step - Decrease the default face height by one step 0 Reset the default face height to the global default -Then, continue to read input events and further adjust the face-height as long as the input event read (with all modifiers removed)-is one of the above.+After the first invokation, continue to read input events and+further adjust the face height as long as the input event read+is `+' or `-'. Each step scales the height of the default face by the variable `text-scale-mode-step' (a negative number of steps decreases the@@ -309,30 +313,15 @@ a top-level keymap, `text-scale-increase' or `text-scale-decrease' may be more appropriate." (interactive "p")- (let ((first t)- (ev last-command-event)- (echo-keystrokes nil))- (let* ((base (event-basic-type ev))- (step- (pcase base- ((or ?+ ?=) inc)- (?- (- inc))- (?0 0)- (t inc))))- (text-scale-increase step)- ;; FIXME: do it after every "iteration of the loop".- (message "+,-,0 for further adjustment: ")- (set-temporary-overlay-map- (let ((map (make-sparse-keymap)))- (dolist (mods '(() (control)))- (define-key map (vector (append mods '(?-))) 'text-scale-decrease)- (define-key map (vector (append mods '(?+))) 'text-scale-increase)- ;; = is unshifted + on most keyboards.- (define-key map (vector (append mods '(?=))) 'text-scale-increase)- (define-key map (vector (append mods '(?0)))- (lambda () (interactive) (text-scale-increase 0))))- map)- t))))+ (let* ((ev (or (event-basic-type (or mod last-command-event))))+ (step (pcase ev (?+ inc) (?- (- inc)) (?0 0) (t inc)))+ c)+ (text-scale-increase step)+ (when (and (not (zerop step))+ (setq c (read-event "Hit + or - for further adjustment, RET to finish")))+ (cond ((member (event-basic-type c) '(?+ ?-))+ (text-scale-adjust (abs step) c))+ (t (message "Done")))))) ;; ----------------------------------------------------------------
-- Bastien
S
S
Stefan Monnier wrote on 11 Sep 2012 15:48
(name . Bastien)(address . bzg@altern.org)
jwvy5kghdtu.fsf-monnier+emacs@gnu.org
Toggle quote (3 lines)> I enhanced the docstring and rewrote the command so that it does> not use a temporary keymap.
Not using a temporary keymap means reverting to the old code: pleaseexplain precisely why using a temporary keymap is a problem (there aregood reasons to use it: e.g. it makes key-translation-map and friendswork correctly).

Stefan
D
D
Drew Adams wrote on 11 Sep 2012 16:24
RE: bug#12345: 24.2.50; doc string of `text-scale-adjust'
(name . 'Bastien')(address . bzg@altern.org)(address . 12345@debbugs.gnu.org)
F82080D52CE64E8D885A70A5CCCC2A60@us.oracle.com
Hi Bastien,
1. I think any changes in the behavior and bindings should be proposed onemacs-devel. Personally, I don't care much, but I'm pretty sure that at leastsome people will want to keep the `=' binding because `+' is often on a Shiftkey. Emacs-devel is also the place to pose your question about zero.
2. Please consider changing the name of the parameter to INCREMENT, so the docstring is more readable: "Adjust the height of the default face by INCREMENT."Say explicitly that INCREMENT defaults to 1.
3. Alternatively, you could say something like this:
"Adjust the height of face `default' by N text-scale steps.N is the numeric prefix agument: positive to increase height, negativeto decrease. Step size is the value of `text-scale-mode-step'."
The rest is OK.
4. Don't be surprised if Stefan doesn't go along with your change to not use thetemporary keymap. He just got through _adding_ such code here and therethroughout Emacs. (Makes no difference to me - my bug report was about the docstring.)
Thx - Drew
Toggle quote (17 lines)> > 1. Describe the parameter, INC.> > 2. Describe the use of a prefix argument.> > In general, describe this as a user command, from a user > > point of view, and not just as a function to be called from code.> > See attached patch.> > I enhanced the docstring and rewrote the command so that it does> not use a temporary keymap. When I tried `C-x C-+' I was left with > the `+' and `-' keys not usable anymore.> > The patch also removes the redundant keybinding `C-x C-='.> > And C-x C-0 does not allow adjusting further. My feeling is that> it is not useful.> > I'll apply this patch within a week if nobody is against it.
B
B
Bastien wrote on 11 Sep 2012 16:24
Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
(name . Stefan Monnier)(address . monnier@iro.umontreal.ca)(address . 12345@debbugs.gnu.org)
87ehm8r61x.fsf@altern.org
Stefan Monnier <monnier@iro.umontreal.ca> writes:
Toggle quote (8 lines)>> I enhanced the docstring and rewrote the command so that it does>> not use a temporary keymap.>> Not using a temporary keymap means reverting to the old code: please> explain precisely why using a temporary keymap is a problem (there are> good reasons to use it: e.g. it makes key-translation-map and friends> work correctly).
The `read-event' loop allows to keep the message displayed (see the FIXME).
But I hit something weird.
from emacs -q, try to edebug-defun `text-scale-adjust', then use `C-x C-+', then `c' in the debug loop, then quit.
The temporary keymap is not temporary anymore, and the `-'and `+' keys are still bounded to `text-scale-adjust'.
Surely something weird when `set-temporary-overlay-map' is called from within a debug loop?
-- Bastien
D
D
Drew Adams wrote on 11 Sep 2012 16:39
RE: bug#12345: 24.2.50; doc string of `text-scale-adjust'
(address . 12345@debbugs.gnu.org)
3B9A6A891CD0485AAC47DEBD891BE144@us.oracle.com
Toggle quote (9 lines)> from emacs -q, try to edebug-defun `text-scale-adjust', > then use `C-x C-+', then `c' in the debug loop, then quit.> > The temporary keymap is not temporary anymore, and the `-'> and `+' keys are still bounded to `text-scale-adjust'.> > Surely something weird when `set-temporary-overlay-map' is > called from within a debug loop?
Debugging is notoriously difficult when events are read. I usually have toresort to adding `message' calls or similar.
See also bug (regression) #12232, which similarly was changed recently to use`set-temporary-overlay-map' (and to use lexical binding), and which since thatchange leads to the error "Lisp nesting exceeds `max-lisp-eval-depth'".
No one has responded to that bug at all so far. I located the commit thatintroduced the regression, but debugging it further and fixing it is beyond me.
B
B
Bastien wrote on 11 Sep 2012 16:53
Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
(name . Drew Adams)(address . drew.adams@oracle.com)(address . 12345@debbugs.gnu.org)
87txv4zk4t.fsf@altern.org
Hi Drew,
"Drew Adams" <drew.adams@oracle.com> writes:
Toggle quote (5 lines)> 1. I think any changes in the behavior and bindings should be proposed on> emacs-devel. Personally, I don't care much, but I'm pretty sure that at least> some people will want to keep the `=' binding because `+' is often on a Shift> key. Emacs-devel is also the place to pose your question about zero.
Right, I will ask on emacs-devel.
Toggle quote (4 lines)> 2. Please consider changing the name of the parameter to INCREMENT, so the doc> string is more readable: "Adjust the height of the default face by INCREMENT."> Say explicitly that INCREMENT defaults to 1.
"INC" seems a rather standard and self-explanatory pet name for"INCREMENT".
Toggle quote (6 lines)> 3. Alternatively, you could say something like this:>> "Adjust the height of face `default' by N text-scale steps.> N is the numeric prefix agument: positive to increase height, negative> to decrease. Step size is the value of `text-scale-mode-step'."
Yes.
Toggle quote (5 lines)> 4. Don't be surprised if Stefan doesn't go along with your change to not use the> temporary keymap. He just got through _adding_ such code here and there> throughout Emacs. (Makes no difference to me - my bug report was about the doc> string.)
I've nothing against temporary keymaps -- I thought the buggy behaviorI've found and reported was the default one, so I just implementedtext-scale-adjust another way. Also, I think the (while ... read-event)construct is a simple way to keep the message displayed. But surely more a matter of taste than a technical point.
<Thanks for the feedback,
-- Bastien
B
B
Bastien wrote on 11 Sep 2012 18:27
(name . Stefan Monnier)(address . monnier@iro.umontreal.ca)(address . 12345@debbugs.gnu.org)
87pq5smsmx.fsf@altern.org
Bastien <bzg@altern.org> writes:
Toggle quote (6 lines)> from emacs -q, try to edebug-defun `text-scale-adjust', > then use `C-x C-+', then `c' in the debug loop, then quit.>> The temporary keymap is not temporary anymore, and the `-'> and `+' keys are still bounded to `text-scale-adjust'.
I've been digging a bit further: in general, using `edebug-defun'on any command that uses `set-temporary-overlay-map' will leave the`pre-command-hook' in a dirty state.
One brute-force solution is to (setq emulation-mode-map-alists nil)manually to get rid of the temporary overlay map, but that's not right.
-- Bastien
S
S
Stefan Monnier wrote on 11 Sep 2012 19:21
(name . Bastien)(address . bzg@altern.org)(address . 12345@debbugs.gnu.org)
jwvd31swk5h.fsf-monnier+emacs@gnu.org
Toggle quote (5 lines)> from emacs -q, try to edebug-defun `text-scale-adjust', > then use `C-x C-+', then `c' in the debug loop, then quit.> The temporary keymap is not temporary anymore, and the `-'> and `+' keys are still bounded to `text-scale-adjust'.
Can you the patch below?

Stefan

=== modified file 'lisp/subr.el'--- lisp/subr.el 2012-09-07 10:19:58 +0000+++ lisp/subr.el 2012-09-11 17:20:45 +0000@@ -3898,6 +3898,7 @@ (lookup-key ',map (this-command-keys-vector)))) (t `(funcall ',keep-pred)))+ (set ',overlaysym nil) (remove-hook 'pre-command-hook ',clearfunsym) (setq emulation-mode-map-alists (delq ',alist emulation-mode-map-alists))))))
B
B
Bastien wrote on 11 Sep 2012 19:31
(name . Stefan Monnier)(address . monnier@IRO.UMontreal.CA)(address . 12345@debbugs.gnu.org)
87mx0wihzo.fsf@altern.org
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
Toggle quote (7 lines)>> from emacs -q, try to edebug-defun `text-scale-adjust', >> then use `C-x C-+', then `c' in the debug loop, then quit.>> The temporary keymap is not temporary anymore, and the `-'>> and `+' keys are still bounded to `text-scale-adjust'.>> Can you the patch below?
I tested it and it does not work for me.
-- Bastien
B
B
Bastien wrote on 11 Sep 2012 19:36
(name . Stefan Monnier)(address . monnier@iro.umontreal.ca)(address . 12345@debbugs.gnu.org)
87ipbkihqf.fsf@altern.org
Stefan Monnier <monnier@iro.umontreal.ca> writes:
Toggle quote (8 lines)>> I enhanced the docstring and rewrote the command so that it does>> not use a temporary keymap.>> Not using a temporary keymap means reverting to the old code: please> explain precisely why using a temporary keymap is a problem (there are> good reasons to use it: e.g. it makes key-translation-map and friends> work correctly).
Here is another patch, using ̀set-temporary-overlay-map'.
It fixes the problem with the message disappearing.
It fixed another problem: the fact that in the current version,INC is always 1 when using the command repeatedly, as we wouldexpect
C-u 4 C-x C-+ +
to increase by INC = 4 then again by INC = 4 again -- right nowit increase by INC = 4 on the first hit, then INC = 1 on the second.
It also changes the behavior regarding C-x C-0, which endsthe loop.
Let me know what you think,
=== modified file 'lisp/face-remap.el'--- lisp/face-remap.el 2012-07-10 11:51:54 +0000+++ lisp/face-remap.el 2012-09-11 17:20:16 +0000@@ -285,7 +285,9 @@ ;;;###autoload (define-key ctl-x-map [(control ?0)] 'text-scale-adjust) ;;;###autoload (defun text-scale-adjust (inc)- "Increase or decrease the height of the default face in the current buffer.+ "Adjust the height of the default face by INC.++INC may be passed as a numeric prefix argument. The actual adjustment made depends on the final component of the key-binding used to invoke the command, with all modifiers removed:@@ -294,9 +296,11 @@ - Decrease the default face height by one step 0 Reset the default face height to the global default -Then, continue to read input events and further adjust the face-height as long as the input event read (with all modifiers removed)-is one of the above.+When adjusting with `+' or `-', continue to read input events and+further adjust the face height as long as the input event read+\(with all modifiers removed) is `+' or `-'.++When adjusting with `0', immediately finish. Each step scales the height of the default face by the variable `text-scale-mode-step' (a negative number of steps decreases the@@ -309,8 +313,7 @@ a top-level keymap, `text-scale-increase' or `text-scale-decrease' may be more appropriate." (interactive "p")- (let ((first t)- (ev last-command-event)+ (let ((ev last-command-event) (echo-keystrokes nil)) (let* ((base (event-basic-type ev)) (step@@ -320,23 +323,16 @@ (?0 0) (t inc)))) (text-scale-increase step)- ;; FIXME: do it after every "iteration of the loop".- (message "+,-,0 for further adjustment: ")- (set-temporary-overlay-map- (let ((map (make-sparse-keymap)))- (dolist (mods '(() (control)))- (define-key map (vector (append mods '(?-))) 'text-scale-decrease)- (define-key map (vector (append mods '(?+))) 'text-scale-increase)- ;; = is unshifted + on most keyboards.- (define-key map (vector (append mods '(?=))) 'text-scale-increase)- (define-key map (vector (append mods '(?0)))- (lambda () (interactive) (text-scale-increase 0))))- map)- t))))-- -;; -----------------------------------------------------------------;; buffer-face-mode+ (unless (zerop step)+ (message "Use + or - for further adjustment")+ (set-temporary-overlay-map+ (let ((map (make-sparse-keymap)))+ (dolist (mods '(() (control)))+ (dolist (key '(?- ?+ ?=))+ (define-key map (vector (append mods (list key)))+ `(lambda () (interactive) (text-scale-adjust (abs ,step))))))+ map)+ t))))) (defcustom buffer-face-mode-face 'variable-pitch "The face specification used by `buffer-face-mode'.
-- Bastien
S
S
Stefan Monnier wrote on 12 Sep 2012 15:13
(name . Bastien)(address . bzg@altern.org)(address . 12345@debbugs.gnu.org)
jwvipbjgzcq.fsf-monnier+emacs@gnu.org
Toggle quote (5 lines)> from emacs -q, try to edebug-defun `text-scale-adjust', > then use `C-x C-+', then `c' in the debug loop, then quit.> The temporary keymap is not temporary anymore, and the `-'> and `+' keys are still bounded to `text-scale-adjust'.
There was a bug in edebug.el's handling of pre/post-command-hook.I just fixed it and it fixes your above recipe.Thanks for that recipe, BTW.

Stefan "haven't looked at your patch yet, sorry"
B
B
Bastien wrote on 12 Sep 2012 15:52
(name . Stefan Monnier)(address . monnier@iro.umontreal.ca)(address . 12345@debbugs.gnu.org)
878vcfz6uv.fsf@altern.org
Stefan Monnier <monnier@iro.umontreal.ca> writes:
Toggle quote (9 lines)>> from emacs -q, try to edebug-defun `text-scale-adjust', >> then use `C-x C-+', then `c' in the debug loop, then quit.>> The temporary keymap is not temporary anymore, and the `-'>> and `+' keys are still bounded to `text-scale-adjust'.>> There was a bug in edebug.el's handling of pre/post-command-hook.> I just fixed it and it fixes your above recipe.> Thanks for that recipe, BTW.
Glad I could help.
Toggle quote (2 lines)> Stefan "haven't looked at your patch yet, sorry"
Nothing urgent at all,
-- Bastien
D
D
Drew Adams wrote on 12 Sep 2012 16:16
RE: bug#12345: 24.2.50; doc string of `text-scale-adjust'
(address . 12345@debbugs.gnu.org)
8A73A81719BB46109B8B9AD938C5D993@us.oracle.com
Toggle quote (8 lines)> > from emacs -q, try to edebug-defun `text-scale-adjust', > > then use `C-x C-+', then `c' in the debug loop, then quit.> > The temporary keymap is not temporary anymore, and the `-'> > and `+' keys are still bounded to `text-scale-adjust'.> > There was a bug in edebug.el's handling of pre/post-command-hook.> I just fixed it and it fixes your above recipe.
Do you think that fix might also help with bug #12232?That would be great.
S
S
Stefan Monnier wrote on 13 Sep 2012 05:18
Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
(name . Drew Adams)(address . drew.adams@oracle.com)
jwvr4q6fw5g.fsf-monnier+emacs@gnu.org
Toggle quote (2 lines)> Do you think that fix might also help with bug #12232?
No, my fix only touches edebug.el which isn't even loaded inyour examples.

Sefan
S
S
Stefan Monnier wrote on 26 Oct 2012 19:12
(name . Bastien)(address . bzg@altern.org)(address . 12345-done@debbugs.gnu.org)
jwv625xxiu2.fsf-monnier+emacs@gnu.org
Toggle quote (2 lines)> Here is another patch, using ̀set-temporary-overlay-map'.
Thanks, I installed it with a few tweaks.
Toggle quote (3 lines)> It also changes the behavior regarding C-x C-0, which ends> the loop.
I did not include this change. I'm not really opposed to it, except forthe fact that it makes it necessary to have (and remember) the C-x C-0binding, whereas I like to use C-x C-+ as "entry point" even if I don'tactually want to increase the text size but instead reset it to itsdefault or decrease it.

Stefan
Closed
B
B
Bastien wrote on 27 Oct 2012 08:11
(name . Stefan Monnier)(address . monnier@iro.umontreal.ca)(address . 12345-done@debbugs.gnu.org)
87sj90sav2.fsf@bzg.ath.cx
Stefan Monnier <monnier@iro.umontreal.ca> writes:
Toggle quote (4 lines)>> Here is another patch, using ̀set-temporary-overlay-map'.>> Thanks, I installed it with a few tweaks.
Thanks,
Toggle quote (9 lines)>> It also changes the behavior regarding C-x C-0, which ends>> the loop.>> I did not include this change. I'm not really opposed to it, except for> the fact that it makes it necessary to have (and remember) the C-x C-0> binding, whereas I like to use C-x C-+ as "entry point" even if I don't> actually want to increase the text size but instead reset it to its> default or decrease it.
Yes, I also think it's better like this.
-- Bastien
Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 12345@debbugs.gnu.org