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

  • Done
  • quality assurance status badge
Details
4 participants
  • Bastien
  • Drew Adams
  • Stefan Monnier
  • Stefan Monnier
Owner
unassigned
Submitted by
Drew Adams
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 MARVIN
Bzr revision: 109861 eggert@cs.ucla.edu-20120902171035-7mzihil3xd6bjfiy
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured 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 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.
=== 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: 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).


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 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.

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.

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'."

The rest is OK.

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.)

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 to
resort 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 that
change 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 that
introduced 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 behavior
I've found and reported was the default one, so I just implemented
text-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 would
expect

C-u 4 C-x C-+ +

to increase by INC = 4 then again by INC = 4 again -- right now
it 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 ends
the 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 in
your 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 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.


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 an email to 12345@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 12345
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch