GNU bug report logs - #12345
24.2.50; doc string of `text-scale-adjust'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 12345 in the body.
You can then email your comments to 12345 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: <bug-gnu-emacs <at> gnu.org>
Subject: 24.2.50; doc string of `text-scale-adjust'
Date: Mon, 3 Sep 2012 19:54:25 -0700

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 <at> 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'
 

Message #8 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 15:37:31 +0200

[Message part 1 (text/plain, inline)]

"Drew Adams" <drew.adams <at> oracle.com> writes:

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

[face-remap.el.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
 Bastien

Message #11 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bastien <bzg <at> altern.org>
Cc: 12345 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 09:48:26 -0400

> 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

Message #14 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Bastien'" <bzg <at> altern.org>
Cc: 12345 <at> debbugs.gnu.org
Subject: RE: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 07:24:08 -0700

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

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

Message #17 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 16:24:26 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

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

Message #20 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Bastien'" <bzg <at> altern.org>, "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>
Cc: 12345 <at> debbugs.gnu.org
Subject: RE: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 07:39:43 -0700

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

Message #23 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 16:53:06 +0200

Hi Drew,

"Drew Adams" <drew.adams <at> oracle.com> writes:

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

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

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

> 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

Message #26 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 18:27:50 +0200

Bastien <bzg <at> altern.org> writes:

> 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

Message #29 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Bastien <bzg <at> altern.org>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 13:21:44 -0400

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

Message #32 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 19:31:23 +0200

Stefan Monnier <monnier <at> IRO.UMontreal.CA> writes:

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

Message #35 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Tue, 11 Sep 2012 19:36:56 +0200

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

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

[face-remap.el.2.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
 Bastien

Message #38 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bastien <bzg <at> altern.org>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Wed, 12 Sep 2012 09:13:10 -0400

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

Message #41 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Wed, 12 Sep 2012 15:52:08 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

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

>         Stefan "haven't looked at your patch yet, sorry"

Nothing urgent at all,

-- 
 Bastien

Message #44 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>, "'Bastien'" <bzg <at> altern.org>
Cc: 12345 <at> debbugs.gnu.org
Subject: RE: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Wed, 12 Sep 2012 07:16:52 -0700

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

Message #47 received at 12345 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 'Bastien' <bzg <at> altern.org>, 12345 <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Wed, 12 Sep 2012 23:18:49 -0400

> 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

Message #52 received at 12345-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bastien <bzg <at> altern.org>
Cc: 12345-done <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Fri, 26 Oct 2012 13:12:47 -0400

> Here is another patch, using ̀set-temporary-overlay-map'.

Thanks, I installed it with a few tweaks.

> 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

Message #55 received at 12345-done <at> debbugs.gnu.org (full text, mbox):

From: Bastien <bzg <at> altern.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12345-done <at> debbugs.gnu.org
Subject: Re: bug#12345: 24.2.50; doc string of `text-scale-adjust'
Date: Sat, 27 Oct 2012 08:11:13 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> Here is another patch, using ̀set-temporary-overlay-map'.
>
> Thanks, I installed it with a few tweaks.

Thanks,

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.