447683 - Use full weekday name in Day and Week views' captions

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

Currently, only Month and Multi-week views show full weekday names in calendar captions, meanwhile Day and Week views use abbreviation. This looks quite ugly in both cases, and I seriously doubt there's a good reason for that (at least in Day view).
Please consider using full weekday names in captions of Day and Week views.

Comment 1

[Simon Paquet [:sipaq]]

After discussion with Martin, this needs 

- an adjustment in the formatDateWithoutYear function of calDateTimeFormatter.js 
  (move from short month name format to long month name format)
- a corresponding change in the IDL description
- the removal of line 2155 of calendar-multiday-view.xml

This will transform the current string from "Mon, 4 Aug" to "Montag, 4. August" in a German Sunbird, which is great for consistency reasons.

Comment 2

[Stefan Sitter [:ssitter]]

Please keep in mind that the text should not be too long to allow displaying the week view on smaller screen resolutions like 800x600 or 1024x768.

Comment 3

[Rimas Kudelis [use needinfo to ping]]

Attachment: Screenshit :)

Actually, when I reported this bug, the calendar headers looked different from now (see screenshit). They used to be like this:

Jul 28
Mon

while now they are like this:

Mon, 28 Jul

Perhaps the weekday name could be moved to a separate line, again?

Comment 4

[Rimas Kudelis [use needinfo to ping]]

Hm, I've cut too much. Anyway, both Day View titles are identical to a day title in Week View.

Comment 5

[Daniel Boelzle [:dbo]]

The week view might look a bit close then. Anyhow, Christian could you please comment on this?

Comment 6

[Simon Paquet [:sipaq]]

Looking at this again, I think this should be set to WONTFIX.

The reason is, that this will make day boxes in week view too long for smaller screens, especially if you like to have a 7-day week view.

Take this week in the German language for example:

Montag, 13. Oktober | Dienstag, 14. Oktober | Mittwoch, 15. Oktober, Donnerstag, 16. Oktober | Freitag, 17. Oktober | Samstag, 18. Oktober | Sonntag, 19. Oktober

This may work on my 1280x1024 19-inch display, but will break even there, once I enable the today pane. Let us not forget, that many people are still working on display with 1024x768 pixels, especially in the notebook space.

Christian, what do you think?

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

If full names don't fit in the weekview, they still can be used in the dayview. Because of that, I disagree with a full wontfix. Maybe just narrow the scope of the bug a bit.

Comment 8

[Rimas Kudelis [use needinfo to ping]]

Also, please don't forget that my suggestion was to split this date presentation into two lines, like it was before (see screenshot). And I can't stress how much better (at least for my locale) it would look with two full words in two lines, than with two abbreviations in one. Nobody writes dates as "Pr Lie 28" in Lithuania. Never.

Comment 9

[Simon Paquet [:sipaq]]

