396819 - Event Summary Dialog doesn't show important information to user

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Pete Riley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: 

This is a followup to Bug 361977.

The Event Summary dialog is missing too much important information for read-only calendars.  The user has no way to see most of the following things:

start time (the dialog shows the date but not the time)
end date & time (e.g. multidays)
category
calendar
reminder (can't see it, can't change it) (at least for ICS)
timezone
importance
privacy
organizer (maybe this isn't supported for ICS)

A similar problem exists with tasks.

Also, I'm not familiar with the Mac world, but in the Windows world it is expected behavior that read-only fields are grayed out.  The purpose of graying out fields is to tell the user that he can't type in those fields.  The user knows that he can select text and copy it to the clipboard because the cursor changes to a text cursor.  In any case, I think that the behavior should be consistent, but currently the attendees field is grayed out but the description field is not.  IMO both should be grayed out, the cursor should change to a text cursor, and we should be able to select/copy text.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Stefan Sitter [:ssitter]]

See existing Bug 395017 for the missing display of start time/end/due date/time.

Comment 2

[Pete Riley]

Also, I can't click on an attendee to send an email from the Event Summary dialog.

IMO it would be better to trash the Event Summary dialog and only have the main event dialog and to gray out the relevant fields.  Maybe add a text description that the event or calendar is readonly.

This would allow us to see all the details, to click on attendees, to set a reminder for the event, etc.  This would also avoid duplicated code and the user would not have to learn two different dialogs.

Comment 3

[Stefan Sitter [:ssitter]]

Not being able to see certain information definitely results in some kind of dataloss. Confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.8pre) Gecko/20071016 Sunbird/0.7.

Comment 4

[Stefan Sitter [:ssitter]]

Either this should be fixed to allow proper usage of read-only calendars within Sunbird/Lightning or this dialog should be removed and the normal event dialog should be extended to handle read-only events. I'd prefer the 2nd solution to avoid code duplication.

Comment 5

[Berend Cornelius]

Attachment: first experimental patch

I started to work at a patch only to get a feeling of what this is really about. 
I find that some of the proposed item-information could also be added to the taskview-detail window - so I see a lot of room for consolidation. Before we will go on with the implementation we should get feedback from Christian and see what he thinks wether we should keep the summary dialog or not. 
I would vote for keeping it. We could implement this in such a way that there is no code duplication.

Comment 6

[Berend Cornelius]

One point I forgot to mention is that we could also display the time-related item-info (startdate, startTime, enddate, endtime) by using formatInterval similar to the agenda-items in the "Soon" section instead of splitting up these information on four different labels.

Comment 7

[Berend Cornelius]

I added the time information with 
Bug 395017-Event & Task dialog for read-only calendars don't show start time and end/due date and time which I think was the most relevant topic of this issue.

Comment 8

[Berend Cornelius]

Attachment: patch v. #2

The patch attached adds the timezone information to the start-date. On the first sight it may look far exaggerated to set up a new binding for such a little feature, but - as I said already there is a lot of common logic in the task-view and in the summary dialog and we certainly do not always want to keep creating duplicate code when we change or add something in one of these two view elements. The patch attached therefor creates a binding that extends to xul:row  and I hope that this serves as a model for all the other rows that can be consolidated and used in various places of the application.

Comment 9

[Berend Cornelius]

Attachment: patch v. #2

The patch attached adds the timezone information to the start-date. On the first sight it may look far exaggerated to set up a new binding for such a little feature, but - as I said already there is a lot of common logic in the task-view and in the summary dialog and we certainly do not always want to keep creating duplicate code when we change or add something in one of these two view elements. The patch attached therefor creates a binding that extends to xul:row  and I hope that this serves as a model for all the other rows that can be consolidated and used in various places of the application.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 327812 [details] [diff] [review]
patch v. #2

