351084 - [Proto] Task dialog: cannot set alarm for new task, existing or default alarm throws error

Status: VERIFIED FIXED

(Calendar :: Tasks, defect)

Description

[Damian Szczepanik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060901 Calendar/0.3a2+

when "default alarm setting for tasks" is on and I want to create new task default time = 0:00

Reproducible: Always

Steps to Reproduce:
1. create new profile
2. goto tools -> options -> alarms
3. switch on alarm setting for tasks
3. double click on task pane to create new task
   -> default date is set to 0:00
Actual Results:  
see bug 342150 comment 20

Expected Results:  
time = current time

related bug 342150 comment 17

Comment 1

[Raymond]

If you follow the above steps and set the time to a time in the future. And then go on to "save and close" the alarm will popup immediately.

If a snooze the alarm, the cursor will remain an hourglass.

If I then edit the alarm, the time will again be 0:00

I'm using lightning 0.7pre (build 2007073105) in thunderbird 2.0.0.5 on windows xp

Comment 2

[Stefan Sitter [:ssitter]]

Upon opening the New Task dialog the following error is displayed:

 Error: document.getElementById("alarm-trigger-relation") has no properties
 Source File: chrome://calendar/content/sun-calendar-event-dialog.js
 Line: 1487

All other issues mentioned in Comment #1 are probably caused by this.

Comment 3

[Martin Schröder [:mschroeder]]

Mickey, this seems to be a serious problem with Tasks and Alarms in the new dialog. Can you have a look?

Comment 4

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

This is a left-over from a discussion I once had with Christian. When he came up with the custom reminder dialog I said that this clashes with the task dialog. In the custom reminder dialog it's possible to define alarms that can relate to the start *or* the end of the timespan of the item. In case of tasks both times (entry date as well as due date) are optional. So what should happen if a reminder is set relative to the end of the timespan but the task has no due date?

It's already a while back, but I decided to just switch reminders off for tasks until the decision has been made how the UI should handle this. So basically this patch just brings the reminders for tasks back and handles the problem by disabling the "Save and Close" button if the reminder/timespan combination is invalid.

I'm asking for UI review since this changes the task dialog but I'd suggest to handle a different solution in a spin-off bug in order to immediately bring the reminders for tasks back and probably handle a different solution later.

Comment 5

[Christian Jansen]

Please note that the whole New Task dialog has not it's final design, until now we only had our focus on the event dialog.

I personally do not like the solution of disabling the Save & Close button. User's won't understand that.

I think it would make more sense to allow users only to set the "date" & "time" for a reminder (Custom Reminder -> Reminder, Option #2).

Comment 6

[Michael Büttner (no reviews TFN)]

(In reply to comment #5)
> I personally do not like the solution of disabling the Save & Close button.
> User's won't understand that.
I also don't like that option, but since there's not clear alternative (at least up to now) this was a shortcut to get reminders for tasks working.

> I think it would make more sense to allow users only to set the
> "date" & "time" for a reminder (Custom Reminder -> Reminder, Option #2).
I don't see how this solves the problem. You could still relate the reminder to the start or end without having that part of the task details being set.

Besides the actual discussion about how we solve this particular issue in an elegant way, I'd like to get this in as I suggested it. We can't have a broken UI for 0.7 and there's probably some time after that release where we'll focus on tasks. Again, either we have an easy solution to the problem or postpone it a bit. Wouldn't you agree?

Comment 7

[Christian Jansen]

(In reply to comment #6)
> (In reply to comment #5)
> > I personally do not like the solution of disabling the Save & Close button.
> > User's won't understand that.
> I also don't like that option, but since there's not clear alternative (at
> least up to now) this was a shortcut to get reminders for tasks working.
> 
> > I think it would make more sense to allow users only to set the
> > "date" & "time" for a reminder (Custom Reminder -> Reminder, Option #2).
> I don't see how this solves the problem. You could still relate the
> reminder to the start or end without having that part of the task
> details being set.

Ok. What do you think about the following solution? We remove the "Custom Reminder" dialog for Tasks. But therefore we replace the
"Reminder: [ No Reminder \/]" drop down by a date and a time picker.


Repeat:       [Does not repeat \/]
----------------------------------------------
Reminder:     [01.08.2007      \/] [16:15 \/]
----------------------------------------------
Description:  +------------------------------
              |

The default would be:

Reminder: [No Reminder \/] [Current Hour \/]

(Time disabled until date has been set)

> 
> Besides the actual discussion about how we solve this particular issue in an
> elegant way, I'd like to get this in as I suggested it. We can't have a broken
> UI for 0.7 and there's probably some time after that release where we'll focus
> on tasks.

Sorry, but from a users point of view is a disabled "Save" button is broken UI.  For them it is nearly impossible to figure out what went wrong during the task creation process.

Even worse. In addition to the disabled Save button we also would have to disable the Task dialog closer. Doing such, would mean that users can't close the dialog easily. With an enabled closer the "Save Task" dialog would pop-up, without the option to save....

> Again, either we have an easy solution to the problem or postpone it
> a bit. Wouldn't you agree?

I'd be very very happy if there is an easy to fix and easy to understand solution, but in this case I think we need to do some redesign.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'll review this when the style nits patch is in and it has UI review.

Comment 9

[Michael Büttner (no reviews TFN)]

(In reply to comment #7)
Christian, this would change the semantics of how reminders work in the dialog. Basically, you suggest to drop support for relative reminders (relative to start or end) and introduce absolute reminders (at a specific date and time). Although this solution is possible it is plagued by the problem that it's technically possible (RFC 2445 - 4.6.6 Alarm Component) but our current API doesn't allow for absolute alarms. As I'm already stuffed with bugs in need within the 0.7 timeframe I currently don't see any chance to take care of fixing this bug from ground up (API changes, revised reminder controls, etc.).

Comment 10

[Michael Büttner (no reviews TFN)]

I spend now some time thinking about a reasonable solution to the problem. The following is proposal I'm coming up with.

We never disable the "Save and close" button. Should the dialog be confronted with an invalid combination it automatically checks the entry- or due-date checboxes. Just to make sure the user can't take away the date the reminder relies on we disable the checkbox itself.

So, to clarify what I'm trying to explain, here's an example:

1) entry- as well as due-date checkbox is not checked
2) user sets an reminder - 5 minutes before
3) dialog automatically checks the entry date (since the reminder relies on it)
4) the entry date checkbox gets disabled
...
5) user revokes set a different reminder - 5 minutes before event ends
6) entry date checkbox gets enabled (but keeps being checked)
7) dialog automatically checks the due date (since the reminder relies on it)
8) the due date checkbox gets disabled
...
9) user revokes reminder - no reminder set at all
10) due date checkbox gets enabled (but keeps being checked)