(In reply to comment #7)
> If full names don't fit in the weekview, they still can be used in the dayview.
> Because of that, I disagree with a full wontfix. Maybe just narrow the scope of
> the bug a bit.

At least in my Lightning 0.9 installation, the full names are shown in the day view. Today I get "Sonntag, 19. Oktober 2008".

Rimas, can you confirm this, so we can at least narrow down the scope of this bug to the week view?

Comment 10

[Rimas Kudelis [use needinfo to ping]]

I can, if you give me links to localized Sunbird/Lightning builds from comm-central.

Comment 11

[Christian Jansen]

(In reply to comment #5)
> The week view might look a bit close then. Anyhow, Christian could you please
> comment on this?


Sure. For the week view I'd prefer a "flexible" solution which displays a short or long string. so if enough space is available a long string should be displayed.

The Day view should display the long version by default. I think it does not make sense to show a long string like stated in comment #6. This is too long.

I can imagine that the following would work:

"Sun, Sep 28"    -> for short
"Sunday, Sep 28" -> for long

or 

28 Sun
28 Sunday

Comment 12

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #9)
> (In reply to comment #7)
> > If full names don't fit in the weekview, they still can be used in the dayview.
> > Because of that, I disagree with a full wontfix. Maybe just narrow the scope of
> > the bug a bit.
> 
> At least in my Lightning 0.9 installation, the full names are shown in the day
> view. Today I get "Sonntag, 19. Oktober 2008".
> 
> Rimas, can you confirm this, so we can at least narrow down the scope of this
> bug to the week view?

It's not true, at least for Lithuanian locale.

Comment 13

[Berend Cornelius]

Attachment: patch v. #1

The behaviour that Christian described in comment #11 was -basically- already existing in the 0.8 version of Lightning but I think that the changes in 
Bug 422369 -  Make Calendar styles LTR (left-to-right) and RTL (right-to-left) agnostic corrupted this behaviour. The flaw of that patch was in the following code-lines:

>-parseInt(document.defaultView.getComputedStyle(longName, >null).getPropertyValue("margin-left")) +
>-parseInt(document.defaultView.getComputedStyle(longName, >null).getPropertyValue("margin-right"));
>+parseInt(document.defaultView.getComputedStyle(longName, >null).getPropertyValue("-moz-margin-start")) +
>+parseInt(document.defaultView.getComputedStyle(longName, >null).getPropertyValue("-moz-margin-end"));

(Btw. the patch was reviewed by me)
The patch attached should reimplement the behaviour that Christian suggested in comment #11. I added the same behaviour to the month-view. 
One general problem I addressed in this patch was:
If the user moves one of the splitters on the sides of the calendar-views there will no resize-event being triggered although the width of the calendar view is changed. The result was that the display of the event-boxes was not relayouted and they tended to overlap themselves in this case. The solution that I added in this patch was to trigger a relayout that also takes care that the the display of the day-labels is calculated according to comment #11.
With the patch I consolidated a lot of redundant code from the calendar-multiday-view.xml and the calendar-month-view.xml file to a new file "calendar-base-view.xml.
Before I am asking for review I would like to have this patch tested by volunteers.

Comment 14

[Andreas Treumann]

Attachment: day name in rotated view

Comment 15

[Andreas Treumann]

I checked Berends first patch:

- many warnings in the jsconsole

Warning: LongWeekdayTotalPixels: 10clientWidth: -16

- day names in rotated view overlaps calendar view (see screen shot)

Comment 16

[Berend Cornelius]

Attachment: patch v. #2

Overworked patch that addresses Andreas issues and (hopefully) adds some performance improvements. Before I am asking for review I would like to have it tested.

Comment 17

[Berend Cornelius]

Attachment: patch v. #3

Forgot to add a new source file to the project, before creating the patch...

Comment 18

[Berend Cornelius]

Attachment: patch v. #4

One change I introduced is that now only the currently displayed view is resized  when
- the window resizes (as done before)
- when the splitter to the left and right of the calendar views are moved by the user (as not been done before)
For this reason I (ab?)used a new XUL-broadcaster, that also fires "viewresize" events when the user switches to a new view:

>@@ -256,6 +256,7 @@ function showCalendarView(type, event) {
>     } else {
>         ltnShowCalendarView(type, event);
>     }
>+    onCalendarViewResize(event);
> }

I am not sure if this is the most elegant solution and the most logical spot to locate my call. At first I added a "DOMAttrModifiedListener to the view-deck but I found that it costed too much performance because triggered way too often.
I asked Andreas if he could detect any drawback from this behaviour that is supposed to improve the overall performance improvement and said he finds it Ok.

This where I actually fire the "resize-Event":

>+function onCalendarViewResize(aEvent) {
>+    if (aEvent && aEvent.attrName){
>+        if (aEvent.attrName != "state") {
>+            return;
>+        }
>+    }
>+    let event = document.createEvent('Events');
>+    event.initEvent(currentView().type + "viewresized", true, false);
>+    document.getElementById("calendarviewBroadcaster").dispatchEvent(event);
>+}


This may seem a bit unusual because I generate the event to be fired from the calendar view type which again is generated from the id of the decorated-calendar-view-pane. I found this was the best way to avoid redundant information but the drawback is  that the whole logic is now dependent on the "id" attribute. Normally I would not like this myself but on the other hand this is "typical XUL". I added some comments to explain this logic.

Comment 19

[Berend Cornelius]

I forgot to mention, that I left some debugging messages in the code, although put them in commments. With venkma being close to being useless I thought that I might need them again before I check in the patch. Then I will of course remove them.

Comment 20

[Daniel Boelzle [:dbo]]

Comment on attachment 350442 [details] [diff] [review]
patch v. #4

>diff --git a/calendar/base/content/calendar-base-view.xml b/calendar>+   - Portions created by the Initial Developer are Copyright (C) 2006
2008...

>+      <destructor><![CDATA[
>+        if (this.mCalendar) {
>+            this.mCalendar.removeObserver(this.mObserver);
>+        }
>+        var alarmService = Components.classes['@mozilla.org/calendar/alarm-
use let

>+        <setter><![CDATA[
>+          if (this.mCalendar)
>+              this.mCalendar.removeObserver (this.mObserver);
rmeove whitespace

>+          this.mCalendar = val;
>+          this.mCalendar.addObserver (this.mObserver);
dto.

>+        <setter><![CDATA[
>+          this.mShowCompleted = val;
>+          return val;
return this.mShowCompleted = val;

>+        <setter><![CDATA[
>+          this.mTimezone = val;
>+          return val;
dto.

>+      <method name="adjustWeekdayLength">
>+        <parameter name="forceShortName"/>
>+        <body><![CDATA[
>+          let useShortNames = false;
>+          let labeldayboxkids = this.labeldaybox.childNodes;
>+          if (!labeldayboxkids || !labeldayboxkids[0]) {
>+              useShortNames = true;
>+          } else if (forceShortName && forceShortName === true) {
any reason to strictly check against === true ?
why not just |forceShortName|? keep it simple, code readers might wonder what's special about the parameter...

>+              useShortNames = true;
>+      <method name="today">
>+        <body><![CDATA[
>+          let date = createDateTime();

How can we use calUtils.jsm' symbols here, i.e. cal.createDatetime()?
Maybe try to import the module in the constructor?
I'd like to get rid of calUtils.js long term.

b/calendar>-            headerbox.appendChild(hdr);
>-            hdr.index = i;
>+              var hdr = createXULElement("calendar-day-label");
use let

>-                         this, "anonid", "childbox");
>-          var propertyValue = childbox.boxObject.firstChild
>+          // get the width/height of the mdScrollBox scrollbar
>+          var mdScrollBox = document.getAnonymousElementByAttribute(
>+                         this, "anonid", "mdScrollBox");
>+          var propertyValue = mdScrollBox.boxObject.firstChild
dto.

>+          var mdScrollBox = document.getAnonymousElementByAttribute(this, "anonid", "mdScrollBox");
>+          var scrollBoxObject = mdScrollBox.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
dto

>+          var mdScrollBox = document.getAnonymousElementByAttribute(this, "anonid", "mdScrollBox");
>+          var scrollBoxObject = 


r1=dbo, asking Philipp for a second review since I am not sure that I foresee all consequences.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Its really hard for me to forsee any consequences from this patch since it includes a lot of consolidation aside from fixing the actual issue. I would appreciate two separate patches.

From looking through the code I did notice the DOMAttributeModified handler though. Isn't there any other way to fix this issue? The overflow code I wrote worked quite well at times, maybe you can recycle that instead?

Comment 22

[Berend Cornelius]

In reply to comment #21:
I understand that of course. This patch has slowly become much bigger than I had originally planned. I filed  Bug 467546 -  Consolidate calendar-multiday-view.xml and calendar-month-view.xml  that I extracted from the patch v. #4.

Comment 23

[Berend Cornelius]

Attachment: patch v. #5

This patch does no longer include all the consolidation stuff anymore. I have dealt with that in Bug 467546 -  Consolidate calendar-multiday-view.xml and calendar-month-view.xml, pushed to comm-central on December 7th.

>From looking through the code I did notice the DOMAttributeModified handler
>though. Isn't there any other way to fix this issue? The overflow code I wrote
>worked quite well at times, maybe you can recycle that instead?

I could not see any other chance to catch the moving of the splitters on the left and right hand side of the calendar view because this user interaction was not observed by the global resize handler - otherwise a different implementation would have been possible, indeed. In this case I did not find a "DOMAttrModified" listener too problematic because it was not fired too often -as I tested- simply because the splitter has only very few defined attributes.

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #23)
> I could not see any other chance to catch the moving of the splitters on the
> left and right hand side of the calendar view because this user interaction was
> not observed by the global resize handler - otherwise a different
> implementation would have been possible, indeed. In this case I did not find a
> "DOMAttrModified" listener too problematic because it was not fired too often
> -as I tested- simply because the splitter has only very few defined attributes.

I found an event that happens when a splitter is dragged, the "command" event. This should make the implementation a bit simpler.

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 351864 [details] [diff] [review]
patch v. #5

>+//          WARN("LongWeekdayTotalPixels: "  blabla + "clientWidth: " + document.getAnonymousElementByAttribute(this, "anonid", "mainbox").clientWidth + "; setting ShortName: " + useShortNames);
Please remove debug comments before checkin.

>+              this.longWeekdayPixels += getSummarizedStyleValues(this, ["border-left-width",
>+                                                                        "padding-left", "padding-right"]);
What about border-right-width?


>+function onCalendarViewResize(aEvent) {
>+    if (aEvent && aEvent.attrName){
>+        if (aEvent.attrName != "state") {
>+            return;extends="chrome://calendar/content/calendar-base-view.xml#calendar-base-view"
Remove stuff after return statement.

>+calendar-base-view {
>+  -moz-binding: url(chrome://calendar/content/calendar-base-view.xml#calendar-base-view);
> }
You don't need to define this unless you use <calendar-base-view> as a tag directly in a xul file.


r=philipp with comments above considered and use of command event instead of DOMAttributeModified.

Comment 26

[Berend Cornelius]

I addressed the comments of philipp and
pushed patch to comm-central:

http://hg.mozilla.org/comm-central/rev/fe713242a103

I changed the DOMAttrModified listener to "command" which was a good idea. 

->fixed.

Comment 27

[Andreas Treumann]

Checked in lightning and sunbird build 20090106 -> VERIFIED.

Comment 28

[Decathlon]

After this bug has been fixed, columns header bottom-border in month-view and multiweek-view has disappeared.
It seems that with the patch, calendar-month-view-column-header binding has been deleted from calendar-month-view.xml, but inside calendar-view.css file, the border-bottom property still belongs to a selector that refers to it.

The border reappears adding:
border-bottom: 1px solid #D2D2D2; 
to  labeldaybox-container selector in calendar-view.css

calendar-month-view-column-header-container should be another selector without css rule in calendar-month-view.xml file.

Comment 29

[Martin Schröder [:mschroeder]]

(In reply to comment #28)
Decathlon, have (all) bugs been filed to clean up or fix the relevant CSS selectors/rules?

Comment 30

[Decathlon]

(In reply to comment #29)
> (In reply to comment #28)
> Decathlon, have (all) bugs been filed to clean up or fix the relevant CSS
> selectors/rules?

bug about missing column headers border is bug 485891 (with a patch waiting for review).
As far as I know there isn't a bug about selectors that refer to deleted rules in this bug (if I'm right about it). Actually it's a minor issue because *it seems* there aren't other side effects. I'm going to file a new bug to have someone's confirmation.

Comment 31

[Decathlon]

(In reply to comment #30)

> As far as I know there isn't a bug about selectors that refer to deleted rules
> in this bug (if I'm right about it). Actually it's a minor issue because *it
> seems* there aren't other side effects. I'm going to file a new bug ...

... filed bug 493812.

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".

Comment 33

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".