>+    content/calendar/calendar-item-bindings.xml  (content/calendar-item-bindings.xml)
>+    content/calendar/calendar-menus.xml          (content/calendar-menus.xml)
I was just thinking, we have a lot of event dialog specific code that should eventually move to base/content/. What do you think about introducing a further folder to group code, i.e base/content/event-dialog/ ?

>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
encoding="UTF-8", remove blank line after processing instruction.

>+<bindings id="calendar-item-bindings"
>+ xmlns="http://www.mozilla.org/xbl"
>+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+ xmlns:xbl="http://www.mozilla.org/xbl">
indent xmlns to align under id=

>+  <binding id="item-date-row" xbl:inherits="mode" extends="xul:row">
the xbl:inherits attribute needs to be defined on <content>, not on the binding itself.

>+        this.mFormatter = Components.classes["@mozilla.org/calendar/datetime-formatter;1"]
>+                            .getService(Components.interfaces.calIDateTimeFormatter);
We use the formatter quite often. Maybe we should rather introduce a function in calUtils like getDateTimeFormatter() that is cached. This way we don't have to get the service every place we use it.


>+      <property name="Mode"
lowercase "mode" and all other properties.

>+              var date = this.mItem.startDate || this.mItem.entryDate;
calGetStartDateProp, same comment as in your other review about tasks without start date?


>+              var date = (this.mItem.dueDate || this.mItem.endDate || this.mItem.startDate);
calGetEndDateProp

>+          var hideLabels = date == null;
Add () around date == null) to emphasize.

>+              if (date.timezone.tzid != calendarDefaultTimezone().tzid) {
>+                  itemDateTimeLabel.value = label + ", " + date.timezone.tzid;
>+              } else {
>+                  itemDateTimeLabel.value = label;
Maybe this should be localizable? Possibly not all locales can handle just appending the tzid to the label with a comma.

>\ No newline at end of file
Can you make your editor add newlines to the end of files so this doesn't always show up in patches?


>+eventDetailsStartDate=Start: 
>+eventDetailsEndDate=End: 
>+taskDetailsStartDate=Start Date: 
>+taskDetailsEndDate=Due Date: 
These strings seem to have spaces at the end, please remove.

Comment 11

[Berend Cornelius]

>I was just thinking, we have a lot of event dialog specific code that should
>eventually move to base/content/. What do you think about introducing a further
>folder to group code, i.e base/content/event-dialog/ ?

I think this is a good idea. For this issue I think it's not so important because it's not really dialog-specific - the new introduced binding is meant to be used from everywhere.

patch checked in on trunk and MOZILLA_1_8_BRANCH and on trunk

->issue remains open

Comment 12

[Berend Cornelius]

I addressed all the other comments of philipp

Comment 13

[Berend Cornelius]

One thing that I also noticed is that the Status field is not always optimally displayed. E.g. when I am the organizer of an event with several attendees the status "confirmed" is not really what I want to read...

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

This checkin caused a tinderbox red due to view-navigation.png being mentioned in the jar.mn. Since I found this wasn't used anywhere I checked in a fix that removes this line.

Comment 15

[Berend Cornelius]

Thank you Philipp. Yes this image is only needed for another patch that I'm working at.

Comment 16

[Christian Jansen]

Comment on attachment 327812 [details] [diff] [review]
patch v. #2

Looks good to me. ui=christian

Comment 17

[Daniel Boelzle [:dbo]]

The remaining issues don't justify blocking-0.9.

Comment 18

[Martin Schröder [:mschroeder]]

Berend, do you agree to file separate bug reports for the remaining issues, i.e. missing details in the summary dialog, and close this bug report?

Comment 19

[Martin Schröder [:mschroeder]]

(In reply to comment #18)
> Berend, do you agree to file separate bug reports for the remaining issues,
> i.e. missing details in the summary dialog, and close this bug report?

I'm resolving this bug as FIXED now. New bug reports should be filed for missing details in the event/task summary dialog.

Comment 20

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100224 Calendar/1.0b2pre