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

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Christian Jansen]

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.

Comment 1

[Michael Büttner (no reviews TFN)]

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?

Comment 2

[Pete Riley]

I understand the reasoning, but I believe that Thunderbird does not prompt you if you open and immediately close a composition window.

Comment 3

[Christian Jansen]

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.

Comment 4

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

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

Comment 5

[Daniel Boelzle [:dbo]]

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.

Comment 6

[Christian Jansen]

Comment on attachment 275948 [details] [diff] [review]
patch v1

ui-r=christian. Works great. Thanks for the patch.

Comment 7

[Christian Jansen]

Comment on attachment 275948 [details] [diff] [review]
patch v1

ui-r=christian. Works great. Thanks for the patch.

Comment 8

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

> 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...

Comment 9

[Daniel Boelzle [:dbo]]

Comment on attachment 277891 [details] [diff] [review]
patch v2

r=dbo

Comment 10

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 11

[Omar Bajraszewski]

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

Comment 12

[Bruno Browning]

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?