329855 - Add support for default alarm settings for new events/tasks

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Nidheesh Dubey]

When events are entered the default alarm/notification is none. There is no property that can change this behaviour which makes it difficult to click on all events just to make them provide these alarms. Default should be to offer alarm, a few min before the event.

Comment 1

[Stefan Sitter [:ssitter]]

I'll working on this issue.

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: add support for default alarm settings

This patch adds the existing support for default alarm settings in Lightning too. The code to read the user preferences is unified in new method setDefaultAlarmValues. This method is then called after createEvent/createTodo.

This patch does not add UI to the Lightning preferences to set this values. I'll file a new bug on this. In the meanwhile you can set the values using Thunderbird Preferences -> Advanced -> General -> Config Editor.

Comment 3

[Joey Minta]

Comment on attachment 214589 [details] [diff] [review]
add support for default alarm settings

Overall, this patch looks pretty good.  I think that the following lines exhibit the biggest architectural problem though:
+                    // XXX We should use the selected day from view instead
+                    var now = new Date();

calendar-item-editing.js should not care whether it's embedded in Sunbird, Lightning, or FooBar2000.  Accordingly, there's no consistent way to get the selected day from the views.

Option 1: Have both Lightning and Sunbird implement a getSelectedDay() function.  While simpler, this seems somewhat fragile in my mind.

Option 2: Require callers to be obligated to pass in entryDates for tasks if an alarm is going to be set.  This has the downside or requiring lots of areas of code to be aware of preferences.

Option 3: Allow functions to register themselves to be called in createEventWithDialog.  This allows Sunbird and Lightning to maintain separate code-paths.  It also makes it easier for extension authors to make further modifications.  It does seem like overkill for this bug though.

In short, I'm not really sure what the best option is.  I could probably be persuaded to even leave the above code (with the comment below addressed).  Ideas welcome.

Regular review:
+    if (!aItem) {
+        return;
+    }
Seems you're being a bit paranoid here.  I think this can be removed.

+        } catch (ex) {
+            Components.utils.reportError(
+                "Failed to apply default alarm settings to event");
+        }
Paranoid here too.  mvl has more sympathy than i do for people who mess with their profiles and hence bork their preferences.  I guess this can stay, but you should at least append the exception to the reportError, so it's easier to debug.

+    }
+    
+    if (isToDo(aItem)) {
This should be an else or an else if

+                    // XXX We should use the selected day from view instead
+                    var now = new Date();
+                    aItem.entryDate = jsDateToDateTime(now);
Won't this get you the wrong timezone?  I'd suggest using now(), which will avoid the js-conversion and ought to give a better tz too.

This patch is quite solid overall, I just need to think about the task-entryDate thing a bit more.

Comment 4

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

> calendar-item-editing.js should not care whether it's embedded in Sunbird,
> Lightning, or FooBar2000.

I don't agree with that statement. We are trying to build an application, not a set of components. Some interaction and dependencies between the components are needed, and in my opinion that's not a problem.

> Option 1: Have both Lightning and Sunbird implement a getSelectedDay()
> function.  While simpler, this seems somewhat fragile in my mind.

This option looks good enough for me. If some other app wants to reuse the .js file, they can simply also add this function. I don't see how this is fragile.

Comment 5

[Stefan Sitter [:ssitter]]

Attachment: add support for default alarm settings, v2

Patch updated according to comment 3. Task-entryDate issue left open.

Comment 6

[Joey Minta]

Comment on attachment 214689 [details] [diff] [review]
add support for default alarm settings, v2

Alright, I'm sold. r=jminta for this part.  We need follow-up patches for the task-issue (I think I agree with mvl now about just defining sunbird and lighting specific functions with the same name that this code can rely on to always exist) and for actually exposing these prefs.

Comment 7

[Joey Minta]

Patch has bitrotted, think you can update it quickly?

File to patch: calendar/resources/content/calendarWindow.js
patching file calendar/resources/content/calendarWindow.js
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file calendar/resources/content/calendarWindow.js.rej
can't find file to patch at input line 245
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: mozilla/calendar/lightning/content/calendar-management.js
|===================================================================
|RCS file: /cvsroot/mozilla/calendar/lightning/content/calendar-management.js,v
|retrieving revision 1.40
|diff -u -9 -p -d -r1.40 calendar-management.js
|--- mozilla/calendar/lightning/content/calendar-management.js  5 Mar 2006 05:06:24 -0000       1.40
|+++ mozilla/calendar/lightning/content/calendar-management.js  10 Mar 2006 17:38:01 -0000
--------------------------
File to patch: calendar/lightning/content/calendar-management.js
patching file calendar/lightning/content/calendar-management.js
Hunk #1 FAILED at 185.
1 out of 1 hunk FAILED -- saving rejects to file calendar/lightning/content/calendar-management.js.rej

Comment 8

[Stefan Sitter [:ssitter]]

Attachment: patch v2, updated for check in

Comment 9

[Joey Minta]

(In reply to comment #8)
> Created an attachment (id=215922) [edit]
> patch v2, updated for check in
> 
Awesome, thanks, patch checked in.  We still need to address the task issue.  It's up to you whether you want to do that here or in another bug.

Comment 10

[Stefan Sitter [:ssitter]]

Filed follow up Bug 331323 (add preference UI) and Bug 331485 (task start date issue). Resolving this bug as 'Fixed'.