Use full weekday name in Day and Week views' captions

VERIFIED FIXED in 1.0b1

Status

--
trivial
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: rimas, Assigned: berend.cornelius09)

Tracking

unspecified
1.0b1

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Flags: wanted-calendar0.9?
(Reporter)

Updated

10 years ago
OS: Windows Vista → All
Hardware: PC → All

Comment 1

10 years ago
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.
Flags: wanted-calendar0.9? → wanted-calendar0.9+
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.
(Reporter)

Comment 3

10 years ago
Created attachment 331654 [details]
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?
(Reporter)

Comment 4

10 years ago
Hm, I've cut too much. Anyway, both Day View titles are identical to a day title in Week View.
Assignee: nobody → mschroeder

Comment 5

10 years ago
The week view might look a bit close then. Anyhow, Christian could you please comment on this?
Flags: wanted-calendar0.9+ → wanted-calendar1.0?
Assignee: mschroeder → nobody

Comment 6

10 years ago
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?
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.
(Reporter)

Comment 8

10 years ago
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

10 years ago
(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?
(Reporter)

Comment 10

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

Comment 11

10 years ago
(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
(Reporter)

Comment 12

10 years ago
(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.
(Assignee)

Comment 13

10 years ago
Created attachment 348541 [details] [diff] [review]
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.
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Assignee: Berend.Cornelius → nobody
Status: ASSIGNED → NEW
Keywords: qawanted
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED

Comment 14

10 years ago
Created attachment 348546 [details]
day name in rotated view

Comment 15

10 years ago
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)
(Assignee)

Comment 16

10 years ago
Created attachment 350375 [details] [diff] [review]
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.
(Assignee)

Comment 17

10 years ago
Created attachment 350377 [details] [diff] [review]
patch v. #3

Forgot to add a new source file to the project, before creating the patch...
Attachment #348541 - Attachment is obsolete: true
Attachment #350375 - Attachment is obsolete: true
(Assignee)

Comment 18

10 years ago
Created attachment 350442 [details] [diff] [review]
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.
Attachment #350377 - Attachment is obsolete: true
Attachment #350442 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #350442 - Flags: review? → review?(daniel.boelzle)
(Assignee)

Comment 19

10 years ago
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 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.
Attachment #350442 - Flags: review?(philipp)
Attachment #350442 - Flags: review?(daniel.boelzle)
Attachment #350442 - Flags: review+
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?
(Assignee)

Comment 22

10 years ago
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.
(Assignee)

Comment 23

10 years ago
Created attachment 351864 [details] [diff] [review]
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.
Attachment #350442 - Attachment is obsolete: true
Attachment #351864 - Flags: review?(philipp)
Attachment #350442 - Flags: review?(philipp)
(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 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.
Attachment #351864 - Flags: review?(philipp) → review+
(Assignee)

Comment 26

10 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Target Milestone: --- → 1.0
Flags: wanted-calendar1.0?

Comment 27

10 years ago
Checked in lightning and sunbird build 20090106 -> VERIFIED.
Status: RESOLVED → VERIFIED
Keywords: qawanted

Comment 28

10 years ago
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.
(In reply to comment #28)
Decathlon, have (all) bugs been filed to clean up or fix the relevant CSS selectors/rules?

Comment 30

9 years ago
(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

9 years ago
(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.
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".
Target Milestone: 1.0 → 1.0b1
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".
You need to log in before you can comment on or make changes to this bug.