After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 632109 - Implement new calendar mockup
Implement new calendar mockup
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Florian Müllner
gnome-shell-maint
: 582058 (view as bug list)
Depends on: 612033 623017 624811 632626 633261
Blocks:
 
 
Reported: 2010-10-13 23:10 UTC by Maxim Ermilov
Modified: 2013-01-21 13:41 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
[WIP] implement Events List in calendar (11.67 KB, patch)
2010-10-14 04:23 UTC, Maxim Ermilov
none Details | Review
screenshot (35.04 KB, image/jpeg)
2010-10-14 04:24 UTC, Maxim Ermilov
  Details
calendar: implement Events List (14.58 KB, patch)
2010-10-20 04:15 UTC, Maxim Ermilov
committed Details | Review
[WIP] Implement time displays from mockup (78.78 KB, patch)
2010-10-27 11:41 UTC, Maxim Ermilov
none Details | Review
Diff from master (47.60 KB, patch)
2010-12-20 20:57 UTC, David Zeuthen (not reading bugmail)
needs-work Details | Review
Adjust for changes in commit 24f1e87 (8.67 KB, patch)
2011-01-18 20:26 UTC, Florian Müllner
committed Details | Review
Updated diff from master (45.53 KB, patch)
2011-01-19 20:23 UTC, David Zeuthen (not reading bugmail)
none Details | Review
Updated diff from master (45.54 KB, patch)
2011-01-20 18:24 UTC, David Zeuthen (not reading bugmail)
none Details | Review
Updated diff from master (169.81 KB, patch)
2011-01-27 16:19 UTC, David Zeuthen (not reading bugmail)
needs-work Details | Review
Updated diff from master (150.73 KB, patch)
2011-01-28 17:10 UTC, David Zeuthen (not reading bugmail)
none Details | Review
Update diff from master (152.75 KB, patch)
2011-01-28 18:34 UTC, David Zeuthen (not reading bugmail)
none Details | Review
Patch against master (154.85 KB, patch)
2011-01-28 21:38 UTC, David Zeuthen (not reading bugmail)
none Details | Review
Patch (155.76 KB, patch)
2011-01-28 21:48 UTC, David Zeuthen (not reading bugmail)
reviewed Details | Review

Description Maxim Ermilov 2010-10-13 23:10:25 UTC
I almost complete  patch for this (I will attach it later today).
It use evolution-data-server as events source.
Comment 1 Maxim Ermilov 2010-10-14 04:23:06 UTC
Created attachment 172324 [details] [review]
[WIP] implement Events List in calendar

