Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Toby Cubitt]

On Sun, Apr 15, 2012 at 11:41:00PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 15 Apr 2012 21:24:39 +0200
> > From: Toby Cubitt <tsc25@cantab.net>
> > 
> > Steps to reproduce:
> > 
> > 1. emacs -Q
> > 2. M-x calendar
> > 3. M-: (setq o (make-overlay 1 2))
> > 4. M-: (overlay-put o 'face 'font-lock-warning-face)
> > 5. Use C-n to move point to date in the last line of calendar
> > 
> > 
> > Symptoms:
> > 
> > When the point moves to the last line of the calendar in step 5., the
> > calendar buffer scrolls down, so that the top part of the calendar
> > scrolls off the top of the window and can't be seen.
> 
> This is not a bug, but a feature: Emacs does not allow the cursor to
> enter a partially visible line; it scrolls the buffer to make the line
> with the cursor fully visible.  Displaying characters in the
> font-lock-warning-face makes them slightly larger (because that face
> makes the characters bold), and that can cause the last line to exceed
> the visible portion of the window.

Aha, that makes sense.

> > The above steps are a minimal sequence needed to reproduce the bug. In
> > everyday Emacs use, the bug is triggered by date selection via
> > `org-read-date' in org-mode (which uses an overlay with non-null 'face
> > property in the calendar buffer to highlight the current date).
> 
> If that face also enlarges the characters, this is a feature of the
> Emacs display engine.

Yup, I chose `font-lock-warning-face' in the steps to reproduce because
org-mode sets `org-warning' face to this by default.

Even if this behaviour is related to a display engine feature, this is
nonetheless a bug in `org-read-date'.

It's very irritating to have the top part of the calendar scroll off the
top of the window. Apart from being ugly, it hides the month and day
names (in addition to half the dates), making it very difficult to select
the desired date.

The obvious solution is for org-mode to use a face that doesn't enlarge
the characters. I imagine that would mean defining a separate org-mode
face specifically for highlighting the date in the calendar, since
`org-warning' is used for all kinds of other things in org-mode for which
bold characters make sense (e.g. warning about todo items whose deadline
is due).

I've copied this to the org-mode mailing list, since presumably the
org-mode maintainers will have to take care of this.

Toby
-- 
Dr T. S. Cubitt
Mathematics and Quantum Information group
Department of Mathematics
Complutense University
Madrid, Spain

email: tsc25@cantab.net
web:   www.dr-qubit.org

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Eli Zaretskii]

> Date: Sun, 15 Apr 2012 23:04:16 +0200
> Cc: 11249@debbugs.gnu.org, emacs-orgmode@gnu.org
> From: Toby Cubitt <toby-dated-1335733484.f898fc@dr-qubit.org>
> 
> The obvious solution is for org-mode to use a face that doesn't enlarge
> the characters.

Another solution would be to enlarge the calendar window by one line.

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Toby Cubitt]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Mon, Apr 16, 2012 at 05:50:50AM +0300, Eli Zaretskii wrote:
> > Date: Sun, 15 Apr 2012 23:04:16 +0200
> > Cc: 11249@debbugs.gnu.org, emacs-orgmode@gnu.org
> > From: Toby Cubitt <toby-dated-1335733484.f898fc@dr-qubit.org>
> > 
> > The obvious solution is for org-mode to use a face that doesn't enlarge
> > the characters.
> 
> Another solution would be to enlarge the calendar window by one line.

I've attached a patch that does exactly this. As expected, it fixes the
bug for me.

As discussed previously, the alternative solution would be to change the
default face used to highlight the current date to something that doesn't
set the :bold attribute (which would probably mean introducing a separate
face for this, instead of reusing org-warning). But expanding the
calendar window is a more general solution, as it fixes the problem for
(almost) any face.


Arguably, it might be worth defining a separate face for highlighting the
date in the calendar during org-read-date, even if this patch is applied,
so that it can be customized separately from the face for impending todo
deadlines etc. If there's interest in adding a new
calendar-date-highlight face, I can post a separate patch for that.

For the record, one can already change the face used to highlight the
date in the calendar from within .emacs, via

  (overlay-put org-date-ovl 'face 'new-face)

But that's much less discoverable than defining a separate face for this.

Toby
-- 
Dr T. S. Cubitt
Mathematics and Quantum Information group
Department of Mathematics
Complutense University
Madrid, Spain

email: tsc25@cantab.net
web:   www.dr-qubit.org

[-- Attachment #2: 0001-Enlarge-calendar-by-1-line-to-fix-scrolling-bug-when.patch --]
[-- Type: text/x-patch, Size: 805 bytes --]

From 107b18f3ee306c0cd28566c8e7e5ba94c4b9268c Mon Sep 17 00:00:00 2001
From: "Toby S. Cubitt" <tsc25@cantab.net>
Date: Mon, 16 Apr 2012 14:22:35 +0200
Subject: [PATCH] Enlarge calendar by 1 line to fix scrolling bug when
 selecting date.

* lisp/org.el (org-read-date): Enlarge calendar window by 1 line, and
make cursor invisible.
---
 lisp/org.el |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 170ddc9..18ae685 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15106,6 +15106,8 @@ user."
       (save-excursion
 	(save-window-excursion
 	  (calendar)
+	  (org-eval-in-calendar
+	   '(progn (enlarge-window 1) (setq cursor-type nil)))
           (unwind-protect
               (progn
 		(calendar-forward-day (- (time-to-days org-def)
-- 
1.7.8.5

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Bastien]

Toby Cubitt <tsc25@cantab.net> writes:

> On Mon, Apr 16, 2012 at 05:50:50AM +0300, Eli Zaretskii wrote:
>> > Date: Sun, 15 Apr 2012 23:04:16 +0200
>> > Cc: 11249@debbugs.gnu.org, emacs-orgmode@gnu.org
>> > From: Toby Cubitt <toby-dated-1335733484.f898fc@dr-qubit.org>
>> > 
>> > The obvious solution is for org-mode to use a face that doesn't enlarge
>> > the characters.
>> 
>> Another solution would be to enlarge the calendar window by one line.
>
> I've attached a patch that does exactly this. As expected, it fixes the
> bug for me.
>
> As discussed previously, the alternative solution would be to change the
> default face used to highlight the current date to something that doesn't
> set the :bold attribute (which would probably mean introducing a separate
> face for this, instead of reusing org-warning). But expanding the
> calendar window is a more general solution, as it fixes the problem for
> (almost) any face.

I introduced a new face `org-date-selected' which is now the one in use
in the calendar and which is not bold.

