GNOME Bugzilla – Bug 603532
Calendar has no calendar weeks
Last modified: 2010-05-30 20:36:05 UTC
Created attachment 148861 [details] [review] Patch adds calendar weeks to the gnome-shell calendar The current calendar implementation of gnome-shell is lacking calendar weeks. Attached patch will fix that.
Review of attachment 148861 [details] [review]: I generally like the idea, but there are some issues with your patch: 1. you posted a reverse patch 2. the patch does not apply, because gnome-shell.css is located in $(top_srcdir)/data/theme, not $(top_srcdir)/theme 3. the top line (< December 2009 >) is misaligned Additional comments: ::: gnome-shell/js/ui/calendar.js @@ -17,3 @@ -Date.prototype.getWeekOfYear = function () { - let startOfYear = new Date(this.getFullYear(), 0, 1); - let secondsInWeek = 604800000; These are milliseconds. Probably you should define a constant in the same way as MSECS_IN_DAY above. The indentation should be 4 spaces. @@ -178,3 @@ - if (iter.getDay() == 0) { - let button = new St.Label({ text: "" + iter.getWeekOfYear () }); - button.style_class = "calendar-day calendar-weekofyear"; I'd prefer iter.getWeekOfYear().toString() for consistency. 'button' is not a button, but a label - you should either rename the variable appropriately, or get rid of it altogether. You could use something like: this.actor.add(new St.Label({ text: ..., style_class: ... }), { row: row, col: ... });
Hey, thanks for the review. Will take care of the issues you mentioned.
Created attachment 148910 [details] [review] Updated patch Updated version of previous patch
Created attachment 148912 [details] [review] Updated patch, now including changes to CSS
Review of attachment 148912 [details] [review]: Thanks, looks mostly fine to me now. Still, the patch does not follow the style guide: 1. trailing white space (ll. 20 + 184) 2. wrong indentation in getWeekOfYear (should be 4 spaces) 3. wrong spacing of parenthesis (myFuntion() instead of myFunction ()) BTW, you might want to have a look at "git format-patch" for patch creation. ::: gnome-shell.orig/js/ui/calendar.js @@ +100,3 @@ + // fixed spacing for calendar week + this.actor.add( new St.Label({text: "\t"}), What exactly are you trying to achieve? In my opinion it looks perfectly fine without the tab - if you nevertheless want to add some additional spacing, you should add padding to the CSS rule instead of hardcoding some fixed width.
Withouth the tab, the calendar changes size if there are only single digit calendar weeks. I tried to fix that with CSS (width:), but it seems to have no impact. Btw; is it possible that Clutter has problems with unicode characters? I tried to replace the < and > arrows of the calendar with unicode characters which resulted in garbage.
(In reply to comment #6) > Withouth the tab, the calendar changes size if there are only single digit > calendar weeks. I tried to fix that with CSS (width:), but it seems to have no > impact. Ah OK, sorry I didn't get this. Then I agree with your approach of doing it in code - did you try prefixing a space for numbers < 10? Something along the lines of (iter.getWeekOYear() < 10 ? " " : "") + iter.getWeekOfYear() could do the job without introducing extra spacing. > Btw; is it possible that Clutter has problems with unicode characters? I tried > to replace the < and > arrows of the calendar with unicode characters > which resulted in garbage. I know that there are still problems with non-ASCII characters in the search entry, but there shouldn't be any problems on labels - just use the proper escape sequence.
Yes, I tried prepending a space, though that didn't have any visible impact.
Created attachment 148975 [details] [review] Patch updated according to review
That last patch doesn't contain the tab in the header row. I'll try to find a more elegant solution.
Review of attachment 148975 [details] [review]: Patch looks good to me except for some minor comments; I don't have an easy solution to the problem of width changing as you go through the months - the simplest approach is probably to connect to the connect to the 'style-changed' signal of StWidget and use st_theme_node_get_font(), clutter_actor_get_pango_context(), pango_context_get_metrics(), pango_font_metrics_get_approximate_digit_width() to compute a new width to pass to clutter_actor_set_width(). The question here is whether we actually want to unconditionally enable the week numbers, or whether it should be controlled: A) By the locale (presumably by the magic-translated-string approach) B) By a preference (we don't have any now, but clearly we are going to need *some* prefs for the calendar) While week-number use is common in some countries, in other countries they are never seen and the vast majority of people will never even have heard of them. Having them always there thus does impair the legibility of the calendar to some extent, since the eye hasn't been trained to ignore numbers off to the side of a calendar as irrelevant data. ::: gnome-shell.orig/js/ui/calendar.js @@ +16,3 @@ } +Date.prototype.getWeekOfYear = function() { We consider the basic Javascript types sealed; just make this a local function that takes a Date as an argument. @@ +22,3 @@ +} + + Excess blank line @@ +176,3 @@ + if (iter.getDay() == 0) { + let label = new St.Label({ text: iter.getWeekOfYear().toString() }); + label.style_class = "calendar-day calendar-weekofyear"; YOu should include this in the properties instead of setting it separately. @@ +179,3 @@ + this.actor.add(label, + { row: row, col: 0, + x_fill: false, x_align: 0.5} ); ' })' not '} )'
Review of attachment 148975 [details] [review]: Oh, other comment - is the computation here right? It doesn't seem to determine the ISO week number as described in http://en.wikipedia.org/wiki/ISO_week_date
(In reply to comment #11) > The question here is whether we actually want to unconditionally enable the > week numbers, or whether it should be controlled: > > A) By the locale (presumably by the magic-translated-string approach) > B) By a preference (we don't have any now, but clearly we are going to need > *some* prefs for the calendar) I cast my vote for B) - while the usefulness of week-numbers is influenced by the locale, there are other factors which come into play - like employers' practices, workflow, and personal customs. This is also the approach with the current clock applet, which has a hidden preference for this. (In reply to comment #12) > Oh, other comment - is the computation here right? It doesn't seem to determine > the ISO week number as described in http://en.wikipedia.org/wiki/ISO_week_date Another possibility would be to use a format string of "%V" or "%U", depending on the locale ("magic-translated-string approach")
It seems that Owen is correct; the calculation is not yet totaly correct. Thanks for spotting that. I'll correct that.
Created attachment 149177 [details] [review] Updated patch, now with calendar week calculation according to ISO 8601 New patch with ISO 8601 calendar week: "[...] the first calendar week of a year is that one which includes the first Thursday of that year and [...] the last calendar week of a calendar year is the week immediately preceding the first calendar week of the next calendar year."
I have also written a patch for this - unfortunately my bugzilla-fu is so weak I didn't find this bug before submitting my own solution (in bug #610271). I have closed my bug, but I will attach my own solution here for inspiration. I will also attach a small test script I have used to verify the correctness of both solutions - and at least for the test cases I have come up with my solution for getting the week number is more correct. Perhaps we should join our two solutions?
Created attachment 154080 [details] [review] Adds themeable week numbers to calendar
Created attachment 154081 [details] Script for testing correctnes of week number calculation
*** Bug 610271 has been marked as a duplicate of this bug. ***
Should wait until we have the clock preferences landed and be done as a preference, so marking dependency.
(In reply to comment #20) > Should wait until we have the clock preferences landed and be done as a > preference, so marking dependency. Not sure whether we want the option exposed in the ui (iirc the gnome-panel clock doesn't expose the key in the preferences) or if adding a gconf key is enough - either way, the relevant parts of bug 600276 have landed, so work on either of the patches could be resumed.
This should be fixed for GNOME 2.32. No need for options. In case of options, on by default. I already had to smack some sense into vuntz when he removed it for gnome-panel (wasn't for long, of course)
Maxx; you said that you found cases where my patch calculates the wrong calendar week numbers? Can you give be a hint? The other remaining issue with my patch is there variable width. Will take care of that.
(In reply to comment #23) > The other remaining issue with my patch is there variable width. Will take care > of that. Don't forget the GConf part: (In reply to comment #22) > This should be fixed for GNOME 2.32. No need for options. In case of options, > on by default. I disagree, I think there is the need for an option (though hidden in gconf) - in some locales calendar weeks are so uncommon that it's getting rather distracting and confusing. The default should probably be locale dependent - and default to "true" for nl_NL I suppose ;) This should be more or less what gnome-panel's clock applet is doing (show_week_numbers gconf key with a locale-dependent default value) ...
(In reply to comment #24) > The default should probably be locale dependent - and default to "true" for > nl_NL I suppose ;) Seems I have to explain again. I work in an international company (not IT, container shipping). Everyone uses week numbers (us, customers, competitors, etc). This is not limited to this type of business. All calendars we receive from vendors (again, only a few are Dutch) have calendar weeks on them. Anyway, this is not related at all to nl_NL. Company isn't Dutch, it is the type of business. Anything which has a small link to shipping or production will use week numbers. E.g. even a store will use week numbers (delivery of goods). For many businesses week numbers are very, very common. Even if you might not have been exposed to this. In short: Hidden option to disable, fine. As long as the default is on (discoverability). PS: when I talk about week number, I mean this one: http://en.wikipedia.org/wiki/ISO_week_date, not any other (for international companies this is used worldwide, even in USA and so on).
(In reply to comment #25) > (In reply to comment #24) > > The default should probably be locale dependent - and default to "true" for > > nl_NL I suppose ;) > > Seems I have to explain again. > > I work in an international company (not IT, container shipping). Everyone uses > week numbers (us, customers, competitors, etc). This is not limited to this > type of business. All calendars we receive from vendors (again, only a few are > Dutch) have calendar weeks on them. Sure, week numbers are important to you and to lots of people. There are also billions of people in the world who have never heard of a week number in their life. This would include, for example, 95% of everybody in the US, but it would include many people even in countries where week numbers are commonly used in business. So, it's a trade-off - do we just suck it up and include the week numbers, and make the calendar a little uglier, a little harder to read, a little more confusing for the people who don't need week numbers (whether or not they've heard of them), or do we on the other hand, make another, smaller, but still quite large group of people have to go and find a preference and turn it on. [ Note that this is a little different from, say, a desk calendar, because the typographic options available in a small calendar are limited - we can't make the week numbers there but inconspicuous by using a small font ] An on-by-default preference is useless. The percentage of people who are going to look for an option to turn off some bit of clutter that they don't understand is tiny. They'll just live with it. Whether an off-by-default preference works depends on how discoverable and obvious the preference dialog is.. is it hidden in a right click or in a huge morass of preference dialogs in the control center, or is there an obvious way to get from the calendar/clock to related settings? As Florian said, locale-dependency for the default is possible, but it's going to be pretty hard to figure out what it should be, since there's a sliding scale of frequency - my understanding is that it's basically Scandinavia - common, Northern Europe, fairly common, rest of Europe, somewhat rare, rest of world, quite rare. With exceptions in every case. So, do we really want to ask translators "translate this as <...> if more than 25% of the business users in your locale will want quick access to the week number"?
Raphael: Look at the attachments I made. One is a small test script for checking the correctness of week number calculation in your implementation and mine. The other is my attempt at a patch that does the same as yours (I made it before I discovered this bug). I haven't looked that closely at the UI part of your patch but I assume our solutions will be pretty much the same. In my patch both the week numbers and the day names are themeable to make them look the same. I spent some time looking at your method getCalendarWeekForDate and came to the conclusion that we didn't agree on the week numbers for many dates. So I made the small test script to try out some corner cases for both our methods. My method is based on the description here: http://en.wikipedia.org/wiki/ISO_week_date#Calculating_the_week_number_of_a_given_date
I'm not sure that a locale-dependent setting is the way to go; its not discoverable and its going to be a nightmare figuring out what setting to use with a particular locale. I'll implement it off-by-default and add a GConf key. People used to calendar weeks will have to dig into the configuration but this minimizes interface clutter cost. Maxx; thanks for the hint! I'll dig into it.
Created attachment 161612 [details] [review] Updated patch, now with gconf-setting and correct header size This patch adds week dates to the gnome-shell calendar. Disabled by default it can be enabled through gconf (/desktop/gnome/shell/calendar/enable_weekdate)
Review of attachment 161612 [details] [review]: Patch is mismerged with head (note the revert of the changes from " to '). Can you fix that and repost a new version? Thanks!
Created attachment 161969 [details] [review] Updated patch Updated patch, now hopefully against head. Sorry for the inconvenience; I'm still learning..
Review of attachment 161969 [details] [review]: First of all: you should use git-format-patch instead of git-diff. The former will make sure that the patch is correctly attributed to you and, more importantly, will contain a commit message :) Except for the comment about the conditional signal connection without disconnect, issues are pretty minor. Oh, and by default git screams bloody murder on trailing whitespace ... ::: data/gnome-shell.schemas @@ +441,3 @@ + <!-- Calendar --> + <schema> + <key>/schemas/desktop/gnome/shell/calendar/enable_weekdate</key> enable_weekdate sounds a little peculiar - I'd prefer show_weekdate (gnome-panel uses show_week_numbers here). @@ +449,3 @@ + <short>Show the week date in the calendar</short> + <long> + Determines whether to or not to display the ISO week date in the calendar. Just a suggestion: "If true, display the ISO week date in the calendar." sounds simpler and is in line with the descriptions of other boolean settings. ::: js/ui/calendar.js @@ +8,3 @@ const Gettext_gtk20 = imports.gettext.domain('gtk20'); +const GConf = Shell.GConf.get_default() Apart from the missing semicolon: CamelCase should be reserved for type names / namespaces. GConf is neither, it is a global object - you should either rename it as gconf or (better) not use a global at all. Shell.GConf.get_default() is cheap, but if you want to avoid redundant calls anyway, use something along the lines of this._gconf = Shell.GConf.get_default(); in _init @@ +14,3 @@ +const WEEKDATE_HEADER_WIDTH_MULTIPLICATOR = 3; + +const GCONF_ENABLE_WEEKDATE_KEY = "calendar/enable_weekdate"; Double quotes are used to mark translatable strings - you must use single quotes here. @@ +31,3 @@ +} + +function _getDigitWidth(actor){ Any particular reason that this is not a private method of Calendar? Just wondering. @@ +51,3 @@ this._weekStart = NaN; + this._weekdate = NaN; + this._digitWidth != NaN Typo, right? @@ +132,3 @@ + + if (this._useWeekdate) { + this.actor.connect('realize', Lang.bind(this, this._onStyleChange)); (1) if the signal's name is 'realize', the handler should be called _onRealize or similar (2) currently it's not possible to change the theme on-the-fly, but if it was you wouldn't catch changes here when this.actor is already realized; the signal you are looking for is 'style-changed' ... (3) you may very well connect more then once to the signal - you must either store the return value of connect() and disconnect in the else part (small modification, but not very pretty) or make the signal connection unconditionally in the constructor. The first two are easy fixes, for the third I'd suggest reorganizing the code along the lines of: In _init: this._weekdateHeader = null; this.actor.connect('style-changed', Lang.bind(this, this._onStyleChanged)); In _onStyleChanged: this._digitWidth = _getDigitWidth(this.actor); if (this._weekdateHeader) this._weekdateHeader.width = this._digitWidth * ...; In _buildHeader: if (this._useWeekdate) { this._weekdateHeader = new St.Label(); this.actor.add(this._weekdateHeader, ...); if (this._digitWidth) this._weekdateHeader.width = ...; } else { this._weekdateHeader = null; } @@ +142,3 @@ this.actor.add(new St.Label({ text: iter.toLocaleFormat('%a') }), { row: 1, + col: this._useWeekdate + (7 + iter.getDay() - this._weekStart) % 7, You should not rely on boolean true equaling numeric 1. @@ +158,3 @@ + _setWeekdateHeader: function() { + let weekdateHeader = this._weekdateHeader || new St.Label(); Cleaner to do: if (this._weekdateHeader == null) this._weekdateHeader = new St.Label(); this.actor.add() should probably go into the conditional - note that there is no problem in adjusting the width of weekdateHeader after adding it to the table. Note that with the code reorganization suggested above, this function wouldn't be needed any longer. @@ +205,3 @@ + let children = this.actor.get_children(); + for (let i = 0; i < children.length; i++) + children[i].destroy(); this.actor.destroy_children(); Not sure that's something I'd expect in the signal handler, _buildHeader might be a more natural place to empty the container ... @@ +239,3 @@ label.style_class = 'calendar-day'; this.actor.add(label, + { row: row, col: this._useWeekdate + (7 + iter.getDay() - this._weekStart) % 7, See above.
Created attachment 162013 [details] [review] Latest patch updated according to Florian's comments (#32). Thanks Florian for that thorough review. I hope I've addressed all you points in this new patch. Also thanks for the hint with git-format-patch. ;) I didn't make _getDigitWidth a member function because it is only a helper function and doesn't manipulate any calendar data. Then again, this would also be true for _getWeekdateHeaderOffset...
(In reply to comment #33) > I didn't make _getDigitWidth a member function because it is only a helper > function and doesn't manipulate any calendar data. On the other hand, it's only use is to have correct spacing in the calendar :) It's fine to leave it where it is though ... review follows.
Review of attachment 162013 [details] [review]: Much better - mostly style comments left. In regard to the patch format - commit message should follow this format: "One-line subject Body with the reason for the change and a more detailed description if necessary." Your current message covers the subject part, but you should provide a body as well. ::: js/ui/calendar.js @@ +10,3 @@ const MSECS_IN_DAY = 24 * 60 * 60 * 1000; +const MSECS_IN_WEEK = MSECS_IN_DAY * 7; +const WEEKDATE_HEADER_WIDTH_MULTIPLICATOR = 3; OK, I didn't mention this in the previous review (mostly because I don't feel like having a good suggestion): The constant name is extremely long, and at the same time does not really convey its purpose. Maybe something like WEEKDATE_DIGITS_NUM? The code would become this._digitsWidth * WEEKDATE_DIGITS_NUM which is a little more expressive than someNumber * SOME_NUMBER_FACTOR ... @@ +11,3 @@ +const MSECS_IN_WEEK = MSECS_IN_DAY * 7; +const WEEKDATE_HEADER_WIDTH_MULTIPLICATOR = 3; +const GCONF_ENABLE_WEEKDATE_KEY = 'calendar/show_weekdate'; s/ENABLE/SHOW/ - also note that the corresponding constants in other places do not have a GCONF_ prefix @@ +146,3 @@ this.actor.add(new St.Label({ text: iter.toLocaleFormat('%a') }), { row: 1, + col: this._getWeekdateHeaderOffset() + (7 + iter.getDay() - this._weekStart) % 7, Not sure if _getWeekdateHeaderOffset() is preferable to the explicit code - it's not like it's avoiding duplication of complex code (the rest of the column calculation, which is way more complex, is duplicated below) I think something like let colOffset = this._useWeekdate ? 1 : 0; this.actor.add(..., { col: colOffset + ... }); is slightly more obvious. (Side note: _getWeekdateHeaderOffset is a little misleading - the name implies that it returns the offset of the weekdate header, but that's not what the function does: it returns the offset for the "normal" date columns, or the number of columns used for displaying the weekdate) @@ +158,3 @@ + let children = this.actor.get_children(); + for (let i = 0; i < children.length; i++) + children[i].destroy(); Again: this function already exists. It is called st_container_destroy_children, so instead of this._cleanup(); you can just do this.actor.destroy_children();
Created attachment 162030 [details] [review] Latest patch updated according to Florian's comments (#35) The name of the header width constant is probably still too long, but at least it's a bit more clear now. I somehow overlooked actor.destroy_children(), that's fixed now. The other things (GCONF_ENABLE_WEEKDATE_KEY, _getWeekdateHeaderOffset() and _cleanup()) have also been taken care of.
Review of attachment 162030 [details] [review]: Thanks for the quick update - I'm still not that convinced that the constant name is that great, but then any alternative I can think of right now feels equally lame, so ... You should use a more active commit message though (first person: "Add ISO week date to the calendar" - alternatively, you can add a topic in square brackets like "[calendar] Add ISO week date"). The body doesn't really mention the "why" (which is admittedly hard for a straightforward feature like this) - maybe something like "As many people rely on week dates for organization and timekeeping, provide an option to display the ISO week date in the calendar." I was just about to mark the patch ACN when I noticed a little glitch - when the setting changes while the calendar is popped out, the calendar updates as expected, but the popup's position stays the same (read: it's no longer centered). Not a show-stopper in my opinion and easily fixed in a consecutive patch, but you may want to update the patch for bonus points ;)
Created attachment 162058 [details] [review] Updated patch, fixes wrong placement after settings change Thanks for the go, Florian. I've update the patch to fix the wrong placement after a settings change, I'm not all too happy with the signal I used for that. A settings change is not really a 'style-change', although it results in a changed presentation of the calendar. If you can point out a better signal I'm happy to change that. There is another thing though; I'd like to add a shrink/expand animation when toggling week dates... hits on how to proceed are very welcome.
Please disregard that last patch: this.calendar.actor.connect('style-changed', Lang.bind(this, this._resetActorPlacement)); onActorStyleChanged instead of _resetActorPlacement. I'll resend an updated patch.
Review of attachment 162058 [details] [review]: Assuming that the only changes to the previous version are in panel.js, I didn't review the rest. Comments are pretty minor. I'd say that mentioning that week dates are controlled by an option is sufficient, you don't have to specify key/default value - especially as the latter is locale dependent :) ::: js/ui/panel.js @@ +1269,3 @@ affectsStruts: false }); this.actor.y = (panelActor.y + panelActor.height - this.actor.height); + this.calendar.actor.connect('style-changed', Lang.bind(this, this._resetActorPlacement)); 'notify::width' seems more appropriate @@ +1301,3 @@ + + _onActorStyleChanged: function() { + this._resetActorPlacement(); A signal handler with a single function call is pointless - just connect the signal directly to the function. @@ +1304,3 @@ + }, + + _resetActorPlacement: function() { I don't like the name - the function is placing the actor, not resetting it (to what?). Something like _centerPopup, _setHorizontalPosition, _updatePosition, ... makes more sense to me. @@ +1306,3 @@ + _resetActorPlacement: function() { + let panelActor = Main.panel.actor; + this.actor.x = Math.round(panelActor.x + (panelActor.width - this.actor.width) / 2); This is duplicated code from show(), so you should replace the code there with a call to this function.
Created attachment 162077 [details] [review] Updated patch, fixes wrong placement after settings change The calendar is now correctly placed after a settings change. I'd still like to use animations, any hints on how to achieve that are welcome. Thanks for the review, Florian. Changes in this patch: * remove duplicate code for horizontal popup placement -> _centerPopup() * using 'notify::width' instead of 'style-changed', don't emit 'style-changed' manually * call _centerPopup() for 'notify-width'
Review of attachment 162077 [details] [review]: (In reply to comment #42) > The calendar is now correctly placed after a settings change. I'd still like to > use animations, any hints on how to achieve that are welcome. I think we should commit this in its current state. Animations could be added later, though I'm not sure it's worth the effort. People either care about week dates or not, so in most cases the setting will be changed either once or not at all - and the popup may not be visible at all while the setting is changed. That wouldn't be a concern if animating the setting change would be easy and straightforward, but I'm afraid it's not ... You can't just animate the width change, as this will shift the whole content around - the correct animation IMO would: (1) When activating the setting: Tween the width of the week date column from 0 to _digitsWidth * WEEKDATE_HEADER_WIDTH_DIGITS, then fade in the column's content (2) When deactivating the setting: Fade out the content, then tween the column width to 0 It's probably easier to do if you restructure the code to not destroy and recreate the content on setting changes, but that itself is a pretty hefty change to your patch - and it will only get you half way there at best ... Personally I think that it's a very small improvement which requires way too much effort, so if I were you I'd pick something else to work on (assuming that you want to continue contributing *hint,hint*)
It was my intent to use the week date animations as a first exercise to get head-to-head with Tweener, but it sounds like an awful lot of work. I guess I start looking for another task/bug. gnome-love it is. ;) Thanks for the mentoring!
Attachment 162077 [details] committed as re3eaa699481b: Add ISO week dates to the calendar