Closed Bug 385183 Opened 13 years ago Closed 13 years ago

[Proto] Closing the event calls alert "Do you want to save changes"

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: chris.j.bugzilla, Assigned: michael.buettner)

Details

Attachments

(1 file, 1 obsolete file)

Agenda Tab, Doubel-Click on Today creates a new Event.
Well, ok, I don't want to create an event right now. I expected to go to the day view of the calendar (was in newsgroup mode).
So I close the event and get the alert "Do you want to save changes"
=> I have no changes. If a new event was not edited then simply clos and remove it.
Basically this works as designed, since we wanted to make the event dialog behave exactly like the mail compose dialog. Are you sure that we want to close the event dialog without confirmation in case it's a new event?
Assignee: nobody → michael.buettner
Component: Provider: WCAP → General
Hardware: PC → All
Summary: Proto Event: Closing the event calls alert "Do you want to save changes" → [Proto] Closing the event calls alert "Do you want to save changes"
I understand the reasoning, but I believe that Thunderbird does not prompt you if you open and immediately close a composition window.
Thats true. A click on the Write icon calls up the compose window. It closes without an alert if users close the window without editing. so, yes we should close the window with no alert.
QA Contact: wcap-provider → general
Attached patch patch v1 (obsolete) — Splinter Review
This patch addresses two distinct issues with the alert dialog coming up in case the dialog is about to be shut down.

First, I still detected changes when there were actually none. The most difficult one was that I'm asking the item if it has the "TRANSP" property. If it's not present I'm leaving it all alone and add this property to the item only if the user explicitly sets this option in the dialog. Otherwise it gets written back to the dialog unconditionally. Sad but true, there's still a major flaw in calItemBase::getProperty(), see [1]. In case we're asking an occurrence the request gets silently passed through to the series in case the occurrence doesn't have this property. Basically this prevents from directly asking the item at hand specific questions. I'm now using getUnproxiedProperty() in order to work around this problem. I had a lengthy discussion with Daniel yesterday whether or not this automatic "pass through" mechanism makes sense or not. We both agree that it should be removed, but I doubt that this will get done anytime soon.

Second, the dialog *always* brought up the alert in case this item has just been created. This is exactly modeled after the mail compose window. I'm probably using a different Thunderbird than you guys do, but mine asks me when I try to close the window after having opened a brand new mail. Anyway, I leave the final decision to Christian, so here we go. The patch doesn't handle new items in any special way. If the newly created item hasn't been modified the dialog closes without asking anything.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calItemBase.js#386
Attachment #275948 - Flags: ui-review?(christian.jansen)
Attachment #275948 - Flags: review?(daniel.boelzle)
Comment on attachment 275948 [details] [diff] [review]
patch v1

mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js	2007-08-09 10:36:15.546875000 +0200
>+    // compare old and new version of this item. we ask the item for its
>+    // representation as icalString in order to have some easily comparable
>+    // form we can work with.
>+    if (newItem.icalString.toLowerCase() ==
>+        oldItem.icalString.toLowerCase()) {
>+        return true;
Problem I see is that changes in case (e.g. title or description won't be detected). You told me that the primary reason are the mailto prefixes of attendees. I'd propose to normalize those in the calAttendee id setters, like

id = id.replace(/^mailto:/i, "MAILTO:");

r- for now.
Attachment #275948 - Flags: ui-review?(christian.jansen)
Attachment #275948 - Flags: ui-review-
Attachment #275948 - Flags: review?(daniel.boelzle)
Attachment #275948 - Flags: review-
Comment on attachment 275948 [details] [diff] [review]
patch v1

ui-r=christian. Works great. Thanks for the patch.
Attachment #275948 - Flags: ui-review- → ui-review+
Comment on attachment 275948 [details] [diff] [review]
patch v1

ui-r=christian. Works great. Thanks for the patch.
Attached patch patch v2Splinter Review
> I'd propose to normalize those in the calAttendee id setters, like
> id = id.replace(/^mailto:/i, "MAILTO:");
That's indeed a better strategy...

I also found another flaw in this patch when passing the organizer of the event to the 'invite attendee' dialog where I didn't check if the organizer exists before the attempt to clone the object...

Anyway, this patch should do the trick...
Attachment #275948 - Attachment is obsolete: true
Attachment #277891 - Flags: ui-review+
Attachment #277891 - Flags: review?(daniel.boelzle)
Comment on attachment 277891 [details] [diff] [review]
patch v2

r=dbo
Attachment #277891 - Flags: review?(daniel.boelzle) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → 0.7
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070829 Calendar/0.7pre and lightning 2007082905
Status: RESOLVED → VERIFIED
It seems odd to me to force "mailto" to upper case. RFC 1738 says that URL schemes are lowercase, and that programs interpreting them should treat upper/lowercase equivalently. Unfortunately Apple's CalendarServer doesn't do this, so mailto: works but MAILTO: does not. Granted that CalendarServer should behave better, but do we need to normalize this wrong only to have to correct it in special cases? Could we not just normalize to lowercase instead?
You need to log in before you can comment on or make changes to this bug.