Created attachment 166318 [details] [review]
main patch (format-patch)

Created attachment 166319 [details] [review]
Makefile.am

Also see bug 344005.

Review of attachment 166318 [details] [review]:

In general this looks very reasonable. I obviously haven't checked the calendar system math. Do have some suggestions about coding style and structure here.

General style issues:

 * Code should have 4 space idents. If tabs characters are used, your editor must be set to 8-space tabs. (It appears that the code might have been created with a 4-space ta width.)
 * Operators (==, +, etc.) should have spaces around them
 * The calendar source files need to credit the sources you adapted from them. (This is not a legal requirement for public domain code, in the US. It is a legal requirement for GPL code. And it's the polite thing to do in any case.)

::: js/misc/calsys/gregorian.js
@@ +4,3 @@
+// & controls it's behaviour
+
+function controller(){

* Missing space between () and {

* constructors for objects should be named with an upper case initial letter
 
But looking at this, I think it would be be better to get rid of the controller() object and make the module itself the "object". So just have getDate(), setDate(), etc, functions directly in the module namespace. I don't think the stateless "controller" object adds anything.

So in calendar.js you'd replace

 this.controller = new CalSystems[this._calSys].controller();

With this:

 this._calendarSystem = CalendarSystems[calendarSystem];

@@ +10,3 @@
+controller.prototype = {
+	/**
+	 * sets date based on passed date object

sets "day of month"

::: js/misc/calsys/hebrew.js
@@ +223,3 @@
+		let h = dateToHebrew(d);
+		h[2] = 1;
+		// next year occures on 6th month (why?)

'occurs' (and again below)

@@ +225,3 @@
+		// next year occures on 6th month (why?)
+		if(h[1]==6){
+			++h[0]; // increase year

Use post-increment when it doesn't matter (and when it does matter, you probably should break the statement up)

@@ +276,3 @@
+	 */
+	getMonth : function(d){
+		return --dateToHebrew(d)[1];

Only use '--' when you want to modify a value. Here write '- 1'. (the above wouldn't even be legal in C!)

::: js/misc/calsys/hijri.js
@@ +175,3 @@
+	 */
+	getMonth : function(d){
+		return --dateToIslamic(d)[1];

Same comment

::: js/misc/calsys/jalali.js
@@ +247,3 @@
+		let gy = d.getFullYear();
+		let j = JalaliDate.gregorianToJalali(gy, gm+1, gd);
+		return format.replace(/%B/g, JalaliDate.j_month_names[j[1]-1]).replace(/%Y/g, this._english2persian(''+j[0]));

If you are going to chain like this, it's better to beak lines to show the structure

 return format.replace(/%B/g, JalaliDate.j_month_names[j[1]-1])
              .replace(/%Y/g, this._english2persian(''+j[0]));

With the '.replace' lined up

::: js/ui/calendar.js
@@ +8,3 @@
 const Shell = imports.gi.Shell;
 const Gettext_gtk20 = imports.gettext.domain('gtk20');
+const CalSystems = [

You have CalSstems here, and CalSys and CALENDAR_SYSTEM_KEY. Rather than have inconsistent abbreviations, it's better to just always put the full thing.

@@ +67,3 @@
+        this._calSys = 0;
+        try {
+			this._calSys = this._settings.get_int(CALENDAR_SYSTEM_KEY);

- You have get_int() here and get_integer() below.

- It would be better to identify calendar systems by strings rather than by arbitrary integers indices into an array.)

@@ +71,3 @@
+			this._settings.set_int(CALENDAR_SYSTEM_KEY, 0);
+		}
+		this.controller = new CalSystems[this._calSys].controller();

- Like other private fields, this should be _controller.
- I think this object is the "calendar system", and the _calSys field you have maybe shouldn't even be stored in the object at all. Controller is a very generic and unclear name to me.

Review of attachment 166319 [details] [review]:

No issues. Obviously shouldn't be separate in a final patch.

Created attachment 179624 [details] [review]
applied owen's comments

now it uses string instead of integer for setting.

i've tied to respect coding style of GJS but there maybe some mistakes :)

i should add 2 comments:

1.
const Shell = imports.gi.Shell;
const Gettext_gtk20 = imports.gettext.domain('gtk20');

is not necessary & none of them has been used in code

2. we don't need try/catch block. because in any case that fetching setting fails (for example setting doesn't exist) GIO won't throw error or at least it's not catchable in JS

I would like to add a switch to change calendar at runtime (if I read the patch correctly, calendar system is autodetected).

We Vietnamese use Western calendar most of the time of the year. But when our lunar new year comes, we turn to lunar calendar instead. I suppose Chinese share the same tradition. Japanese also use dual calendar system if I remember correctly.

Another way of displaying, used by paper calendars, is show both calendar at the same time. Lunar date is shown as a small number at the bottom right corner. When lunar month changes, show both month and date. We don't have to show every lunar date, just some important ones (customized by locales?). For Vietnamese, they are the first, 15th and the last day of each month, and first three days of January.

Owen: Can the patch in comment 6 get a review, please?

Is this feature going to be added on gnome 3.4? This is a cool feature for gnome-shell, while there's no built-in support for these calendars on Glib.

No, as we are past feature freeze. Maybe this get some reviews for 3.5.x but I highly recommend to ping again as people tend to forget...

Review of attachment 179624 [details] [review]:

I only reviewed Hebrew and Gregorian, but most of my comments apply to both. I don't understand the complex date code, so I didn't review it, but there's lots of formatting/code style issues.

I'm not sure I like the module-based API, though. It's an interesting idea, but I think I'm more comfortable with a class-based duck-typing API.

I will also say that the "Date" type in JS is just a wrapper around a timestamp that provides a basic Gregorian-style calendaring API based around that timestamp, which is a notoriously bad API for calendering. It probably makes sense not to use the JS Date object. There is a current Harmony Strawman to introduce a "globalization API", making note of date/time formatting and other things, and it looks like it has a calendaring API going forward:

"""
EXAMPLE
An implementation of DateTimeFormat might include the language tag "th" in its [[availableLocales]]
internal property, and must (according to 13.2.3) include the key "ca" in its [[relevantExtensionKeys]] internal property. For
Thai, the "buddhist" calendar is usually the default, but an implementation might also support the calendars "gregory",
"chinese", and "islamicc" for the locale "th". The [[localeData]] internal property would therefore at least include {"th": {ca:
["buddhist", "gregory", "chinese", "islamicc"]}}.
"""

    * http://wiki.ecmascript.org/doku.php?id=globalization:globalization
    * http://wiki.ecmascript.org/doku.php?id=globalization:specification_drafts

::: data/org.gnome.shell.gschema.xml.in
@@ +63,3 @@
+      <default>'gregorian'</default>
+      <summary>calendar system of calendar widget</summary>
+      <description>can be gregorian, jalali, hebrew, hijri</description>

That's not how you do an enum.

::: js/Makefile.am
@@ +11,3 @@
 	misc/telepathy.js	\
 	misc/util.js		\
+	misc/CalendarSystems/gregorian.js \

No. "calendarSystems" if anything.

::: js/misc/CalendarSystems/gregorian.js
@@ +15,3 @@
+ * 
+ * @author behrooz shabani <behrooz@rock.com>
+ * @copyright 2010-2011 behrooz shabani

We do not add GPL headers.

::: js/misc/CalendarSystems/hebrew.js
@@ +15,3 @@
+ * 
+ * @author behrooz shabani <behrooz@rock.com>
+ * @copyright 2010-2011 behrooz shabani

We do not add GPL headers.

@@ +18,3 @@
+ */
+
+// based on http://www.fourmilab.ch/documents/calendar/calendar.js

// Based on Fourmilab JavaScript Converter by John Walker:
// http://www.fourmilab.ch/documents/calendar/calendar.js

@@ +20,3 @@
+// based on http://www.fourmilab.ch/documents/calendar/calendar.js
+
+let HEBREW_EPOCH = 347995.5;

const HEBREW_EPOCH =

@@ +21,3 @@
+
+let HEBREW_EPOCH = 347995.5;
+let hebrew_month_names = [

const HEBREW_MONTH_NAMES =

@@ +37,3 @@
+];
+
+function hebrew_leap(year) {

We use camelCase, so hebrewLeap.

@@ +47,3 @@
+/**
+ * converts an integer to letter form of it
+ */

Remove the useless docstrings.

@@ +51,3 @@
+    let length = 0;
+    let h_number = "" ;
+    let digits = [

const HEBREW_DIGITS, and put at the top of the module.

@@ +53,3 @@
+    let digits = [
+        " \u05d0\u05d1\u05d2\u05d3\u05d4\u05d5\u05d6\u05d7\u05d8",
+        "\u05d8\u05d9\u05db\u05dc\u05de\u05e0\u05e1\u05e2\u05e4\u05e6",

Is the lack of space here intentional?

@@ +58,3 @@
+
+    /* sanity checks */
+    if (n < 1 || n > 10000)

No sanity checks.

@@ +219,3 @@
+                               (leap_gregorian(year) ? -1 : -2)
+           ) +
+           day);

This is a mess.

@@ +263,3 @@
+    let jd = hebrew_to_jd(year, month, day);
+    let gd = jd_to_gregorian(jd);
+    return new Date(gd[0], gd[1] - 1, gd[2], 0, 0, 0);

Please use destructuring assignment.

::: js/misc/CalendarSystems/jalali.js
@@ +18,3 @@
+ */
+
+// based on http://farhadi.ir/works/jalalijscalendar 

This is GPLv3. We cannot import GPLv3 into the Shell.

@@ +24,3 @@
+let JalaliDate = {};
+
+JalaliDate = {

Don't do this. Just define everything in module scope.

::: js/ui/calendar.js
@@ +71,3 @@
+            _calendarSystem = this._settings.get_string(CALENDAR_SYSTEM_KEY);
+        } catch(e) {
+            this._settings.set_string(CALENDAR_SYSTEM_KEY, _calendarSystem);

get_string should never throw an error.

@@ +74,3 @@
+        }
+        if ( !CalendarSystems[_calendarSystem] )
+            _calendarSystem = 'gregorian' ;

If you use a gsettings enum properly, this should never happen.

@@ +75,3 @@
+        if ( !CalendarSystems[_calendarSystem] )
+            _calendarSystem = 'gregorian' ;
+        this._calendarSystem = CalendarSystems[_calendarSystem] ;

No space after semi.

@@ +76,3 @@
+            _calendarSystem = 'gregorian' ;
+        this._calendarSystem = CalendarSystems[_calendarSystem] ;
+        this._settings.connect('changed::'+CALENDAR_SYSTEM_KEY, Lang.bind(this, this._calendarSystemChanged));

'changed::' + CALENDAR_SYSTEM_KEY

@@ +119,3 @@
     },
+    
+    // trigger for changing calendar system

Useless docstring.

@@ +123,3 @@
+        let _calendarSystem = this._settings.get_string(CALENDAR_SYSTEM_KEY);
+        if(CalendarSystems[_calendarSystem]){
+            this._calendarSystem = CalendarSystems[_calendarSystem] ;

Don't you need to update the UI or something here?

And just call this on first init instead of having the same logic in two places.

(In reply to Nguyen Thai Ngoc Duy from comment #8)
> I would like to add a switch to change calendar at runtime (if I read the
> patch correctly, calendar system is autodetected).
> 
> We Vietnamese use Western calendar most of the time of the year. But when
> our lunar new year comes, we turn to lunar calendar instead. I suppose
> Chinese share the same tradition.

I can't speak for all Chinese people, but I can hopefully describe how (some? most?) Chinese people in Hong Kong use these things.

Hong Kong people will look at both calendars all year long, depending on what they are looking for.

There, the Western calendar is the default for business, school, official matters, i.e almost everything. It should be the default, and probably should always be displayed.

However, for traditional things (like Chinese New Year which you mention), people will often look at the Lunar calendar.

People won't simply switch to the Lunar calendar temporarily as you describe though, they'll want to see either, or both at the same time.

Another example is birthdays. Some (generally older) people only known their birthday in the Lunar calendar. Their relative will want to look at the Lunar calendar to find what day (in the Western calendar) their birthday is this year.

So that's more of a « conversion » use case, which requires seeing both calendars at the same time.

> Another way of displaying, used by paper calendars, is show both calendar at
> the same time. Lunar date is shown as a small number at the bottom right
> corner. When lunar month changes, show both month and date.

This is what some other systems/applications do, and would be very useful, at least to Hong Kong Chinese people.

GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.