I suggest that we go ahead with this proposal, at least for the 0.7 timeframe. We could later add support for absolute alarms but take this as an interims solution. Christian, what's your opinion on that?

Comment 11

[Christian Jansen]

I'll keep it short. +1 for that.

Comment 12

[Michael Büttner (no reviews TFN)]

Upping severity and putting this on the 0.7 blocker list, since basically the new dialog cut off the "alarm for tasks" feature and we presumably don't want to go without.

Comment 13

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

This patch implements the suggestion I made previously (see comment #10). It was much more complex than I initially thought, since I already used the same approach for repeating tasks (I'm checking & disabling the entry date since repeating tasks need it). This made the whole story a bit more complicated as the dialog should behave correctly in cases such as the following:

1) make task recurring
2) set a reminder
3) revoke recurrence
4) remove reminder

This should leave us with a checked but enabled entry date. There are a hell of a lot of other combinations that we need to take care of. I'm introducing in this patch a locking mechanism that is applied to the checkbox of the entry- and due date, respectively. The checkbox remembers who is holding locks and allows to be enabled only if no lock is held any longer.

Comment 14

[Christian Jansen]

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

r=Christian for this patch.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

r- as discussed, some disable/enable semantics don't quite work.

Everything else is fine, minor style nits:


>+        if (!element.hasAttribute(lockId)) {
>+            element.setAttribute(lockId,"true");
>+            var n = parseInt(element.getAttribute("lock") || 0);
>+            element.setAttribute("lock",n + 1);
>+        }
Check spaces after commas here