patch for datetime branch.
require Evolution-Data-Server with patch from Bug 623017.
(after build, need start e-calendar-factory e-addressbook-factory)
Comment 2 Maxim Ermilov 2010-10-14 04:24:59 UTC
Created attachment 172325 [details]
screenshot
Comment 3 David Zeuthen (not reading bugmail) 2010-10-14 15:07:30 UTC
Very cool - some questions

 - Is the intent that the shell relies on Evolution? I'm thinking
   that it would probably be nice if the user could add an iCalendar
   URL [1] or maybe we use something like CalDAV [2].

   (Like many others I use web applications for mail or calendaring)

   Of course the nice thing is that the current setup punts all setup
   to Evolution. If we were to support iCalendar/CalDAV/etc in the
   shell then we'd need some kind of way in gnome-shell-clock-preferences
   or similar where the user can add/remove calendars. Maybe that's not
   so hard after all - I'm pretty sure we can do something a lot simpler
   than Evolution. I don't know.

   I think the answer is that we probably should support both...

   [1] : http://en.wikipedia.org/wiki/ICalendar
   [2] : http://en.wikipedia.org/wiki/CalDAV

 - Is the code for retrieving event synchronous (e.g. blocking) or
   asynchronous (it looks synchronous.. :-( ..). Have you considered
   just using the D-Bus interface for retrieving events?
Comment 4 David Zeuthen (not reading bugmail) 2010-10-14 15:31:08 UTC
Adding Matt for some feedback on comment 3. Thanks!
Comment 5 Maxim Ermilov 2010-10-14 16:03:12 UTC
(In reply to comment #3)
> Very cool - some questions
> 
>  - Is the intent that the shell relies on Evolution? I'm thinking
>    that it would probably be nice if the user could add an iCalendar
>    URL [1] or maybe we use something like CalDAV [2].
Evolution-Data-Server have a lot of backends(CalDAV, libical, GroupWise, Exchange, Web calendars).

source's urls store in one gconf key. It is easy to add custom configuration menu.
Comment 6 Matthew Barnes 2010-10-14 17:25:33 UTC
What Maxim said.

There's always been a desire to let users add calendars to Evolution-Data-Server without having to go through Evolution, but unfortunately all the calendar configuration widgetry still lives in Evolution.

A good mid-term goal for GNOME 3.x would be to add a "Setup Calendars" button to the gnome-shell calendar (maybe next to "Open Calendar"), which would let you manipulate E-D-S calendar sources without relying on Evolution.
Comment 7 David Zeuthen (not reading bugmail) 2010-10-14 19:01:24 UTC
OK, sounds good to me. What about the other question - e.g. is the e-d-s interfaces used asynchronous or synchronous? Thanks!
Comment 8 Matthew Barnes 2010-10-14 19:21:25 UTC
ECal is mostly synchronous, with maybe a couple awkwardly asynchronous bits.  It all predates GIO and needs a rewrite.
Comment 9 David Zeuthen (not reading bugmail) 2010-10-14 20:42:35 UTC
(In reply to comment #8)
> ECal is mostly synchronous, with maybe a couple awkwardly asynchronous bits. 
> It all predates GIO and needs a rewrite.

OK. Hmm, I don't think gjs supports threading. Maybe we could have a simple C class that exports the async operations .. I mean, with implementations running in a thread... That depends on ECal being thread-safe though.. or at least thread-safe enough to run from a thread - if it uses libdbus then it's a no...

Maybe just not bother about doing sync IO from the UI thread right now... thoughts?
Comment 10 Maxim Ermilov 2010-10-14 20:58:49 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > ECal is mostly synchronous, with maybe a couple awkwardly asynchronous bits. 
> > It all predates GIO and needs a rewrite.
> 
> OK. Hmm, I don't think gjs supports threading. Maybe we could have a simple C
> class that exports the async operations .. I mean, with implementations running
> in a thread... That depends on ECal being thread-safe though.. or at least
> thread-safe enough to run from a thread - if it uses libdbus then it's a no...

e-s-d have async variant (or alternatives) of all functions that used in patch. I will rewrite it.
Comment 11 Maxim Ermilov 2010-10-20 04:15:17 UTC
Created attachment 172790 [details] [review]
calendar: implement Events List

fully async.
can work without introspection for e-d-s(will always display 'no events')
(require patch for gjs from Bug 632626 )
Comment 12 David Zeuthen (not reading bugmail) 2010-10-20 18:32:38 UTC
Comment on attachment 172790 [details] [review]
calendar: implement Events List

Pushed that patch to the datetime branch (with a slight change to make it apply
(I just merged master into datetime)). I don't see any calendar events yet but
I think that's just a local problem with my e-d-s installation...
Comment 13 Maxim Ermilov 2010-10-27 11:41:30 UTC
Created attachment 173314 [details] [review]
[WIP] Implement time displays from mockup

Add 'change locations' to clock-preference
(depend on gjs patch from bug 624811 and second patch from Bug 612033, and Bug 633261)
Comment 14 David Zeuthen (not reading bugmail) 2010-11-16 17:20:34 UTC
Maxim: No events are showing up for me. I'm using the following gjs patches

 - the one from bug 632626
 - the one from bug 624811
 - and the second one from bug 612033 except hunk 4 (which
   does not apply anymore))

There are no warnings printed (gjs master prints warnings). Is it working for you? If so, exactly what patches against gjs are you using? Any chance you can upload them somewhere? Thanks!
Comment 15 David Zeuthen (not reading bugmail) 2010-11-16 17:21:30 UTC
FWIW, I'm using evolution-data-server-2.91.2-1.fc15.x86_64 - things work fine in the gnome 2 clock applet from my distro (e.g. my appointments are showing up).
Comment 16 Florian Müllner 2010-12-04 04:13:13 UTC
*** Bug 582058 has been marked as a duplicate of this bug. ***
Comment 17 David Zeuthen (not reading bugmail) 2010-12-09 19:44:10 UTC
Colin and I just talked about how to proceed

 - First, I blogged about this last week

   http://davidz25.blogspot.com/2010/11/gnome-3-calendar-integration.html

 - Currently we use fake events instead of e-d-s events. This
   allows us to make forward progress on the UI bits (some work is
   still needed)

 - Once the UI bits look good, I'll ask for review and get the code
   into a state so it can be committed

 - The interfaces used to get events (currently fake events) is very
   simple - it's basically just implementing two methods.

 - We'll then switch from fake events to no events

 - Then we can commit all the UI code. While there are no events being
   shown, the interface will at least have the new look+feel.

 - Once the UI code has been committed, it should be easy to write
   some C code using e-d-s that implements the simple interface for
   getting events. It is probably easiest if Matt or someone else
   with Evolution experience does this.
Comment 18 David Zeuthen (not reading bugmail) 2010-12-20 20:57:39 UTC
Created attachment 176790 [details] [review]
Diff from master

OK, I'd like to request a review of the code currently on the datetime branch. I've attached a patch with the diff from master:

 data/clock-preferences.ui           |   50 +++
 data/theme/calendar-arrow-left.svg  |   82 +++++
 data/theme/calendar-arrow-right.svg |   82 +++++
 data/theme/gnome-shell.css          |  175 +++++++++++-
 js/Makefile.am                      |    1 
 js/prefs/clockPreferences.js        |   13 
 js/ui/calendar.js                   |  500 +++++++++++++++++++++++++++++++-----
 js/ui/dateMenu.js                   |  219 +++++++++++++++
 js/ui/main.js                       |    4 
 js/ui/panel.js                      |   13 
 10 files changed, 1055 insertions(+), 84 deletions(-)

Because of all the changes, it's probably easiest to read js/ui/calendar.js itself instead of the patch. Here's a link:

 http://git.gnome.org/browse/gnome-shell/tree/js/ui/calendar.js?h=datetime

Caveats/issues:

 - I didn't find a way to implement the lookup requested by the designers, e.g.

    http://live.gnome.org/GnomeShell/Design/Whiteboards/DateNTime

   in particular because it's hard to have a menu with something side-by-side
   due to how the PopupMenu class works. So right now the "Date and Time
   Settings" button is a menu item spanning the entire width of the menu.

   I also didn't find a way to do stippled lines so I used a gradient.

   Someone who knows the gnome-shell internals better than me should be
   able to fix both issues.

 - I never got the Evolution integration to work, see comment 17 for more
   details. But everything is nicely abstracted so it's easy to change that
   if wanted (see EmptyEventSource and FakeEventSource in calendar.js). In
   particular the fake event source is fully fledged insofar that it actually
   synthesizes the ::changed signal and the UI is hooked up to act on it.

   As comment 17 suggests, it's probably easier to write a simple object in
   C using EDS and then hook it up, duck-typing and all.

   We'd also hook up "Open Calendar" when doing this work.

 - The locations stuff isn't done either - it's not clear how it should
   work given that gnome-control-panel also has a notion of Region and
   City (which, sigh, corresponds to timezone stuff).

   Design input is needed here.

 - The mockups http://live.gnome.org/GnomeShell/Design/Whiteboards/DateNTime
   call for displaying the week number but

    - week number is useless for e.g. Americans for showing the week number
      as in the mockup is very confusing

    - it's not clear to me that the mockup is useful for Europeans - you
      really want current behavior (e.g. week numbers shown in the calendar)
      since everyone else does that

    - not sure what's wrong with having a setting for this; in the default
      install we could even guess based on the locale and just punt it to
      the translators to choose, e.g. something like this

        let show_week = this._settings.get_boolean(SHOW_WEEKDATE_KEY);
        if (show_week == "auto") {
          /* Translators: set to show/hide depending on whether the week
           * number should be shown by default in your locale
           */
          show_week = _("show");
        }

      (e.g. the 'week-number' preference would default to the value 'auto'
      and the two other options are 'show' and 'hide'.)

    Or something less abusive of the translation machinery. I don't
    know. Anyway, in short, design input is needed here.

I would like to commit this code [1] and then open bugs for the remaining issues. Thanks for the review.

[1] : with this change:

--- a/js/ui/dateMenu.js
+++ b/js/ui/dateMenu.js
@@ -57,8 +57,8 @@ DateMenuButton.prototype = {
         let hbox;
         let vbox;


-        //this._event_source = new Calendar.EmptyEventSource();
-        this._event_source = new Calendar.FakeEventSource();
+        this._event_source = new Calendar.EmptyEventSource();
+        //this._event_source = new Calendar.FakeEventSource();
Comment 19 André Klapper 2010-12-20 21:06:32 UTC
        /* Translators: Abbreviation used in event list for Sunday */
        ret = _("Su");
        /* Translators: Abbreviation used in event list for Saturday */
        ret = _("S");
        /* Translators: Abbreviation used in event list for Tuesday */
        ret = _("T");
        /* Translators: Abbreviation used in event list for Thursday */
        ret = _("Th");

Thanks for the translator comments, but why don't have all strings two letters? I expect some sheeple to translate Thursday & Sunday with two letters and the rest with one, no matter what it means in their languages... :-/

        log('Translation of "calendar:week_start:0" in GTK+ is not correct');
Wondering: Would an additional "Please file a bug at https://bugzilla.gnome.org/enter_bug.cgi?product=L10N for the language that you use" make sense here?
Comment 20 David Zeuthen (not reading bugmail) 2010-12-21 15:11:13 UTC
(In reply to comment #19)
>         /* Translators: Abbreviation used in event list for Sunday */
>         ret = _("Su");
>         /* Translators: Abbreviation used in event list for Saturday */
>         ret = _("S");
>         /* Translators: Abbreviation used in event list for Tuesday */
>         ret = _("T");
>         /* Translators: Abbreviation used in event list for Thursday */
>         ret = _("Th");
> 
> Thanks for the translator comments, but why don't have all strings two letters?

Yeah, but it what the mockups at

 http://live.gnome.org/GnomeShell/Design/Whiteboards/DateNTime

ask for. So I defer to McCann/Jimmac on this question.

> I expect some sheeple to translate Thursday & Sunday with two letters and the
> rest with one, no matter what it means in their languages... :-/

Do you think this can be fixed with more translator comments?

>         log('Translation of "calendar:week_start:0" in GTK+ is not correct');
> Wondering: Would an additional "Please file a bug at
> https://bugzilla.gnome.org/enter_bug.cgi?product=L10N for the language that you
> use" make sense here?

I don't think it matters since this output goes to something like ~/.xsession-errors and no-one reads that anyway. But, sure, wouldn't hurt.
Comment 21 Owen Taylor 2011-01-03 20:15:17 UTC
Assigning to Florian for review
Comment 22 Florian Müllner 2011-01-18 20:14:14 UTC
Review of attachment 176790 [details] [review]:

Looking forward to seeing this land! The branch will need some adjustments for the changes in commit 24f1e87 (sorry for that - basically the custom gnome-shell-clock-preferences has gone in favor of the control center's datetime panel; I'll attach a patch which should make merging with master easier). Apart from the issues pointed out below, there are a lot of style issues not pointed out in detail; basically all javascript code should use camelCase (in contrast with methods/properties from GI).

::: data/theme/calendar-arrow-left.svg
@@ +1,1 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>

Needs to be added to Makefile.am

::: data/theme/calendar-arrow-right.svg
@@ +1,1 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>

Dto.

::: js/ui/calendar.js
@@ +23,3 @@
 
+/* TODO: maybe needs config - right now we assume that Saturday and
+ * Sunday are work days (not true in e.g. Israel, it's Sunday and

non-work, work-free or something

@@ +64,3 @@
+         * together and in order, e.g. "S M T W T F S"
+         */
+        ret = _("S");

Missing "Calendar abbreviation for Sunday" comment; maybe it's more concise to do something like:

let dayAbbreviations = [_("S") /* Translators: Calendar abbreviation used for Sunday */,
                        _("M") /* ... */]
return dayAbbreviations[dayNumber];

@@ +141,3 @@
+}
+
+// ------------------------------------------------------------------------

We generally don't use these separator lines

@@ +142,3 @@
+
+// ------------------------------------------------------------------------
+// Abstraction for an appointment/task in a calendar

Should be directly above the prototype, e.g.

// Abstraction for an appointment/task in a calendar
function CalendarTask(date, summary) {}

@@ +183,3 @@
+Signals.addSignalMethods(EmptyEventSource.prototype);
+
+// Second, an implementation with fake events

OK to get designer input, but should probably go away before pushing

@@ +204,3 @@
+        // '10-oclock pow-wow' is an event occuring IN THE PAST every four days at 10am
+        for(let n = 0; n < 10; n++) {
+            let t = new Date(now.getTime() - n*4*86400*1000);

Missing spaces around '*'

@@ +212,3 @@
+        // '11-oclock thing' is an event occuring every three days at 11am
+        for(let n = 0; n < 10; n++) {
+            let t = new Date(now.getTime() + n*3*86400*1000);

... and here ...

@@ +220,3 @@
+        // 'Weekly Meeting' is an event occuring every seven days at 1:45pm (two days displaced)
+        for(let n = 0; n < 5; n++) {
+            let t = new Date(now.getTime() + (n*7+2)*86400*1000);

... and here

@@ +227,3 @@
+        }
+
+        // 'Get Married' is an event that actually reflects reality (Dec 4, 2010) :-)

Should be 'Got Married' - congrats BTW :-)

@@ +228,3 @@
+
+        // 'Get Married' is an event that actually reflects reality (Dec 4, 2010) :-)
+        this._fake_tasks.push(new CalendarTask(new Date(2010,11,4,16,0), 'Get Married'));

Missing spaces after ','

@@ +231,3 @@
+
+        // ditto for 'NE Patriots vs NY Jets'
+        this._fake_tasks.push(new CalendarTask(new Date(2010,11,6,20,30), 'NE Patriots vs NY Jets'));

Dto

@@ +294,3 @@
+//
+// @event_list is the EventList object to control
+//

Should be gtk-doc like, e.g.

// Calendar:
// @eventSource: an object implementing the EventSource API, ...
// @eventList: the EventList to control
function Calendar(eventSource, eventList) {}

That said, I don't really like the eventList parameter - at least for me, it makes more sense to keep the prototype in DateMenu and adapt to calendar changes, rather than let the calendar control it (e.g. connect to 'selected-date-changed' in DateMenuButton)

@@ +304,3 @@
+        this._event_list = event_list;
+
+        this._event_source.connect('changed', Lang.bind(this, this._onEventSourceChanged));

Could just as well use this._update directly

@@ +391,3 @@
         // we do this by just getting the next 7 days starting from right now and then putting
         // them in the right cell in the table. It doesn't matter if we add them in order
+        //

Remove trailing empty comment

@@ +489,3 @@
         let row = 2;
+        let dayButtons = [];
+        this._dayButtons = dayButtons;

Unused.

@@ +568,3 @@
+        this.actor = new St.BoxLayout({ vertical: true, style_class: 'events-header-vbox'});
+        // FIXME: Evolution backend is currently disabled
+        // this.evolutionTasks = new EvolutionEventsSource();

Uhm - so eventSource is passed in as a parameter, but there should be a separate property evolutionTasks? Doesn't really make sense to me ...

@@ +570,3 @@
+        // this.evolutionTasks = new EvolutionEventsSource();
+
+        this._event_source = event_source;

Hmmm, looks like changes to eventSource are handled from the calendar as well. I think encapsulation is better when doing something like

DateMenuButton {
   this._eventSource = new EventSource();
   this._eventList = new EventList(this._eventSource);
   this._calendar = new Calendar(this._eventSource);

   this._calendar.connect('selected-date-changed',
                          Lang.bind(this, function(cal, date) {
                              this._eventList.displayForDate(date);
                          });
}

EventList {
    _init(eventSource) {
       this._date = new Date();
       this._eventSource = eventSource;
       this._eventSource.connect('changed',
                                 Lang.bind(this, this._updateEvents));
    }

    displayForDate(date) { // shitty name, feel free to come up with sth better
       if (_sameDay(this._date, date))
           return;
       this._date = date;
       this._updateEvents();
    }

    _updateEvents() {
       let today = new Date();
       if (_sameDay(this._date, today))
          this._showToday();
       else
          this._showOtherDay();
    }
}

@@ +575,3 @@
+    _addEvent: function(dayNameBox, timeBox, eventTitleBox, includeDayName, day, time, desc) {
+        if (includeDayName) {
+            dayNameBox.add(new St.Label({ style_class: 'events-day-dayname', text: day}), {x_fill: false});

Could use some line breaks; missing spaces before '{' and after '}'

@@ +577,3 @@
+            dayNameBox.add(new St.Label({ style_class: 'events-day-dayname', text: day}), {x_fill: false});
+        }
+        timeBox.add(new St.Label({ style_class: 'events-day-time', text: time}), {x_fill: false});

Here as well

@@ +578,3 @@
+        }
+        timeBox.add(new St.Label({ style_class: 'events-day-time', text: time}), {x_fill: false});
+        eventTitleBox.add(new St.Label({ style_class: 'events-day-task', text: desc}));

space before }

@@ +594,3 @@
+        let dayNameBox = new St.BoxLayout({ vertical: true, style_class: 'events-day-name-box' });
+        let timeBox = new St.BoxLayout({ vertical: true, style_class: 'events-time-box' });
+        let eventTitleBox = new St.BoxLayout({ vertical: true, style_class: 'events-event-box' });

Splitting events between three vertical boxes is pretty ugly - maybe you can use St.Table instead?

::: js/ui/dateMenu.js
@@ +22,3 @@
+const CLOCK_SHOW_SECONDS_KEY  = 'show-seconds';
+
+function DateMenuButton() {

Should be right above the prototype (e.g. below onVertSepRepaint)

@@ +56,3 @@
+        let item;
+        let hbox;
+        let vbox;

Another C-ism - we generally don't do forward declarations in JS.

@@ +71,3 @@
+
+        // Fill up the first column
+        //

Should be an empty line instead of '//'

@@ +87,3 @@
+
+        // Add vertical separator
+        //

Dto.

@@ +95,3 @@
+
+        // Fill up the second column
+        //

Here as well

@@ +100,3 @@
+
+        // Whenever the menu is opened, select today
+        //

Remove empty comment line

@@ +109,3 @@
+
+        // Done with hbox for calendar and event list
+        //

Here as well.

@@ +118,3 @@
+        item = new PopupMenu.PopupImageMenuItem(_("Date and Time Settings"), 'gnome-shell-clock-preferences');
+        item.connect('activate', Lang.bind(this, this._onPreferencesActivate));
+        this.menu.addMenuItem(item);

There's a shortcut for this:
this.menu.addAction(_("Date and Time Settings"),
                    Lang.bind(this, this._onPreferencesActivate));

@@ +122,3 @@
+        // Track changes to clock settings
+        this._clockSettings = new Gio.Settings({ schema: 'org.gnome.shell.clock' });
+        this._clockSettings.connect('changed', Lang.bind(this, this._clockSettingsChanged));

Why aren't you using _updateClockAndDate() directly?

@@ +206,3 @@
+
+    _onPreferencesActivate: function() {
+        Main.overview.hide();

We don't leave the overview when opening any other settings panel, so we shouldn't do it here either.

@@ +207,3 @@
+    _onPreferencesActivate: function() {
+        Main.overview.hide();
+        this._spawn(['gnome-shell-clock-preferences']);

Should be replaced by Util.spawnDesktop('gnome-datetime-panel');

::: js/ui/panel.js
@@ -801,3 @@
         /* center */
-
-        this._clockButton = new ClockButton();

ClockButton is no longer used and should be removed.
Comment 23 Florian Müllner 2011-01-18 20:26:01 UTC
Created attachment 178654 [details] [review]
Adjust for changes in commit 24f1e87

Small patch to make merging master easier:
  - revert changes to clock-preferences.ui/gnome-shell-clock-preferences.in
    (removed in master)
  - adjust dateMenu.js to pick up settings from control-center instead
Comment 24 David Zeuthen (not reading bugmail) 2011-01-19 16:15:07 UTC
(In reply to comment #23)
> Created an attachment (id=178654) [details] [review]
> Adjust for changes in commit 24f1e87
> 
> Small patch to make merging master easier:
>   - revert changes to clock-preferences.ui/gnome-shell-clock-preferences.in
>     (removed in master)
>   - adjust dateMenu.js to pick up settings from control-center instead

Hey, thanks for that patch. I've applied it and merged master into the branch - this is all in the datetime branch on git.gnome.org now. And thanks for the review in comment 22 too. I'll start working on addressing these comments.
Comment 25 David Zeuthen (not reading bugmail) 2011-01-19 20:21:14 UTC
Comment on attachment 178654 [details] [review]
Adjust for changes in commit 24f1e87

Committed on the datemenu branch
Comment 26 David Zeuthen (not reading bugmail) 2011-01-19 20:23:37 UTC
Created attachment 178773 [details] [review]
Updated diff from master

$ diffstat datemenu-take2.patch 
 data/Makefile.am                    |    2 
 data/theme/calendar-arrow-left.svg  |   82 ++++++
 data/theme/calendar-arrow-right.svg |   82 ++++++
 data/theme/gnome-shell.css          |  175 ++++++++++++-
 js/Makefile.am                      |    1 
 js/ui/calendar.js                   |  453 +++++++++++++++++++++++++++++++-----
 js/ui/dateMenu.js                   |  204 ++++++++++++++++
 js/ui/main.js                       |    4 
 js/ui/panel.js                      |  126 ----------
 9 files changed, 933 insertions(+), 196 deletions(-)
Comment 27 David Zeuthen (not reading bugmail) 2011-01-19 20:36:35 UTC
(In reply to comment #22)
> Review of attachment 176790 [details] [review]:

Hey - thanks for the review.

> 
> Looking forward to seeing this land! The branch will need some adjustments for
> the changes in commit 24f1e87 (sorry for that - basically the custom
> gnome-shell-clock-preferences has gone in favor of the control center's
> datetime panel; I'll attach a patch which should make merging with master
> easier). 

There's a couple of settings that the shell relies on that cannot be set in the control center (show date, show week numbers etc.). But I don't think we need to worry about that - if people want to change the setting they can use the gsettings(1) program or dconf-editor or whatever.

(This is similar to GNOME 2 having a lot of settings that can only be changed via gconf-editor etc.)

> Apart from the issues pointed out below, there are a lot of style
> issues not pointed out in detail; basically all javascript code should use
> camelCase (in contrast with methods/properties from GI).

OK, I wasn't aware of this convention. Fixed.

> ::: data/theme/calendar-arrow-left.svg
> @@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> 
> Needs to be added to Makefile.am

Done.

> ::: data/theme/calendar-arrow-right.svg
> @@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> 
> Dto.

Done.

> ::: js/ui/calendar.js
> @@ +23,3 @@
> 
> +/* TODO: maybe needs config - right now we assume that Saturday and
> + * Sunday are work days (not true in e.g. Israel, it's Sunday and
> 
> non-work, work-free or something

Not sure what you want me to do here...

> @@ +64,3 @@
> +         * together and in order, e.g. "S M T W T F S"
> +         */
> +        ret = _("S");
> 
> Missing "Calendar abbreviation for Sunday" comment; maybe it's more concise to
> do something like:
> 
> let dayAbbreviations = [_("S") /* Translators: Calendar abbreviation used for
> Sunday */,
>                         _("M") /* ... */]
> return dayAbbreviations[dayNumber];


Done. Ditto for event list abbreviations.


> +// ------------------------------------------------------------------------
> 
> We generally don't use these separator lines

Nuked.

> @@ +142,3 @@
> +
> +// ------------------------------------------------------------------------
> +// Abstraction for an appointment/task in a calendar
> 
> Should be directly above the prototype, e.g.

Done.

> OK to get designer input, but should probably go away before pushing

That's your decision, of course. But it's pretty useful when doing UI changes (in particular the transient event every second is useful).

> @@ +204,3 @@
> +        // '10-oclock pow-wow' is an event occuring IN THE PAST every four
> days at 10am
> +        for(let n = 0; n < 10; n++) {
> +            let t = new Date(now.getTime() - n*4*86400*1000);
> 
> Missing spaces around '*'

Done.

> @@ +212,3 @@
> +        // '11-oclock thing' is an event occuring every three days at 11am
> +        for(let n = 0; n < 10; n++) {
> +            let t = new Date(now.getTime() + n*3*86400*1000);
> 
> ... and here ...

Done.
 
> @@ +220,3 @@
> +        // 'Weekly Meeting' is an event occuring every seven days at 1:45pm
> (two days displaced)
> +        for(let n = 0; n < 5; n++) {
> +            let t = new Date(now.getTime() + (n*7+2)*86400*1000);
> 
> ... and here

Done.

> @@ +227,3 @@
> +        }
> +
> +        // 'Get Married' is an event that actually reflects reality (Dec 4,
> 2010) :-)
> 
> Should be 'Got Married' - congrats BTW :-)

Thanks! :-)

> @@ +228,3 @@
> +
> +        // 'Get Married' is an event that actually reflects reality (Dec 4,
> 2010) :-)
> +        this._fake_tasks.push(new CalendarTask(new Date(2010,11,4,16,0), 'Get
> Married'));
> 
> Missing spaces after ','

Fixed.

> @@ +231,3 @@
> +
> +        // ditto for 'NE Patriots vs NY Jets'
> +        this._fake_tasks.push(new CalendarTask(new Date(2010,11,6,20,30), 'NE
> Patriots vs NY Jets'));
> 
> Dto

Fixed.

> @@ +294,3 @@
> +//
> +// @event_list is the EventList object to control
> +//
> 
> Should be gtk-doc like, e.g.
> 
> // Calendar:
> // @eventSource: an object implementing the EventSource API, ...
> // @eventList: the EventList to control
> function Calendar(eventSource, eventList) {}

Fixed.

> That said, I don't really like the eventList parameter - at least for me, it
> makes more sense to keep the prototype in DateMenu and adapt to calendar
> changes, rather than let the calendar control it (e.g. connect to
> 'selected-date-changed' in DateMenuButton)

I agree. Fixed.

> @@ +304,3 @@
> +        this._event_list = event_list;
> +
> +        this._event_source.connect('changed', Lang.bind(this,
> this._onEventSourceChanged));
> 
> Could just as well use this._update directly

Done.

> @@ +391,3 @@
>          // we do this by just getting the next 7 days starting from right now
> and then putting
>          // them in the right cell in the table. It doesn't matter if we add
> them in order
> +        //
> 
> Remove trailing empty comment

Done.

> @@ +489,3 @@
>          let row = 2;
> +        let dayButtons = [];
> +        this._dayButtons = dayButtons;
> 
> Unused.

Nuked.

> @@ +568,3 @@
> +        this.actor = new St.BoxLayout({ vertical: true, style_class:
> 'events-header-vbox'});
> +        // FIXME: Evolution backend is currently disabled
> +        // this.evolutionTasks = new EvolutionEventsSource();
> 
> Uhm - so eventSource is passed in as a parameter, but there should be a
> separate property evolutionTasks? Doesn't really make sense to me ...

This is just old stale code.

> @@ +570,3 @@
> +        // this.evolutionTasks = new EvolutionEventsSource();
> +
> +        this._event_source = event_source;
> 
> Hmmm, looks like changes to eventSource are handled from the calendar as well.
> I think encapsulation is better when doing something like
> 
> DateMenuButton {
>    this._eventSource = new EventSource();
>    this._eventList = new EventList(this._eventSource);
>    this._calendar = new Calendar(this._eventSource);
> 
>    this._calendar.connect('selected-date-changed',
>                           Lang.bind(this, function(cal, date) {
>                               this._eventList.displayForDate(date);
>                           });
> }
> 
> EventList {
>     _init(eventSource) {
>        this._date = new Date();
>        this._eventSource = eventSource;
>        this._eventSource.connect('changed',
>                                  Lang.bind(this, this._updateEvents));
>     }
> 
>     displayForDate(date) { // shitty name, feel free to come up with sth better
>        if (_sameDay(this._date, date))
>            return;
>        this._date = date;
>        this._updateEvents();
>     }
> 
>     _updateEvents() {
>        let today = new Date();
>        if (_sameDay(this._date, today))
>           this._showToday();
>        else
>           this._showOtherDay();
>     }
> }

OK, I did something like this.

> @@ +575,3 @@
> +    _addEvent: function(dayNameBox, timeBox, eventTitleBox, includeDayName,
> day, time, desc) {
> +        if (includeDayName) {
> +            dayNameBox.add(new St.Label({ style_class: 'events-day-dayname',
> text: day}), {x_fill: false});
> 
> Could use some line breaks; missing spaces before '{' and after '}'

Fixed.

> @@ +577,3 @@
> +            dayNameBox.add(new St.Label({ style_class: 'events-day-dayname',
> text: day}), {x_fill: false});
> +        }
> +        timeBox.add(new St.Label({ style_class: 'events-day-time', text:
> time}), {x_fill: false});
> 
> Here as well

Fixed.

> @@ +578,3 @@
> +        }
> +        timeBox.add(new St.Label({ style_class: 'events-day-time', text:
> time}), {x_fill: false});
> +        eventTitleBox.add(new St.Label({ style_class: 'events-day-task', text:
> desc}));
> 
> space before }

OK. Fixed.

> @@ +594,3 @@
> +        let dayNameBox = new St.BoxLayout({ vertical: true, style_class:
> 'events-day-name-box' });
> +        let timeBox = new St.BoxLayout({ vertical: true, style_class:
> 'events-time-box' });
> +        let eventTitleBox = new St.BoxLayout({ vertical: true, style_class:
> 'events-event-box' });
> 
> Splitting events between three vertical boxes is pretty ugly - maybe you can
> use St.Table instead?

I tried doing that originally but IIRC ran into issues with spacing.

> ::: js/ui/dateMenu.js
> @@ +22,3 @@
> +const CLOCK_SHOW_SECONDS_KEY  = 'show-seconds';
> +
> +function DateMenuButton() {
> 
> Should be right above the prototype (e.g. below onVertSepRepaint)

Fixed.

> @@ +71,3 @@
> +
> +        // Fill up the first column
> +        //
> 
> Should be an empty line instead of '//'

Fixed.

> @@ +87,3 @@
> +
> +        // Add vertical separator
> +        //
> 
> Dto.

Fixed.

> @@ +95,3 @@
> +
> +        // Fill up the second column
> +        //
> 
> Here as well

Fixed.

> @@ +100,3 @@
> +
> +        // Whenever the menu is opened, select today
> +        //
> 
> Remove empty comment line

Fixed.

> @@ +109,3 @@
> +
> +        // Done with hbox for calendar and event list
> +        //
> 
> Here as well.

Fixed.

> @@ +118,3 @@
> +        item = new PopupMenu.PopupImageMenuItem(_("Date and Time Settings"),
> 'gnome-shell-clock-preferences');
> +        item.connect('activate', Lang.bind(this,
> this._onPreferencesActivate));
> +        this.menu.addMenuItem(item);
> 
> There's a shortcut for this:
> this.menu.addAction(_("Date and Time Settings"),
>                     Lang.bind(this, this._onPreferencesActivate));

Done.

> @@ +122,3 @@
> +        // Track changes to clock settings
> +        this._clockSettings = new Gio.Settings({ schema:
> 'org.gnome.shell.clock' });
> +        this._clockSettings.connect('changed', Lang.bind(this,
> this._clockSettingsChanged));
> 
> Why aren't you using _updateClockAndDate() directly?

It's a style-thing, mostly. Mostly for the case where you want to run code when settings change but not when updating. But since the only code we want to run when settings change _currently_ is only _updateClockAndDate() I guess we can short-circuit to that. I don't care much so I changed it to what you suggested.

> @@ +206,3 @@
> +
> +    _onPreferencesActivate: function() {
> +        Main.overview.hide();
> 
> We don't leave the overview when opening any other settings panel, so we
> shouldn't do it here either.

OK, nuked.

> @@ +207,3 @@
> +    _onPreferencesActivate: function() {
> +        Main.overview.hide();
> +        this._spawn(['gnome-shell-clock-preferences']);
> 
> Should be replaced by Util.spawnDesktop('gnome-datetime-panel');

Done.

> ::: js/ui/panel.js
> @@ -801,3 @@
>          /* center */
> -
> -        this._clockButton = new ClockButton();
> 
> ClockButton is no longer used and should be removed.

OK, nuked.
Comment 28 Florian Müllner 2011-01-19 21:31:20 UTC
(In reply to comment #27)
> There's a couple of settings that the shell relies on that cannot be set in the
> control center (show date, show week numbers etc.). But I don't think we need
> to worry about that - if people want to change the setting they can use the
> gsettings(1) program or dconf-editor or whatever.
> 
> (This is similar to GNOME 2 having a lot of settings that can only be changed
> via gconf-editor etc.)

Yeah, this is already the case in master, and I don't think it's much of a problem (though it's a bit ugly that the settings are split between two schemas).



> > ::: js/ui/calendar.js
> > @@ +23,3 @@
> > 
> > +/* TODO: maybe needs config - right now we assume that Saturday and
> > + * Sunday are work days (not true in e.g. Israel, it's Sunday and
> > 
> > non-work, work-free or something
> 
> Not sure what you want me to do here...

In the comment: "assume that Saturday and Sunday are work days"
I think that's not true in a lot of countries apart from Israel ;-)


> > OK to get designer input, but should probably go away before pushing
> 
> That's your decision, of course. But it's pretty useful when doing UI changes
> (in particular the transient event every second is useful).

Well, we should try to get real events in there anyway. I'm OK with leaving it in for now, but then I'm not a designer ...


> > @@ +594,3 @@
> > Splitting events between three vertical boxes is pretty ugly - maybe you can
> > use St.Table instead?
> 
> I tried doing that originally but IIRC ran into issues with spacing.

OK.
Comment 29 Matthew Barnes 2011-01-19 21:42:06 UTC
(In reply to comment #27)
> There's a couple of settings that the shell relies on that cannot be set in the
> control center (show date, show week numbers etc.). But I don't think we need
> to worry about that - if people want to change the setting they can use the
> gsettings(1) program or dconf-editor or whatever.

Consider adding "show week numbers" and perhaps "12 / 24 hour format" to gsettings-desktop-schemas.

Evolution has options for these in its Preferences window.  If we could link them to desktop-wide settings then the gnome-shell calendar and Evolution would automatically look more consistent.
Comment 30 Florian Müllner 2011-01-19 23:00:14 UTC
(In reply to comment #29)
> Consider adding "show week numbers" and perhaps "12 / 24 hour format" to
> gsettings-desktop-schemas.

"12 / 24 hour format" already is in org.gnome.desktop.interface ("clock-format" with possible values of "12h"/"24h"). The settings left in gnome-shell are "show week numbers", "show seconds" and "show date".


> Evolution has options for these in its Preferences window.  If we could link
> them to desktop-wide settings then the gnome-shell calendar and Evolution would
> automatically look more consistent.

Sounds like a good idea to me.
Comment 31 David Zeuthen (not reading bugmail) 2011-01-20 18:24:14 UTC
Created attachment 178868 [details] [review]
Updated diff from master

(In reply to comment #28)
> (In reply to comment #27)
> > There's a couple of settings that the shell relies on that cannot be set in the
> > control center (show date, show week numbers etc.). But I don't think we need
> > to worry about that - if people want to change the setting they can use the
> > gsettings(1) program or dconf-editor or whatever.
> > 
> > (This is similar to GNOME 2 having a lot of settings that can only be changed
> > via gconf-editor etc.)
> 
> Yeah, this is already the case in master, and I don't think it's much of a
> problem (though it's a bit ugly that the settings are split between two
> schemas).

Right. Moving some (or all) of these keys to gsettings-desktop-schemas sounds good and, I guess, can be done in the future.

> In the comment: "assume that Saturday and Sunday are work days"
> I think that's not true in a lot of countries apart from Israel ;-)

Ah, OK, I fixed that now! :-)

> > > OK to get designer input, but should probably go away before pushing
> > 
> > That's your decision, of course. But it's pretty useful when doing UI changes
> > (in particular the transient event every second is useful).
> 
> Well, we should try to get real events in there anyway. I'm OK with leaving it
> in for now, but then I'm not a designer ...

I left it with the reason stated here

 http://git.gnome.org/browse/gnome-shell/commit/?h=datetime&id=bce2cf91648837d05b787ce0c6fca5df52d8baaa

  We are leaving in the fake event source so it's easy to debug the
  UI. This is helpful because the fake event source has a transient
  event that is being added/removed every five seconds.

Attached is a new patch with the diff to master (using the empty event source, not the fake one). Let me know if this is suitable to commit to master. Thanks.
Comment 32 William Jon McCann 2011-01-24 18:58:07 UTC
A few comments from a quick test of the branch.

Overall looks really good.  A few, mostly stylistic issues...

Ideally before landing:
 * The calendar (left) pane is too narrow for the height and looks unbalanced wrt to the right pane - also causes the calendar grid to be taller than wide (it should be wider than tall ideally)
 * We need a better vertical separator (see mockups)
 * The Settings item should not stretch across both panes
 * Increase vertical spacing between event section ("Today") headings and the event items (see mockup)
 * Hide the event pane by default or have a way to add events
 * Add Open Calendar item.
 * The hover background color of the calendar seems too bright.  Might be better to use the same hover color we use for menu items.
 * Drop the year from the calendar heading unless it is different from the current year.

After landing:
 * The left pane width changes when I change month
 * We currently don't show the day of week name in the drop down but that might be ok.
 * During the fade out after dismissal the weekend backgrounds appear to leave a residual and two distinct vertical stripes can be seen.  We should try to avoid that somehow.
 * Consider hiding the event pane when no event sources are available.
 * Fix selected item styling (the gradient thing) - I guess this is shell wide.
 * The time vs location stuff.
Comment 33 David Zeuthen (not reading bugmail) 2011-01-24 20:22:15 UTC
(In reply to comment #32)
>  * Hide the event pane by default

I'm not sure what you mean by this... do you mean that there needs to be a way to open the event pane? Also https://live.gnome.org/GnomeShell/Design/Whiteboards/DateNTime does not specify what to do when there are no events to show... Right now it just displays lots of nothing. Please clarify.

> or have a way to add events

Clearly this is what "Open Calendar" is for, no?
Comment 34 William Jon McCann 2011-01-24 23:05:22 UTC
(In reply to comment #33)
> (In reply to comment #32)
> >  * Hide the event pane by default
> 
> I'm not sure what you mean by this... do you mean that there needs to be a way
> to open the event pane? Also
> https://live.gnome.org/GnomeShell/Design/Whiteboards/DateNTime does not specify
> what to do when there are no events to show... Right now it just displays lots
> of nothing. Please clarify.

 "* Hide the event pane by default or have a way to add events"

Currently it doesn't seem to read events from evolution or anywhere and relies on a fake source.  We should either make it work or not show the event pane at all before landing.

Even if we make it work I think we probably want to say something like "No events" when there aren't any pending and offer an option to open the calendar.

Maybe I'm misunderstanding the current state - I just did a quick test.

> > or have a way to add events
> 
> Clearly this is what "Open Calendar" is for, no?

Yep.  It wasn't there when I tried it.
Comment 35 Matthias Clasen 2011-01-24 23:10:14 UTC
> Currently it doesn't seem to read events from evolution or anywhere and relies
> on a fake source.  We should either make it work or not show the event pane at
> all before landing.
>
> Even if we make it work I think we probably want to say something like "No
> events" when there aren't any pending and offer an option to open the calendar.

Saying "No events" if there are no events makes sense to me. Changing the entire UI before landing it just because something is not currently working (but will be, shortly) doesn't makes sense to me.
Comment 36 David Zeuthen (not reading bugmail) 2011-01-25 15:24:07 UTC
Hi again,

(In reply to comment #32)
> Ideally before landing:
>  * The calendar (left) pane is too narrow for the height and looks
> unbalanced wrt to the right pane - also causes the calendar grid
> to be taller than wide (it should be wider than tall ideally)

OK, I've changed this so it looks a lot more like the mockup now. Unfortunately to do this, I had to do a number of hacks since the PopupMenu + friends isn't as flexible as I need them to be....

>  * We need a better vertical separator (see mockups)

OK, this is done (thanks Colin for fixing bug 633477). There's CSS to set the width and the color but not the dash pattern (in its most complex form, this is expressed as an array of floating point numbers - right now it's [3,1]).

>  * The Settings item should not stretch across both panes

Fixed.

>  * Increase vertical spacing between event section ("Today") headings and the
> event items (see mockup)

Fixed.

>  * Hide the event pane by default or have a way to add events

Once we have a real event source the user can do this.

>  * Add Open Calendar item.

Done. (Just like we do for system settings, we probably want a dedicated desktop file for Evolution's calendar component. We also want to pass the currently selected date. I've left TODO items in the code so we won't forget.)

>  * The hover background color of the calendar seems too bright.  Might be
> better to use the same hover color we use for menu items.

Done.

>  * Drop the year from the calendar heading unless it is different from the
> current year.

The calendar heading (e.g. "Tuesday January 25, 2011") doesn't change... and I think it's useful to show the current year... but I can see what you are getting at.. so I've done this for the Month Switcher (e.g. "< January >" or "< March 2010 >") and the heading in the event list when displaying events from !today ("e.g. "January 27" or "March 2, 2010").

> After landing:
>  * The left pane width changes when I change month

This might be fixed.

>  * We currently don't show the day of week name in the drop down but that might
> be ok.

I've added this back. It could probably be removed since the clock is already showing the abbreviated day, e.g.

 Tue 10:20 AM
    ^
 +------------
 | Tuesday January 25, 2010 

>  * During the fade out after dismissal the weekend backgrounds appear
> to leave a residual and two distinct vertical stripes can be seen.
> We should try to avoid that somehow.

Yeah, not really sure how to prevent this...

>  * Consider hiding the event pane when no event sources are available.

I've gone ahead and show "Nothing Scheduled" if there are no events (or event sources).

Please try the datetime branch and let me know what you think. Thanks.
Comment 37 David Zeuthen (not reading bugmail) 2011-01-25 20:48:36 UTC
I've now implemented the Evolution event source (in the datetime branch). We will need to add e-d-s to gnome-shell's jhbuild for this to work. I've also maximized code-reuse by copying the bulk of the code from gnome-panel's clock applet.

Here are some screenshot with live events from my calendars

 http://people.freedesktop.org/~david/calendar-today.png
 http://people.freedesktop.org/~david/calendar-other-day.png
 http://people.freedesktop.org/~david/calendar-other-year.png

that illustrates the look+feel explained in the previous comments.
Comment 38 Bastien Nocera 2011-01-26 00:05:14 UTC
Just a quick note that the dates on the right hand-side (especially visible for the last screenshot) should be left-aligned (or even right-aligned), but not centred. Centring makes it harder to reader and parse (which is why "fancy" restaurants don't align prices, but centre them).

If the design mentions that it should be centred, then the design should be changed...
Comment 39 David Zeuthen (not reading bugmail) 2011-01-26 18:39:52 UTC
(In reply to comment #38)
> Just a quick note that the dates on the right hand-side (especially visible for
> the last screenshot) should be left-aligned (or even right-aligned), but not
> centred. Centring makes it harder to reader and parse (which is why "fancy"
> restaurants don't align prices, but centre them).
> 
> If the design mentions that it should be centred, then the design should be
> changed...

fixed in the datetime branch:

 http://people.freedesktop.org/~david/calendar-today-align-fixes.png
Comment 40 David Zeuthen (not reading bugmail) 2011-01-27 16:19:26 UTC
Created attachment 179441 [details] [review]
Updated diff from master

OK, here's an updated patch with the diff from master

Since comment 39, I've made the following changes

 - Collapse two-pixel borders inside the table
 - Align day name to "This week"
 - Fix width of calendar popup and ellipsis for events

See

 http://people.freedesktop.org/~david/calendar-table-collapse-hack.png

for an illustration.

Diffstat:

 configure.ac                           |    8 
 data/Makefile.am                       |    2 
 data/gs-applications.menu              |    4 
 data/theme/calendar-arrow-left.svg     |   82 +
 data/theme/calendar-arrow-right.svg    |   82 +
 data/theme/gnome-shell.css             |  173 ++
 js/Makefile.am                         |    1 
 js/ui/calendar.js                      |  591 +++++++-
 js/ui/dateMenu.js                      |  225 +++
 js/ui/main.js                          |    4 
 js/ui/panel.js                         |  126 -
 js/ui/workspace.js                     |    4 
 po/et.po                               |   23 
 po/nb.po                               |  165 +-
 src/Makefile-calendar-client.am        |   20 
 src/Makefile.am                        |    7 
 src/calendar-client/README             |    1 
 src/calendar-client/calendar-client.c  | 2169 +++++++++++++++++++++++++++++++
 src/calendar-client/calendar-client.h  |  149 ++
 src/calendar-client/calendar-debug.h   |   52 
 src/calendar-client/calendar-sources.c |  658 ++++++++++
 src/calendar-client/calendar-sources.h |   66 +
 src/shell-evolution-event-source.c     |  245 +++
 src/shell-evolution-event-source.h     |   45 
 24 files changed, 4589 insertions(+), 313 deletions(-)
Comment 41 Florian Müllner 2011-01-28 01:46:45 UTC
Review of attachment 179441 [details] [review]:

Wow - looks great and works really well. Still a bunch of comments, but nothing that requires brains to fix, so I'd expect this to land tomorrow. At this point it probably makes sense to attach a squashed patch from a rebased datetime branch rather than a diff.

::: configure.ac
@@ +69,3 @@
+LIBECAL_REQUIRED=1.6.0
+LIBEDATASERVER_REQUIRED=1.2.0
+LIBEDATASERVERUI_REQUIRED=1.2.0

New dependencies should also be added to tools/build/gnome-shell-build-setup.sh (if it can be satisfied by common distros) or tools/build/gnome-shell.modules (if it needs building).

::: data/gs-applications.menu
@@ +81,3 @@
+					<Category>Documentation</Category>
+					<Not><Category>Core</Category></Not>
+				</Or>

Merge error - that part was removed in master.

::: data/theme/gnome-shell.css
@@ +709,3 @@
 }
 
 .calendar-change-month:hover {

Not needed anymore (it's now used for the label which is not reactive)

@@ +714,3 @@
 }
 
 .calendar-change-month:active {

Dito.

@@ +722,3 @@
+    width: 20px;
+    height: 20px;
+    background-image: url("calendar-arrow-left.svg");

I think it looks nicer with border-radius

@@ +726,3 @@
+.calendar-change-month-back:hover {
+    background: #999999;
+    background-image: url("calendar-arrow-left.svg");

Not needed (inherited from .calendar-change-month-back) if you use background-color instead of background.

@@ +736,3 @@
+    width: 20px;
+    height: 20px;
+    background-image: url("calendar-arrow-right.svg");

Same comments as .calendar-change-month-back

@@ +795,3 @@
+
+.calendar-nonwork-day {
+    background: #181818;

Using an opaque color here looks a bit weird when fading out the menu - how about
  background-color: rgba(128, 128, 128, .1)?

@@ +803,3 @@
+    background-gradient-direction: vertical;
+    background-gradient-start: #3c3c3c;
+    background-gradient-end: #131313;

Similar, but as the affected area is rather small I guess it's OK (but maybe check with Jon if he disagrees)

@@ +811,3 @@
+
+.calendar-day-with-events {
+    font-weight: bold;

Hmmm - it's rather subtle at this font size.

@@ +861,3 @@
+}
+
+.open-calendar {

Not used.

@@ +867,3 @@
+}
+
+.open-calendar:hover {

Dito.

::: js/ui/calendar.js
@@ +168,3 @@
+
+// First, an implementation with no events
+function EmptyEventSource() {

Still useful?

@@ +392,2 @@
         // Start off with the current date
+        this.selectedDate = new Date();

Do you expect this to be used from outside the calendar at some point? If not, it should probably made private.

@@ +430,1 @@
+        this._dateLabel = new St.Label({style_class: 'calendar-change-month'});

Hmmm, weird style class - how about 'calendar-date-label' or something?

@@ +448,3 @@
+            let customDayAbbrev = _getCalendarDayAbbreviation(iter.getDay());
+            let label = new St.Label({ text: customDayAbbrev });
+            label.style_class = 'calendar-day-base calendar-day-heading';

I prefer style_class in the constructor property list unless it's going to be modified. Just a personal preference though ...

@@ +541,3 @@
+            let iterStr = iter.toUTCString();
+            button.connect('clicked', Lang.bind(this, function() {
+                let newly_selectedDate = new Date(iterStr);

newlySelectedDate

@@ +548,3 @@
+            let hasEvents;
+            hasEvents = this._eventSource.hasEvents(iter);
+            styleClass = 'calendar-day-base calendar-day';

Shorter to do
let hasEvents = ...
let styleClass = ...

@@ +600,3 @@
+Signals.addSignalMethods(Calendar.prototype);
+
+function EventsList(eventSource) {

It's only used in dateMenu.js, so it would make sense to me to move it there.

::: js/ui/dateMenu.js
@@ +33,3 @@
+    let stippleWidth = 1.0;
+    let stippleColor = new Clutter.Color();
+    [found, stippleWidth] = themeNode.lookup_length('-stipple-width', false);

You are not using found, so you can use
let stippleWidth = themeNode.get_length('-stipple-width');

@@ +39,3 @@
+    cr.lineTo(x, height);
+    Clutter.cairo_set_source_color(cr, stippleColor);
+    cr.setDash([1, 3], 1); // Hard-code for now

I think hard-coding the dash is perfectly fine - as far as I know, "real" CSS only gives you the choice between "dotted", "dashed", "solid" and "double" ... If we supported the border-style property properly, you could use

#calendarArea > :first-child {
    border: 0px solid #505050;
    border-right: 1px dotted #505050;
},

but as we don't it's fine simulating it here with a fixed dash.

@@ +59,3 @@
+        //this._eventSource = new Calendar.FakeEventSource();
+        this._eventSource = new Calendar.EvolutionEventSource();
+        // TODO: write e.g. EvolutionEventSource

That's actually kind of funny :-)

@@ +90,3 @@
+
+        //item = new St.Button({style_class: 'popup-menu-item', label: 'foobar'});
+        //vbox.add(item);

Remove

@@ +93,3 @@
+        item = new PopupMenu.PopupSeparatorMenuItem();
+        item.actor.remove_actor(item._drawingArea);
+        vbox.add(item._drawingArea);

Gah - less hacky with:

item = new PopupMenu.PopupSeparatorMenuItem();
item.setColumnWidths(1);
vbox.add(item.actor);

@@ +115,3 @@
+        item = new PopupMenu.PopupMenuItem(_("Open Calendar"));
+        item.connect('activate', Lang.bind(this, this._onOpenCalendarActivate));
+        vbox.add(item.actor, {y_align : St.Align.END, expand : true, y_fill : false});

No space before colons

@@ +135,3 @@
+        // Add button to get to the Date and Time settings
+        //this.menu.addAction(_("Date and Time Settings"),
+        //                    Lang.bind(this, this._onPreferencesActivate));

Remove

::: js/ui/workspace.js
@@ -1329,3 @@
-                    Lang.bind(this, function() {
-                        this.positionWindows(0);
-                    }));

Another merge error.

::: po/et.po
@@ +15,3 @@
 "shell&component=general\n"
+"POT-Creation-Date: 2011-01-15 00:23+0000\n"
+"PO-Revision-Date: 2011-01-15 15:45+0200\n"

Dito.

::: po/nb.po
@@ +3,3 @@
 # This file is distributed under the same license as the gnome-shell package.
 #
+# Kjartan Maraas <kmaraas@gnome.org>, 2009-2010.

... and again!

::: src/Makefile-calendar-client.am
@@ +6,3 @@
+	calendar-client/calendar-debug.h						\
+	calendar-client/calendar-sources.c	calendar-client/calendar-sources.h	\
+	$(NULL)

Hmmm, you need
  EXTRA_DIST += calendar-client/README
or similar, right?
Comment 42 Florian Müllner 2011-01-28 12:58:56 UTC
Review of attachment 179441 [details] [review]:

Sorry for the noise, but I found another issue - it's a bit embarrassing, but I didn't notice until last night that the events (both in the calendar and the events list) are actually off by one day. I'm not entirely sure about the reason, so take the below with a grain of salt:

::: src/shell-evolution-event-source.c
@@ +161,3 @@
+
+  begin_date = g_date_time_new_from_unix_utc (msec_begin / 1000);
+  end_date = g_date_time_new_from_unix_utc (msec_end / 1000);

I suspect that this is not correct - as far as I can tell, Date.getTime() in Javascript returns local time, not UTC[0]. So I think you either need to use g_date_time_new_from_unix_local() here or translate to UTC in the Javascript code.

[0] http://www.techotopia.com/index.php/JavaScript_Date_Object
Comment 43 Owen Taylor 2011-01-28 14:43:35 UTC
> +  begin_date = g_date_time_new_from_unix_utc (msec_begin / 1000);
> +  end_date = g_date_time_new_from_unix_utc (msec_end / 1000);
> 
> I suspect that this is not correct - as far as I can tell, Date.getTime() in
> Javascript returns local time, not UTC[0]. So I think you either need to use
> g_date_time_new_from_unix_local() here or translate to UTC in the Javascript
> code.
> 
> [0] http://www.techotopia.com/index.php/JavaScript_Date_Object

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Date/getTime is explicit:

 The value returned by the getTime method is the number of milliseconds since 1 January 1970 00:00:00 UTC
Comment 44 Florian Müllner 2011-01-28 15:11:16 UTC
(In reply to comment #43)
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Date/getTime
> is explicit:
> 
>  The value returned by the getTime method is the number of milliseconds since 1
> January 1970 00:00:00 UTC

Hmmm, I can confirm that with
gjs -c "print(new Date().getTime())"; date +"%s"

Any other idea where things go wrong then?
Comment 45 David Zeuthen (not reading bugmail) 2011-01-28 17:06:31 UTC
(In reply to comment #41)
> ::: configure.ac
> @@ +69,3 @@
> +LIBECAL_REQUIRED=1.6.0
> +LIBEDATASERVER_REQUIRED=1.2.0
> +LIBEDATASERVERUI_REQUIRED=1.2.0
> 
> New dependencies should also be added to tools/build/gnome-shell-build-setup.sh
> (if it can be satisfied by common distros) or tools/build/gnome-shell.modules
> (if it needs building).

TODO.

> ::: data/gs-applications.menu
> @@ +81,3 @@
> +                    <Category>Documentation</Category>
> +                    <Not><Category>Core</Category></Not>
> +                </Or>
> 
> Merge error - that part was removed in master.

Fixed by merging master into the branch.

> ::: data/theme/gnome-shell.css
> @@ +709,3 @@
>  }
> 
>  .calendar-change-month:hover {
> 
> Not needed anymore (it's now used for the label which is not reactive)

Nuked.

> @@ +714,3 @@
>  }
> 
>  .calendar-change-month:active {
> 
> Dito.

Nuked.

> @@ +722,3 @@
> +    width: 20px;
> +    height: 20px;
> +    background-image: url("calendar-arrow-left.svg");
> 
> I think it looks nicer with border-radius

OK, I've added

     border-radius: 4px;

> @@ +726,3 @@
> +.calendar-change-month-back:hover {
> +    background: #999999;
> +    background-image: url("calendar-arrow-left.svg");
> 
> Not needed (inherited from .calendar-change-month-back) if you use
> background-color instead of background.
> 
> @@ +736,3 @@
> +    width: 20px;
> +    height: 20px;
> +    background-image: url("calendar-arrow-right.svg");
> 
> Same comments as .calendar-change-month-back

Nifty. Fixed.

> 
> @@ +795,3 @@
> +
> +.calendar-nonwork-day {
> +    background: #181818;
> 
> Using an opaque color here looks a bit weird when fading out the menu - how
> about
>   background-color: rgba(128, 128, 128, .1)?

It looks better. Changed.

> @@ +803,3 @@
> +    background-gradient-direction: vertical;
> +    background-gradient-start: #3c3c3c;
> +    background-gradient-end: #131313;
> 
> Similar, but as the affected area is rather small I guess it's OK (but maybe
> check with Jon if he disagrees)

I just copied this from .panel-button:active because the mockup called for the same look+feel. I guess .panel-button:active and .calendar-today should use a common base?

> 
> @@ +811,3 @@
> +
> +.calendar-day-with-events {
> +    font-weight: bold;
> 
> Hmmm - it's rather subtle at this font size.

Yeah. Maybe that's just an issue with the Cantarell font? Not sure how to fix it.

> @@ +861,3 @@
> +}
> +
> +.open-calendar {
> 
> Not used.
> 
> @@ +867,3 @@
> +}
> +
> +.open-calendar:hover {
> 
> Dito.

Nuked both.

> ::: js/ui/calendar.js
> @@ +168,3 @@
> +
> +// First, an implementation with no events
> +function EmptyEventSource() {
> 
> Still useful?

I think so since it kinda defines the interface.

> @@ +392,2 @@
>          // Start off with the current date
> +        this.selectedDate = new Date();
> 
> Do you expect this to be used from outside the calendar at some point? If not,
> it should probably made private.

It's probably better to just make it private. Done.

> @@ +430,1 @@
> +        this._dateLabel = new St.Label({style_class:
> 'calendar-change-month'});
> 
> Hmmm, weird style class - how about 'calendar-date-label' or something?

Historical reasons - the label used to be "< MONTH >" before we added real buttons. I've changed the style class to calendar-month-label and the variable name to _monthLabel.

> @@ +448,3 @@
> +            let customDayAbbrev = _getCalendarDayAbbreviation(iter.getDay());
> +            let label = new St.Label({ text: customDayAbbrev });
> +            label.style_class = 'calendar-day-base calendar-day-heading';
> 
> I prefer style_class in the constructor property list unless it's going to be
> modified. Just a personal preference though ...

Fixed.

> @@ +541,3 @@
> +            let iterStr = iter.toUTCString();
> +            button.connect('clicked', Lang.bind(this, function() {
> +                let newly_selectedDate = new Date(iterStr);
> 
> newlySelectedDate

I must have been asleep at the wheel! Fixed.

> @@ +548,3 @@
> +            let hasEvents;
> +            hasEvents = this._eventSource.hasEvents(iter);
> +            styleClass = 'calendar-day-base calendar-day';
> 
> Shorter to do
> let hasEvents = ...
> let styleClass = ...

C-isms! Fixed!

> @@ +600,3 @@
> +Signals.addSignalMethods(Calendar.prototype);
> +
> +function EventsList(eventSource) {
> 
> It's only used in dateMenu.js, so it would make sense to me to move it there.

But it shares code with Calendar (_sameDay) so I've left it there.

> ::: js/ui/dateMenu.js
> @@ +33,3 @@
> +    let stippleWidth = 1.0;
> +    let stippleColor = new Clutter.Color();
> +    [found, stippleWidth] = themeNode.lookup_length('-stipple-width', false);
> 
> You are not using found, so you can use
> let stippleWidth = themeNode.get_length('-stipple-width');

Fixed.

> @@ +39,3 @@
> +    cr.lineTo(x, height);
> +    Clutter.cairo_set_source_color(cr, stippleColor);
> +    cr.setDash([1, 3], 1); // Hard-code for now
> 
> I think hard-coding the dash is perfectly fine - as far as I know, "real" CSS
> only gives you the choice between "dotted", "dashed", "solid" and "double" ...
> If we supported the border-style property properly, you could use
> 
> #calendarArea > :first-child {
>     border: 0px solid #505050;
>     border-right: 1px dotted #505050;
> },
> 
> but as we don't it's fine simulating it here with a fixed dash.

Sounds good to me.

> @@ +59,3 @@
> +        //this._eventSource = new Calendar.FakeEventSource();
> +        this._eventSource = new Calendar.EvolutionEventSource();
> +        // TODO: write e.g. EvolutionEventSource
> 
> That's actually kind of funny :-)

Heh. Fixed!

> @@ +90,3 @@
> +
> +        //item = new St.Button({style_class: 'popup-menu-item', label:
> 'foobar'});
> +        //vbox.add(item);
> 
> Remove

Nuked.

> @@ +93,3 @@
> +        item = new PopupMenu.PopupSeparatorMenuItem();
> +        item.actor.remove_actor(item._drawingArea);
> +        vbox.add(item._drawingArea);
> 
> Gah - less hacky with:
> 
> item = new PopupMenu.PopupSeparatorMenuItem();
> item.setColumnWidths(1);
> vbox.add(item.actor);

Oh, OK, that works too. Fixed.

(I spent a lot of time making it work...)

> @@ +115,3 @@
> +        item = new PopupMenu.PopupMenuItem(_("Open Calendar"));
> +        item.connect('activate', Lang.bind(this,
> this._onOpenCalendarActivate));
> +        vbox.add(item.actor, {y_align : St.Align.END, expand : true, y_fill :
> false});
> 
> No space before colons

Fixed.

> @@ +135,3 @@
> +        // Add button to get to the Date and Time settings
> +        //this.menu.addAction(_("Date and Time Settings"),
> +        //                    Lang.bind(this, this._onPreferencesActivate));
> 
> Remove

Nuked.

> ::: js/ui/workspace.js
> @@ -1329,3 @@
> -                    Lang.bind(this, function() {
> -                        this.positionWindows(0);
> -                    }));
> 
> Another merge error.

Fixed by merging master into the branch.

> ::: po/et.po
> @@ +15,3 @@
>  "shell&component=general\n"
> +"POT-Creation-Date: 2011-01-15 00:23+0000\n"
> +"PO-Revision-Date: 2011-01-15 15:45+0200\n"
> 
> Dito.
> 
> ::: po/nb.po
> @@ +3,3 @@
>  # This file is distributed under the same license as the gnome-shell package.
>  #
> +# Kjartan Maraas <kmaraas@gnome.org>, 2009-2010.
> 
> ... and again!
> 

Fixed by merging master into the branch.

> ::: src/Makefile-calendar-client.am
> @@ +6,3 @@
> +    calendar-client/calendar-debug.h                        \
> +    calendar-client/calendar-sources.c    calendar-client/calendar-sources.h  
>  \
> +    $(NULL)
> 
> Hmmm, you need
>   EXTRA_DIST += calendar-client/README
> or similar, right?

Technically it's not needed in the distributed tarballs (it's only interesting in the context of a SCM) but I added it anyway.

The changes I did are here

 http://git.gnome.org/browse/gnome-shell/commit/?h=datetime&id=8a1313be71b1dab49188812aef124fc2148333a4
Comment 46 David Zeuthen (not reading bugmail) 2011-01-28 17:10:33 UTC
Created attachment 179529 [details] [review]
Updated diff from master

New patch:

$ diffstat shell-datemenu-20110128.patch
 configure.ac                           |    8 
 data/Makefile.am                       |    2 
 data/theme/calendar-arrow-left.svg     |   82 +
 data/theme/calendar-arrow-right.svg    |   82 +
 data/theme/gnome-shell.css             |  161 ++
 js/Makefile.am                         |    1 
 js/ui/calendar.js                      |  591 +++++++-
 js/ui/dateMenu.js                      |  212 +++
 js/ui/main.js                          |    4 
 js/ui/panel.js                         |  126 -
 src/Makefile-calendar-client.am        |   22 
 src/Makefile.am                        |    7 
 src/calendar-client/README             |    1 
 src/calendar-client/calendar-client.c  | 2169 +++++++++++++++++++++++++++++++
 src/calendar-client/calendar-client.h  |  149 ++
 src/calendar-client/calendar-debug.h   |   52 
 src/calendar-client/calendar-sources.c |  658 ++++++++++
 src/calendar-client/calendar-sources.h |   66 +
 src/shell-evolution-event-source.c     |  245 +++
 src/shell-evolution-event-source.h     |   45 
 20 files changed, 4472 insertions(+), 211 deletions(-)
Comment 47 David Zeuthen (not reading bugmail) 2011-01-28 17:12:20 UTC
(In reply to comment #41)
> +LIBECAL_REQUIRED=1.6.0
> +LIBEDATASERVER_REQUIRED=1.2.0
> +LIBEDATASERVERUI_REQUIRED=1.2.0
> 
> New dependencies should also be added to tools/build/gnome-shell-build-setup.sh
> (if it can be satisfied by common distros) or tools/build/gnome-shell.modules
> (if it needs building).

Since we are using exactly the same code as gnome-panel for interacting with e-d-s, the dependency can probably be satisfied by the distro. But I'm definitely not the best person to answer this so I didn't make any change here... Barnes?
Comment 48 Florian Müllner 2011-01-28 17:51:23 UTC
(In reply to comment #45)
> > Similar, but as the affected area is rather small I guess it's OK (but maybe
> > check with Jon if he disagrees)
> 
> I just copied this from .panel-button:active because the mockup called for the
> same look+feel. I guess .panel-button:active and .calendar-today should use a
> common base?

Ah, OK. As I said, due to being much smaller, the effect is a lot less intrusive (hardly noticeable even ...)

Not sure about a common base style though - avoiding duplication is certainly good, but I can't think quickly of a good name which would apply to both panel buttons and the current day marker (unless .panel-button makes sense for the latter too?).


> > @@ +811,3 @@
> > Hmmm - it's rather subtle at this font size.
> 
> Yeah. Maybe that's just an issue with the Cantarell font? Not sure how to fix
> it.

Neither am I, so let's leave it like that and let the designers figure out something if it turns out to be a problem.


> > ::: js/ui/calendar.js
> > @@ +168,3 @@
> > +
> > +// First, an implementation with no events
> > +function EmptyEventSource() {
> > 
> > Still useful?
> 
> I think so since it kinda defines the interface.

Ah, OK - a pattern we use for this is something like:


EventSource.prototype = {
    _init: function() {
       // do stuff?
    },

    hasEvents: function(day) {
        throw new Error('no implementation of hasEvents() in ' + this);
    }
};

The a "real" event source would inherit from the base class and overwrite the "abstract" interface functions with real implementations:

EvolutionEventSource.prototype = {
    __proto__: EventSource.prototype;

    _init: function() {
        EventSource.prototype._init.call(this);
    },

    hasEvents(day) {
        // real stuff
    }
}


Right now that seems like overkill, given that there aren't any other implementations, so I'd leave it for now (e.g. have EmptyEventSource and EvolutionEventSource as you do now).


> > It's only used in dateMenu.js, so it would make sense to me to move it there.
> 
> But it shares code with Calendar (_sameDay) so I've left it there.

OK, I overlooked the use of _sameDay there.


> Technically it's not needed in the distributed tarballs (it's only interesting
> in the context of a SCM) but I added it anyway.

It breaks distcheck ;-)
Comment 49 Florian Müllner 2011-01-28 17:58:16 UTC
(In reply to comment #47)
> Since we are using exactly the same code as gnome-panel for interacting with
> e-d-s, the dependency can probably be satisfied by the distro. But I'm
> definitely not the best person to answer this so I didn't make any change
> here... Barnes?

If the required version numbers are correct, I guess it can (for reference: libecal is 2.32.1 on F14 ...)

It's definitively preferable, so I'd say add it to the setup script - we can always add it to the moduleset later if people start complaining.
Comment 50 David Zeuthen (not reading bugmail) 2011-01-28 18:22:57 UTC
(In reply to comment #49)
> (In reply to comment #47)
> > Since we are using exactly the same code as gnome-panel for interacting with
> > e-d-s, the dependency can probably be satisfied by the distro. But I'm
> > definitely not the best person to answer this so I didn't make any change
> > here... Barnes?
> 
> If the required version numbers are correct, I guess it can (for reference:
> libecal is 2.32.1 on F14 ...)
> 
> It's definitively preferable, so I'd say add it to the setup script - we can
> always add it to the moduleset later if people start complaining.

OK, done with this commit

 http://git.gnome.org/browse/gnome-shell/commit/?h=datetime&id=57ebfb596bf061c3154fa6c2bbede32d8241885f
Comment 51 David Zeuthen (not reading bugmail) 2011-01-28 18:25:15 UTC
(In reply to comment #48)
> Ah, OK. As I said, due to being much smaller, the effect is a lot less
> intrusive (hardly noticeable even ...)
> 
> Not sure about a common base style though - avoiding duplication is certainly
> good, but I can't think quickly of a good name which would apply to both panel
> buttons and the current day marker (unless .panel-button makes sense for the
> latter too?).

Right. I'll just leave it as is for now.

> Neither am I, so let's leave it like that and let the designers figure out
> something if it turns out to be a problem.

Sounds good to me.

> Right now that seems like overkill, given that there aren't any other
> implementations, so I'd leave it for now (e.g. have EmptyEventSource and
> EvolutionEventSource as you do now).

OK. Will leave it for now.


> > But it shares code with Calendar (_sameDay) so I've left it there.
> 
> OK, I overlooked the use of _sameDay there.

OK.

> > Technically it's not needed in the distributed tarballs (it's only interesting
> > in the context of a SCM) but I added it anyway.
> 
> It breaks distcheck ;-)

OK, so good thing I added it :-)
Comment 52 David Zeuthen (not reading bugmail) 2011-01-28 18:34:11 UTC
Created attachment 179538 [details] [review]
Update diff from master
Comment 53 David Zeuthen (not reading bugmail) 2011-01-28 19:44:20 UTC
Was just stress-testing the calendar popup with my own personal calendar and ran into some bugs when there was a lot of all-day events.

I also simplified (and documented) the "This week" vs "Next week" handling. See

 http://git.gnome.org/browse/gnome-shell/commit/?h=datetime&id=e82cbf3cc880087c8341af78b31ae62642fb7e81
 http://people.freedesktop.org/~david/many-all-day-events.png

for details. Thanks!
Comment 54 David Zeuthen (not reading bugmail) 2011-01-28 21:38:08 UTC
Created attachment 179549 [details] [review]
Patch against master

Here's a patch against master.
Comment 55 David Zeuthen (not reading bugmail) 2011-01-28 21:48:18 UTC
Created attachment 179552 [details] [review]
Patch

Updated patch (with missing src/Makefile-calendar-client.am).
Comment 56 Florian Müllner 2011-01-28 23:31:01 UTC
Review of attachment 179552 [details] [review]:

Ready to go after fixing the two comment issues noted below. I'll leave it to you if you squash my "fix" (see comment) for the timezone issue or not. Also, the bug reference should be removed from the subject line (it's a bit overkill to mention it twice in a three line message).

::: js/ui/calendar.js
@@ +695,3 @@
+        if (dayEnd.getDay() <= 4) {
+            /* if now is Sunday through Thursday show "This week" and include events up until
+             * and including Saturday

As mentioned on IRC, some locales use Monday as week start, so this should be taken into account here (the calendar already does it). Probably best to file a separate bug about it after landing the patch though.

::: js/ui/dateMenu.js
@@ +101,3 @@
+
+        // Fill up the second column
+        //

Should be a blank line

::: src/calendar-client/calendar-client.c
@@ +235,3 @@
+/* Timezone code adapted from evolution/calendar/gui/calendar-config.c */
+/* The current timezone, e.g. "Europe/London". It may be NULL, in which case
+   you should assume UTC. */

So this seems to be the reason for the events-off-by-one-day issue - CalendarClient uses localtime, while the rest of the code assumes UTC. I came up with a patch, but it looks like it's not correct after all - CalendarClient does not use the user/system timezone, but gets it from gconf ... *grumble*

Not sure if squashing the patch with the assumption that the timezone used by Evolution corresponds to the system timezone is better than landing with the bug and coming up with a better fix later.

::: tools/build/gnome-shell-build-setup.sh
@@ +84,3 @@
     libgstreamer0.10-dev gstreamer0.10-plugins-base gstreamer0.10-plugins-good
     libltdl-dev libvorbis-dev libxklavier-dev libgnome-keyring-dev
+    libupower-glib-dev libcups2-dev evolution-data-server-dev

There's a generic list of dependencies (split between devel and other) in a comment above - e-d-s should be added there as well
Comment 57 Bastien Nocera 2011-01-29 01:40:53 UTC
(In reply to comment #56)
> Review of attachment 179552 [details] [review]:
> ::: js/ui/calendar.js
> @@ +695,3 @@
> +        if (dayEnd.getDay() <= 4) {
> +            /* if now is Sunday through Thursday show "This week" and include
> events up until
> +             * and including Saturday
> 
> As mentioned on IRC, some locales use Monday as week start, so this should be
> taken into account here (the calendar already does it). Probably best to file a
> separate bug about it after landing the patch though.

This is even a US-ism [1]. Pretty much no one uses Sunday as the first day of the week. Let me know when you've filed a separate bug and I'll add it to the GNOME 3.0 blockers.

[1]: and Canada-ism: http://en.wikipedia.org/wiki/Seven-day_week#Week_numbering
Comment 58 David Zeuthen (not reading bugmail) 2011-01-31 16:52:48 UTC
(In reply to comment #56)
> Review of attachment 179552 [details] [review]:
> 
> Ready to go after fixing the two comment issues noted below. I'll leave it to
> you if you squash my "fix" (see comment) for the timezone issue or not. Also,
> the bug reference should be removed from the subject line (it's a bit overkill
> to mention it twice in a three line message).

Thanks for the review!

> ::: js/ui/calendar.js
> @@ +695,3 @@
> +        if (dayEnd.getDay() <= 4) {
> +            /* if now is Sunday through Thursday show "This week" and include
> events up until
> +             * and including Saturday
> 
> As mentioned on IRC, some locales use Monday as week start, so this should be
> taken into account here (the calendar already does it). Probably best to file a
> separate bug about it after landing the patch though.

Will file bug.

> ::: js/ui/dateMenu.js
> @@ +101,3 @@
> +
> +        // Fill up the second column
> +        //
> 
> Should be a blank line

Fixed.

> ::: src/calendar-client/calendar-client.c
> @@ +235,3 @@
> +/* Timezone code adapted from evolution/calendar/gui/calendar-config.c */
> +/* The current timezone, e.g. "Europe/London". It may be NULL, in which case
> +   you should assume UTC. */
> 
> So this seems to be the reason for the events-off-by-one-day issue -
> CalendarClient uses localtime, while the rest of the code assumes UTC. I came
> up with a patch, but it looks like it's not correct after all - CalendarClient
> does not use the user/system timezone, but gets it from gconf ... *grumble*
> 
> Not sure if squashing the patch with the assumption that the timezone used by
> Evolution corresponds to the system timezone is better than landing with the
> bug and coming up with a better fix later.

Will apply your patch for now. Then I'll file a bug so we can look at this in detail.

> ::: tools/build/gnome-shell-build-setup.sh
> @@ +84,3 @@
>      libgstreamer0.10-dev gstreamer0.10-plugins-base gstreamer0.10-plugins-good
>      libltdl-dev libvorbis-dev libxklavier-dev libgnome-keyring-dev
> +    libupower-glib-dev libcups2-dev evolution-data-server-dev
> 
> There's a generic list of dependencies (split between devel and other) in a
> comment above - e-d-s should be added there as well

Done.
Comment 60 David Zeuthen (not reading bugmail) 2011-01-31 16:58:42 UTC
Filed bug 641048 for the UTC/localtime problem and bug 641049 for the first-day-of-week problem. Thanks.
Comment 61 David Zeuthen (not reading bugmail) 2011-01-31 17:36:04 UTC
Also, just filed bug 641057.