(the above was supposed include the current date (day month year? just month year?) above the "calendar page"

I have a start on this now - a calendar implemented but not yet hooked up to the clock. It's based on the nbtk-introduction branch with NbtkTable so I need to sort that out.

Created attachment 144404 [details] [review]
Add a calendar pop-down to the clock

Here's a patch implementing the calendar popdown. It depends upon the shell-toolkit
branch including the patches in bug 596811 to add StTable.

Some things that might be debatable:

 - It isn't menu-like - it doesn't grab the keyboard and pointer when up;
   this is meant to allow someone to interact with an app while it is up.

 - Not sure the slide-in adds anything, though it's consistent with what
   LookingGlass does.

 - To get an input-region-but-no-struts actor, doing:

        Main.chrome.actor.add_actor(this.actor);
        Main.chrome.addInputRegionActor(this.actor);

   is definitely a bit of a hack. I wasn't quite sure about how to best rearrange
   all the chrome functions to enable this more cleanly.
   It maybe should be chrome.addActor(actor, { options: ... }) ?

Created attachment 144405 [details] [review]
testcommon.css: Don't theme all buttons

This patch isn't needed for the calendar popdown, but the test case
looks a bit ugly without it.

Created attachment 144533 [details] [review]
Add scroll-wheel support to the calendar

Make the calendar reactive and handle scroll events to change the month.
(GtkCalendar and hence the old gnome-panel calendar supported this and
it is apparently a handy way to flip through months.)

The padding is moved from the CalenderPopup to the Calendar so that the
scroll region extends all the way to the edge of the popup.

Comment on attachment 144404 [details] [review]
Add a calendar pop-down to the clock

>+#calendarPopup {
>+    border-radius: 5px;

It looks odd IMHO to have rounded borders on the top.

>+    background: rgba(0,0,0,0.9);

likewise, it just seems distracting/inconvenient to be able to see windows through the calendar. 

>+        let back = new St.Button({ label: "<", style_class: 'calendar-change-month'  });

< / > seem a little too pointy. I was going to suggest \u2039 / \u203A (single guillemets), which are closer to GtkCalendar, but they're too small, and my attempts to fiddle with the CSS didn't seem to do anything...

>+        iter.setSeconds(0); // Leap second protection. Hah!

If we're actually worrying about leap seconds, then every place that uses MSECS_IN_DAY is potentially wrong. Eg, starting at 00:00:00 on a day with a leap day and doing

>+            iter.setTime(iter.getTime() + MSECS_IN_DAY);

will set iter to 24:59:60 on the same day rather than 00:00:00 tomorrow.

In this particular case, since we don't care which dates we're looking at, just that they're valid dates, can't you do:

    for (let i = 0; i < 7; i++) {
        iter.setDate(i + 1);
        ...

? (This doesn't help with figuring out the actual days to show later on though)

>+    _update: function() {
>+        this._dateLabel.text = this.date.toLocaleFormat("%B %Y");

should use this._headerFormat

>+        for (let i = 8; i < children.length; i++)

"8"? srsly? (Yes, I know the comment I didn't quote explains it, but it seems pretty fragile.)

>+        Main.chrome.actor.add_actor(this.actor);
>+        Main.chrome.addInputRegionActor(this.actor);

You're not supposed to fiddle with Main.chrome.actor behind its back, and the fact that this is the only way to do what you wanted to do here should be considered a bug/missing feature in chrome.

I think

    Main.chrome.addActor(this.actor, null);
    Main.chrome.addInputRegionActor(this.calendar.actor);

would work without breaking abstraction barriers or changing chrome.

>+++ b/tests/interactive/calendar.js

>+const Gettext_gtk20 = imports.gettext.domain('gtk20');

don't need that there.

(In reply to comment #5)
> (From update of attachment 144404 [details] [review])
> >+#calendarPopup {
> >+    border-radius: 5px;
> 
> It looks odd IMHO to have rounded borders on the top.

I tried sliding it under the panel a few pixels to get rounded corners just at the bottom and it just looked weird.

> >+    background: rgba(0,0,0,0.9);
> 
> likewise, it just seems distracting/inconvenient to be able to see windows
> through the calendar. 

I think I'll leave that to the graphics artists to improve. This is apparently the same as we're using in the Fedora 12 notification bubble theme which is supposed to match.

> >+        let back = new St.Button({ label: "<", style_class: 'calendar-change-month'  });
> 
> < / > seem a little too pointy. I was going to suggest \u2039 / \u203A (single
> guillemets), which are closer to GtkCalendar, but they're too small, and my
> attempts to fiddle with the CSS didn't seem to do anything...

Hmm, trying to get small subscript characters to look not like small subscripts without breaking the overall spacing is going to be hard. But:

 .calendar-change-month {
     padding: 2px;
+    font-size: 200%;
 }

seems to work fine (was worried that you were seeing some CSS bug)

> >+        iter.setSeconds(0); // Leap second protection. Hah!
> 
> If we're actually worrying about leap seconds, then every place that uses
> MSECS_IN_DAY is potentially wrong. Eg, starting at 00:00:00 on a day with a
> leap day and doing
> 
> >+            iter.setTime(iter.getTime() + MSECS_IN_DAY);
> 
> will set iter to 24:59:60 on the same day rather than 00:00:00 tomorrow.

Oh, I knew there was a reason I used to have a iter.setHours(12); in there too. I'll add it back. (Yes, worrying about leap seconds to start with is ridiculous) 

> In this particular case, since we don't care which dates we're looking at, just
> that they're valid dates, can't you do:
> 
>     for (let i = 0; i < 7; i++) {
>         iter.setDate(i + 1);
>         ...

setDate(32) isn't going to work. setDate() needs to set a valid date.

> ? (This doesn't help with figuring out the actual days to show later on though)
> 
> >+    _update: function() {
> >+        this._dateLabel.text = this.date.toLocaleFormat("%B %Y");
> 
> should use this._headerFormat

Hmm, should have actually found local with the reversed and tested. (Did that now.) Fixed.

> >+        for (let i = 8; i < children.length; i++)
> 
> "8"? srsly? (Yes, I know the comment I didn't quote explains it, but it seems
> pretty fragile.)

OK, fixed with a:

+        // All the children after this are days, and get removed when we update the calendar
+        this._firstDayIndex = this.actor.get_children().length;

in the _init().
 
> >+        Main.chrome.actor.add_actor(this.actor);
> >+        Main.chrome.addInputRegionActor(this.actor);
> 
> You're not supposed to fiddle with Main.chrome.actor behind its back, and the
> fact that this is the only way to do what you wanted to do here should be
> considered a bug/missing feature in chrome.
> 
> I think
> 
>     Main.chrome.addActor(this.actor, null);
>     Main.chrome.addInputRegionActor(this.calendar.actor);
> 
> would work without breaking abstraction barriers or changing chrome.

Doesn't look like it to me - the actor will get tracked twice as far as I can see. OK if I leave this like this for the moment and file a bug for addActor(actor, { option: .. })? I'll try to get to that soon, but not today, tomorrow.

> >+++ b/tests/interactive/calendar.js
> 
> >+const Gettext_gtk20 = imports.gettext.domain('gtk20');
> 
> don't need that there.

Ah yes, as you can see the whole calendar was originally developed in that test case. Fixed.

Created attachment 144540 [details] [review]
Fixes for Calendar widget

Miscellaneous fixes from review:

- Distribute calendar.js and the interactive test
- Make the pointless protection against leap seconds actually work
  by starting in the middle of the day so that forward/back always
  move a day.
- Use a variable instead of an inline '8' to know where to start
  when removing old day actors.
- Remove a stray comment from the test

>  .calendar-change-month {
>      padding: 2px;
> +    font-size: 200%;
>  }
> 
> seems to work fine (was worried that you were seeing some CSS bug)

probably I was using the wrong syntax...

> > In this particular case, since we don't care which dates we're looking at, just
> > that they're valid dates, can't you do:
> > 
> >     for (let i = 0; i < 7; i++) {
> >         iter.setDate(i + 1);
> >         ...
> 
> setDate(32) isn't going to work. setDate() needs to set a valid date.

right, but it never does setDate(32). It does setDate(1) through setDate(7). That's what I meant about "since we don't care which dates we're looking at". Since the 1st through 7th exist in every month, we can just look at those.

> > ? (This doesn't help with figuring out the actual days to show later on though)

that comment still applies; if you redraw the calendar at exactly midnight when there is a day with a leap second visible, you'll lose. (The correct response to this may be "shrug".)

> > I think
> > 
> >     Main.chrome.addActor(this.actor, null);
> >     Main.chrome.addInputRegionActor(this.calendar.actor);
> > 
> > would work without breaking abstraction barriers or changing chrome.
> 
> Doesn't look like it to me - the actor will get tracked twice as far as I can
> see.

if you pass null for the second arg to addActor then it doesn't call trackActor.

Attachment 144533 [details] pushed as 061a2cf - Add scroll-wheel support to the calendar
Attachment 144540 [details] pushed as afb3b1e - Fixes for Calendar widget

(In reply to comment #8)

> > > In this particular case, since we don't care which dates we're looking at, just
> > > that they're valid dates, can't you do:
> > > 
> > >     for (let i = 0; i < 7; i++) {
> > >         iter.setDate(i + 1);
> > >         ...
> > 
> > setDate(32) isn't going to work. setDate() needs to set a valid date.
> 
> right, but it never does setDate(32). It does setDate(1) through setDate(7).
> That's what I meant about "since we don't care which dates we're looking at".
> Since the 1st through 7th exist in every month, we can just look at those.

Ah, yeah, sure.

> > > ? (This doesn't help with figuring out the actual days to show later on though)
> 
> that comment still applies; if you redraw the calendar at exactly midnight when
> there is a day with a leap second visible, you'll lose. (The correct response
> to this may be "shrug".)

I think it's fine with the current code. We start off at:

 12:xy:00

and step days from there. You'd have to go past ~40,000 leap seconds to hit the wrong day boundary. (And there's only been about a dozen leap seconds total, iirc)

> > > I think
> > > 
> > >     Main.chrome.addActor(this.actor, null);
> > >     Main.chrome.addInputRegionActor(this.calendar.actor);
> > > 
> > > would work without breaking abstraction barriers or changing chrome.
> > 
> > Doesn't look like it to me - the actor will get tracked twice as far as I can
> > see.
> 
> if you pass null for the second arg to addActor then it doesn't call
> trackActor.

OK, well, count it as confusing at least. :-).  I filed bug 597044 and added a reference to it in the code.