298373 - Deleting a calendar should prompt to confirm and really delete, not instantly unregister

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Mike Shaver (:shaver emeritus)]

Deleting is desired, prompting here is righteous, etc.

Comment 1

[Adam Guthrie]

*** Bug 310486 has been marked as a duplicate of this bug. ***

Comment 2

[Mike Shaver (:shaver emeritus)]

Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.

Comment 3

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Given that Gecko still has unaddressed focus problems, I think this probably needs to block 0.1.

Comment 5

[Michael Büttner (no reviews TFN)]

Attachment: added confirmation dialog

added the confirmation dialog asking whether the calendar should be deleted or not.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

This UI is the way sb0.2 used to do it. And it always confused me. Delete which file, the local one or the one on the server?
I think a better UI would be:
1) a menuitem 'unsubscribe' which would  unsubscribe, but not delete the data.
   This item is only available for all ics and caldav calendars, but not for
   storage.
2) a menuitem 'delete' which would unsubscribe and delete data
   This item is only available for storage calendars, and ics files that can
   actually be deleted (those having an file:// path)

The second part of 2 might change.
Both items will have a confirmation dialog, ofcourse.

The advantage: 'delete' is know to really remove things, and that is what the item does. Unsubscribe isn't as destructive.

Comment 7

[Joey Minta]

Comment on attachment 211277 [details] [diff] [review]
added confirmation dialog

+
+   var gCalendarBundle = document.getElementById("bundle_calendar");
+
The g- prefix is generally reserved for global vars.  Also, I don't think lightning has any entities named "bundle_calendar".  You probably need to use the string bundle service.

+   if(buttonPressed == 0) // Delete calendar
+   { 
+        ltnRemoveCalendar(ltnSelectedCalendar());
+   }
+   else if(buttonPressed == 2) //delete calendar and file
+   { 
+        ltnRemoveCalendar(ltnSelectedCalendar());
+   } 
Given the mess we had before with 'Delete Calendar' vs 'Delete Calendar and File', and the general agreement to push handling the complexities of it until later, I think this might be better as "Really delete calendar?" [Yes] [No].  Also, since we're not doing anything different in either case, let's not confuse users further by offering them a choice we won't take into account.

It'd be great if you could make the same fixes work in Sunbird, since it should pretty much be copy+paste. http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendar.js#237

Some style nits:
-Use java-style if/else braces
if (con) {
  myfunc();
} else {
  myfunc2();
}

-Don't put spaces after parens in arguments.
foor("bar"); not foo( "bar" );

-Preserve 4-space indenting.

Comment 8

[Joey Minta]

Comment on attachment 211277 [details] [diff] [review]
added confirmation dialog

Ignore the comment about the bundle_calendar entity not existing.  I see the second file in the patch now.

Comment 9

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

reworked patch as discussed on irc.

Comment 10

[Joey Minta]

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

This looks a lot better, but I still have some nits:

+    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(); 
+    promptService = promptService.QueryInterface(Components.interfaces.nsIPromptService); 
+
You can do this in one call, by putting the interface in as an argument to getService()

+    var result = {value:0};
If you're not going to actually use result, then there's no point in initializing it as a variable.  Just pass in {} in its place.

+    if(ok)
+        ltnRemoveCalendar(ltnSelectedCalendar());
We're trying to get people to always use braces around if, since it makes it easier to add additional code later on.  Also, use a space after any control statements, ie.
if (foo) instead of if(foo)

+unregisterCalendarTitle=Unregister from Calendar
This sounds like awkward English to my ears.  You either 'Unregister Calendar' or 'Unsubscribe from Calendar'.  (Note, dmose says he prefers the 2nd option, so let's go with that.)

+unregisterCalendarMessage=Are you sure you want to unregister from this calendar?
+
It would be nice if you could format this string to use the calendar's name, just to help out the user.  In Sunbird, you have to use a context menu, which makes it pretty easy to miss the calendar you intended.  Something like http://lxr.mozilla.org/mozilla/source/calendar/base/src/calCalendarManager.js#402

Comment 11

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

updated patch as suggested.

Comment 12

[Joey Minta]

Comment on attachment 211728 [details] [diff] [review]
patch v3

+    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService); 
Should break that line off before the .getService, but I'll fix that before I check in.

r=jminta Thanks for all the work on this!

Comment 13

[Joey Minta]

Patch checked in.  The 'really delete' part is covered elsewhere (bug 296202).