Closed Bug 387863 Opened 17 years ago Closed 16 years ago

Event Invitation by mail does not display END TIME of event

Categories

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: klint, Assigned: mcicogni)

References

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Lightning 20070711

The email containing an invitation with the Accept/Reject buttons does not display the END time of the event, but only the START time. So it is difficult to make a decision whether to accept or reject !

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; fr-FR; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.666 ID:2007060411

Reproducible: Always

Steps to Reproduce:
1.Receive an iTIP invitation (sent by Outlook or by Lightning)
2.Open the invitation mail

Actual Results:  
The "WHEN" part of the invitation block does not show the end time of the event

Expected Results:  
The "WHEN" part of the invitation block should show the end time of the event as well
I can confirm this behavior and can provide a proposed patch.

Can someone please assign this bug to me so that I can work on it? Thanks.
Reassigning bug per previous comment.
Assignee: nobody → mcicogni
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Changing the OS and HW to "All", since relevant code isn't platform specific.
OS: Windows XP → All
Hardware: PC → All
This patch adds the functionality to display the end date below the start date, in two different lines labeled "Start" and "End". Event length can usually be easily inferred.

This arrangement might not suit everyone's taste, since some are deemed to prefer it showed event duration instead, or to have just the end time alongside the start time, unless obviously the event ended on a different date and we'd need to output the whole date/time then.

I have experimented the various arrangements but I feel (unless a significant effort in terms of user preferences and related UI is made) this is the clearest output. Depending on feedback, we may choose to prettyprint in some other way.
Attachment #284207 - Flags: review?
Attachment #284208 - Flags: review?
Mauro, thanks for your patch. I have some general improvement suggestions:

1. Please provide a unified patch based on the instructions in 
   http://developer.mozilla.org/en/docs/Creating_a_patch
2. Please provide just one patch for all changes, not one patch per file that you 
   change. This make reviewing the patch far easier.
3. If you ask for review, you need to ask review from a person. Otherwise the 
   patch might fall through the cracks. The list of possible reviewers is here: 
   http://wiki.mozilla.org/Calendar:Module_Ownership
   For your patch you should probably ask ctalbert for review.
Attached patch Unified patch (obsolete) — Splinter Review
Simon, thanks for the clarification.

Here's the unified patch (I hope it fits the guidelines, it's my first time really contributing a patch).
Attachment #284207 - Attachment is obsolete: true
Attachment #284208 - Attachment is obsolete: true
Attachment #284225 - Flags: review?(ctalbert)
Attachment #284207 - Flags: review?
Attachment #284208 - Flags: review?
Also, I'd prefer not to see the seconds.  HH:MM is good, HH:MM:SS is unnecessarily confusing, IMO.  I realize that this behavior also existed before Mauro's patch.
It's just that what you see is the default behaviour of toLocaleString(), so that you see it formatted using your own choice of separators, number of digits, AM/PM vs. 24hr, etc.

I'll do a bit of research and see if there's a Good way to output locale-formatted datetime strings omitting the seconds, otherwise I'd just keep what we have.
Thanks, Mauro.  Note that there are no seconds in the calendar views or tooltips in the rest of Lightning, so maybe there is a good way.  :)
Hmmm, documentation says that toLocaleString() does not take any parameter, so that way we're stuck.

We could use toLocaleFormat(), that takes a strftime-like format string, but that way we'd lose the local ordering of dates, the choice of date/time separators, AM/PM format, and so on, unless we settled on One True Format for all languages, or tried to mimick toLocaleString behaviour.

The only obvious benefit would be to explicitly display timezone (which is something i do wonder about at times, having co-workers scattered more or less over the globe).

Unless we do a substantial bit of research on how things are done in the rest of Lightning, we'd be better off just leaving things as they are, IMHO.

Pete?
Instead of calling e.g. event.startDate.jsDate.toLocaleString I'd recommend to just use event.startDate and pass it to calIDateTimeFormatter::formatDateTime(). This is how the entire UI formats the dates and/or times according to the users preferences.
Stefan, thanks for the wonderful suggestion.

It turns out that calIDateTimeFormatter has all the needed magic: actually it boasts an interesting formatInterval() function I'm currently investigating, since it may do The Right Thing and display in a compact format the time interval taking care of the different cases (interval within a day, spanning different dates, and so on).

I should be able to post an improved patch here during the weekend.
Attached patch Better patchSplinter Review
I finally got around to using calIDateTimeFormatter, and the resulting output is much better.

Also, I added the display of the COMMENT section, if present, like in Outlook-generated replies.
Attachment #284225 - Attachment is obsolete: true
Attachment #286763 - Flags: review?(ctalbert)
Attachment #284225 - Flags: review?(ctalbert)
Flags: wanted-calendar0.8?
Attachment #286763 - Flags: review?(ctalbert) → review?(daniel.boelzle)
Comment on attachment 286763 [details] [diff] [review]
Better patch

Moving over review to Clint.
Attachment #286763 - Flags: review?(daniel.boelzle) → review?(ctalbert)
Setting wanted0.8- as the main Calendar developers will not devote any time to
this in the 0.8 timeframe. Patches are, of course, always welcome.
Flags: wanted-calendar0.8? → wanted-calendar0.8-
Hmmm, I already submitted a patch, and asked for review - but it's been sitting there for a while...
Clint, any estimate on when you might to reviewing the patch in this bug?
I'm going to try to get to this tonight.  I'm respinning my lightning build now.  Mauro, sorry for the delay.
Target Milestone: --- → 0.8
Comment on attachment 286763 [details] [diff] [review]
Better patch

This looks good.  My only issue here is that we probably want to localize the dash in this line:
>+        var dateString = startString.value + " - " + endString.value;

There are some languages, like Japanese, that would prefer a different character for that. For example a character that looks a lot like a dash means "one". In other localizations, people might prefer to use a word, like the word "to" in English.

r+ with that
Attachment #286763 - Flags: review?(ctalbert) → review+
Patch checked in for Mauro on HEAD and MOZILLA_1_8_BRANCH.
Marking FIXED per comment#22. Filed follow up for issues from comment#20 and comment#21 -> bug 413103.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Thanks for the checkin, guys.

I hope that a little later on I'll be able to take on the followup bug.
This most probably caused Bug 414897. See Bug 414897 Comment #2 for details.
Depends on: 414897
Component: Lightning Only → E-mail based Scheduling (iTIP/iMIP)
QA Contact: lightning → email-scheduling
Status: RESOLVED → VERIFIED
Flags: wanted-calendar0.8-
You need to log in before you can comment on or make changes to this bug.