Thanks for the detailed feedback,

-- 
 Bastien

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Toby Cubitt]

On Fri, Apr 20, 2012 at 01:57:55PM +0200, Bastien wrote:
> Toby Cubitt <tsc25@cantab.net> writes:
> 
> > On Mon, Apr 16, 2012 at 05:50:50AM +0300, Eli Zaretskii wrote:
> >> > Date: Sun, 15 Apr 2012 23:04:16 +0200
> >> > Cc: 11249@debbugs.gnu.org, emacs-orgmode@gnu.org
> >> > From: Toby Cubitt <toby-dated-1335733484.f898fc@dr-qubit.org>
> >> > 
> >> > The obvious solution is for org-mode to use a face that doesn't enlarge
> >> > the characters.
> >> 
> >> Another solution would be to enlarge the calendar window by one line.
> >
> > I've attached a patch that does exactly this. As expected, it fixes the
> > bug for me.
> >
> > As discussed previously, the alternative solution would be to change the
> > default face used to highlight the current date to something that doesn't
> > set the :bold attribute (which would probably mean introducing a separate
> > face for this, instead of reusing org-warning). But expanding the
> > calendar window is a more general solution, as it fixes the problem for
> > (almost) any face.
> 
> I introduced a new face `org-date-selected' which is now the one in use
> in the calendar and which is not bold.

Thanks for the fix. I notice that the new face's dosctring still says
"Face for deadlines and TODO keywords", which needs changing.

Was there some reason to reject Eli's suggested fix of enlarging the
calendar window by 1 line? As it stands, if anyone customizes the new
org-date-selected face to be bold (the highlighted date *is* a little
less visible now), it will cause the same problem as before. And the fact
that it's caused by making the font bold is not at all obvious...at
least, it wasn't obvious to me! Eli's fix works more generally.

Anyway, since I now know how to patch things if I do decide I want a bold
font, it's not a big issue for me.

In the patch I posted, I also took the opportunity to set `cursor-type'
to nil when opening the calendar in `org-read-date'. The cursor obscures
one digit of the selected date, making that bit harder to read (as well
as looking ugly). Did you intend to reject this change too?

Toby
-- 
Dr T. S. Cubitt
Mathematics and Quantum Information group
Department of Mathematics
Complutense University
Madrid, Spain

email: tsc25@cantab.net
web:   www.dr-qubit.org

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Bastien]

Toby Cubitt <tsc25@cantab.net> writes:

> Thanks for the fix. I notice that the new face's dosctring still says
> "Face for deadlines and TODO keywords", which needs changing.

Fixed, thanks.

> Was there some reason to reject Eli's suggested fix of enlarging the
> calendar window by 1 line? As it stands, if anyone customizes the new
> org-date-selected face to be bold (the highlighted date *is* a little
> less visible now), it will cause the same problem as before. And the fact
> that it's caused by making the font bold is not at all obvious...at
> least, it wasn't obvious to me! Eli's fix works more generally.

This general solution comes with a cost -- enlarging the window by
one-line for everyone just because a few users will use bold fonts and 
have a problem while displaying... does not feel right to me.

> Anyway, since I now know how to patch things if I do decide I want a bold
> font, it's not a big issue for me.
>
> In the patch I posted, I also took the opportunity to set `cursor-type'
> to nil when opening the calendar in `org-read-date'. The cursor obscures
> one digit of the selected date, making that bit harder to read (as well
> as looking ugly). Did you intend to reject this change too?

I did because I found the not-bold-anymore face wasn't visible enough,
and the shallow cursor made it visible.

I use inverse-video now for this face, so cursor-type nil is okay, 
I applied a patch with this.

-- 
 Bastien

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Toby Cubitt]

On Sun, Apr 22, 2012 at 08:46:03AM +0200, Bastien wrote:
> Toby Cubitt <tsc25@cantab.net> writes:
> > Was there some reason to reject Eli's suggested fix of enlarging the
> > calendar window by 1 line? As it stands, if anyone customizes the new
> > org-date-selected face to be bold (the highlighted date *is* a little
> > less visible now), it will cause the same problem as before. And the fact
> > that it's caused by making the font bold is not at all obvious...at
> > least, it wasn't obvious to me! Eli's fix works more generally.
> 
> This general solution comes with a cost -- enlarging the window by
> one-line for everyone just because a few users will use bold fonts and 
> have a problem while displaying... does not feel right to me.

Fair enough.

> > Anyway, since I now know how to patch things if I do decide I want a bold
> > font, it's not a big issue for me.
> >
> > In the patch I posted, I also took the opportunity to set `cursor-type'
> > to nil when opening the calendar in `org-read-date'. The cursor obscures
> > one digit of the selected date, making that bit harder to read (as well
> > as looking ugly). Did you intend to reject this change too?
> 
> I did because I found the not-bold-anymore face wasn't visible enough,
> and the shallow cursor made it visible.
> 
> I use inverse-video now for this face, so cursor-type nil is okay, 
> I applied a patch with this.

Inverse-video is a good solution - thanks!

Toby
-- 
Dr T. S. Cubitt
Mathematics and Quantum Information group
Department of Mathematics
Complutense University
Madrid, Spain

email: tsc25@cantab.net
web:   www.dr-qubit.org

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Matt Lundin]

Bastien <bzg@gnu.org> writes:

> Toby Cubitt <tsc25@cantab.net> writes:
>
>> In the patch I posted, I also took the opportunity to set
>> `cursor-type' to nil when opening the calendar in `org-read-date'.
>> The cursor obscures one digit of the selected date, making that bit
>> harder to read (as well as looking ugly). Did you intend to reject
>> this change too?
>
> I did because I found the not-bold-anymore face wasn't visible enough,
> and the shallow cursor made it visible.
>
> I use inverse-video now for this face, so cursor-type nil is okay, I
> applied a patch with this.

