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)
Calendar
E-mail based Scheduling (iTIP/iMIP)
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: klint, Assigned: mcicogni)
References
Details
Attachments
(1 file, 3 obsolete files)
5.89 KB,
patch
|
cmtalbert
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•17 years ago
|
||
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.
Comment 2•17 years ago
|
||
Reassigning bug per previous comment.
Assignee: nobody → mcicogni
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•17 years ago
|
||
Changing the OS and HW to "All", since relevant code isn't platform specific.
OS: Windows XP → All
Hardware: PC → All
Assignee | ||
Comment 4•17 years ago
|
||
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?
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #284208 -
Flags: review?
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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?
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
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.
Comment 10•17 years ago
|
||
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. :)
Assignee | ||
Comment 11•17 years ago
|
||
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?
Comment 12•17 years ago
|
||
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.
Assignee | ||
Comment 13•17 years ago
|
||
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.
Assignee | ||
Comment 14•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Flags: wanted-calendar0.8?
Assignee | ||
Updated•17 years ago
|
Attachment #286763 -
Flags: review?(ctalbert) → review?(daniel.boelzle)
Comment 15•17 years ago
|
||
Comment on attachment 286763 [details] [diff] [review] Better patch Moving over review to Clint.
Attachment #286763 -
Flags: review?(daniel.boelzle) → review?(ctalbert)
Comment 16•17 years ago
|
||
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-
Assignee | ||
Comment 17•17 years ago
|
||
Hmmm, I already submitted a patch, and asked for review - but it's been sitting there for a while...
Comment 18•17 years ago
|
||
Clint, any estimate on when you might to reviewing the patch in this bug?
Comment 19•17 years ago
|
||
I'm going to try to get to this tonight. I'm respinning my lightning build now. Mauro, sorry for the delay.
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → 0.8
Comment 20•16 years ago
|
||
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+
Comment 21•16 years ago
|
||
Currently we format the string via [start + " - " + end] in other locations too, e.g. in http://lxr.mozilla.org/mozilla/source/calendar/prototypes/wcap/calendar-invitations-list.xml#169 http://lxr.mozilla.org/mozilla/source/calendar/import-export/calHtmlExport.js#152 http://lxr.mozilla.org/mozilla/source/calendar/resources/content/mouseoverPreviews.js#394 http://lxr.mozilla.org/mozilla/source/calendar/lightning/content/agenda-tree.js#203 http://lxr.mozilla.org/mozilla/source/calendar/lightning/content/agenda-listbox.xml#80 http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-alarm-widget.xml#114 I suggest to file a followup bug to unify the string building and fix the intl issue.
Comment 22•16 years ago
|
||
Patch checked in for Mauro on HEAD and MOZILLA_1_8_BRANCH.
Comment 23•16 years ago
|
||
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
Assignee | ||
Comment 24•16 years ago
|
||
Thanks for the checkin, guys. I hope that a little later on I'll be able to take on the followup bug.
Comment 25•16 years ago
|
||
This most probably caused Bug 414897. See Bug 414897 Comment #2 for details.
Updated•16 years ago
|
Component: Lightning Only → E-mail based Scheduling (iTIP/iMIP)
QA Contact: lightning → email-scheduling
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
Flags: wanted-calendar0.8-
You need to log in
before you can comment on or make changes to this bug.
Description
•