>+function enableElementWithLock(elementId,lockId) {
and here

>-        editRepeat();
>+        if (gIsUpAndRunning) {
>+            editRepeat();
>+        }
A small comment about what gIsUpAndRunning is for would be nice. I realize where it is set above, but I'm not too sure why editRepeat should only be set if gIsUpAndRunning is true



mozilla/calendar/base/content/calendar-dialog-utils.js:24: Trailing whitespace
+

mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js:103: Trailing whitespace
+

mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js:177: Missing blank before `(`
+        if(menuitem.value == 'none') {

mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js:192: Trailing whitespace
+

Comment 16

[Stefan Sitter [:ssitter]]

Updating the summary to reflect the topics that are now handled in this bug.

Comment 17

[Omar Bajraszewski]

(In reply to comment #0)

> Actual Results:  
> see bug 342150 comment 20


Tooltip shows also "all day" next to start date

Comment 18

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

This is an updated version of the patch. I addressed all the issues raised in previous comments, and I also got rid of the global variable 'gIsUpAndRunning'.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

>+
>+    // possibly the selected reminder conflicts with the item.
>+    // for example an end-relation combined with a task without duedate
>+    // is an invalid state we need to take care of. we take the same
>+    // approach as with recurring tasks. in case the reminder is related
>+    // to the entry date we check the entry date automatically and disable
>+    // the checkbox. the same goes for end related reminder and the due date.
Here and in the rest of this chunk, a lot of ^M's show up. Other than that everything looks fine. r=philipp

Comment 20

[Omar Bajraszewski]

(In reply to comment #0)

> when "default alarm setting for tasks" is on and I want to create new task
> default time = 0:00

and in "New task" window date&time picker for 'Due' is active although the box is unchecked. Checking and unchecking the box make the picker inactive

Comment 21

[Michael Büttner (no reviews TFN)]

Attachment: patch v4

There were still some problems related to updating the various controls in the last patch. I thoroughly tested this final patch which I'm going to check in now. Attaching it for reference purposes and carrying over review flags...

Comment 22

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 23

[Omar Bajraszewski]

I can still reproduce it with lightning 2007083103 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070831 Calendar/0.7pre


1) Start time is always 0:00
2) After changing it and saving the task, it's reset back to 0:00

Steps to reproduce:
1)Create new profile
2)Go to settings and set default alarm for event and task on
3)Open new task window
4)Set start time i.e. 08:00
5)Set due time i.e. 17:00
6)Enter title and save

Seems the patch doesn't work

Comment 24

[Omar Bajraszewski]

After talking to Mickey---> reopening
See flash video

Comment 25

[Michael Büttner (no reviews TFN)]

Attachment: add-on patch v1

This patch actually resolves the initial problem. I was indeed focused on finding a solution to the problems stated in comment #10 and comment #13 above that I missed the initial problem - this add-on patch should get it done.

Comment 26

[Daniel Boelzle [:dbo]]

Comment on attachment 280613 [details] [diff] [review]
add-on patch v1

spend a helper function, e.g. identicalDatetimes(); IMO makes the code clearer.

>+                value.isDate && !item.entryDate.isDate ||
>+                !value.isDate && item.entryDate.isDate ||
for brevity just (one.isDate != item.entryDate.isDate)

r=dbo

Comment 27

[Michael Büttner (no reviews TFN)]

add-on patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 28

[Omar Bajraszewski]

Verified with lightning 2007091903 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20070919 Calendar/0.7pre