This patch broke org-read-date. It will no longer parse date strings
such as "Aug 15" correctly, instead inserting the current day.

Steps to reproduce

1. Start with a headline:

,----
| * A Headline
`----

2. Type C-c C-s (org-schedule)

3. Enter a future date (e.g., Aug 20).

4. Note the resulting headline:

,----
| * A Headline
|    SCHEDULED: <2012-04-25 Wed>
`----

If I comment out the following line:

	  (org-eval-in-calendar '(setq cursor-type nil))

org-read-date works correctly.

Best, Matt

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Matt Lundin]

Matt Lundin <mdl@imapmail.org> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Toby Cubitt <tsc25@cantab.net> writes:
>>
>>> In the patch I posted, I also took the opportunity to set
>>> `cursor-type' to nil when opening the calendar in `org-read-date'.
>>> The cursor obscures one digit of the selected date, making that bit
>>> harder to read (as well as looking ugly). Did you intend to reject
>>> this change too?
>>
>> I did because I found the not-bold-anymore face wasn't visible enough,
>> and the shallow cursor made it visible.
>>
>> I use inverse-video now for this face, so cursor-type nil is okay, I
>> applied a patch with this.
>
> This patch broke org-read-date. It will no longer parse date strings
> such as "Aug 15" correctly, instead inserting the current day.
>
[snip]
> If I comment out the following line:
>
> 	  (org-eval-in-calendar '(setq cursor-type nil))
>
> org-read-date works correctly.

Note: It seems that adding the optional argument KEEPDATE is necessary
for parsing to work. With the following line instead of the one above
org-read-date works as expected:

 	  (org-eval-in-calendar '(setq cursor-type nil) t)

Best,
Matt

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Bastien]

Hi Matt,

Matt Lundin <mdl@imapmail.org> writes:

> Note: It seems that adding the optional argument KEEPDATE is necessary
> for parsing to work. With the following line instead of the one above
> org-read-date works as expected:
>
>  	  (org-eval-in-calendar '(setq cursor-type nil) t)

that's right -- I pushed this fix to master.  Thanks for reporting this
and for detailing the solution!

-- 
 Bastien

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Toby Cubitt]

On Thu, Apr 26, 2012 at 10:59:50AM +0200, Bastien wrote:
> Hi Matt,
> 
> Matt Lundin <mdl@imapmail.org> writes:
> 
> > Note: It seems that adding the optional argument KEEPDATE is necessary
> > for parsing to work. With the following line instead of the one above
> > org-read-date works as expected:
> >
> >  	  (org-eval-in-calendar '(setq cursor-type nil) t)
> 
> that's right -- I pushed this fix to master.  Thanks for reporting this
> and for detailing the solution!

Thanks for the fix!

What exactly does KEEPDATE argument do? The `org-eval-in-calendar'
docstring doesn't mention it anywhere, which is probably why I missed it
in my patch. Looking at the code, it seems that the part about storing
the cursor date in org-ans2 only happens for a non-nil KEEPDATE?

I'd post an update to improve the `org-eval-in-calendar' docstring, but I
don't feel like I really understand what the KEEPDATE argument does well
enough.

Toby
-- 
Dr T. S. Cubitt
Mathematics and Quantum Information group
Department of Mathematics
Complutense University
Madrid, Spain

email: tsc25@cantab.net
web:   www.dr-qubit.org

Re: bug#11249: 24.1.50; Overlay with face property causes calendar buffer to scroll

[Bastien]

Toby Cubitt <tsc25@cantab.net> writes:

> On Thu, Apr 26, 2012 at 10:59:50AM +0200, Bastien wrote:
>> Hi Matt,
>> 
>> Matt Lundin <mdl@imapmail.org> writes:
>> 
>> > Note: It seems that adding the optional argument KEEPDATE is necessary
>> > for parsing to work. With the following line instead of the one above
>> > org-read-date works as expected:
>> >
>> >  	  (org-eval-in-calendar '(setq cursor-type nil) t)
>> 
>> that's right -- I pushed this fix to master.  Thanks for reporting this
>> and for detailing the solution!
>
> Thanks for the fix!
>
> What exactly does KEEPDATE argument do? The `org-eval-in-calendar'
> docstring doesn't mention it anywhere, which is probably why I missed it
> in my patch. Looking at the code, it seems that the part about storing
> the cursor date in org-ans2 only happens for a non-nil KEEPDATE?

When keepdate is nil (which is the default), org-ans2 is updated wrt the
cursor date. At the time we set the cursor-type, we don't want to override 
org-ans2, we want to stick to the value already set.

> I'd post an update to improve the `org-eval-in-calendar' docstring, but I
> don't feel like I really understand what the KEEPDATE argument does well
> enough.

I just fixed the docstring like this:

  "Eval FORM in the calendar window and return to current window.
When KEEPDATE is non-nil, update `org-ans2' from the cursor date,
otherwise stick to the current value of `org-ans2'."

Thanks!

-- 
 Bastien