Last Comment Bug 561181 - Reminder "0 minutes before" is shown as "The moment the events starts" after restart
: Reminder "0 minutes before" is shown as "The moment the events starts" after ...
Status: RESOLVED FIXED
[good first bug][mentor=Fallen][lang=js]
:
Product: Calendar
Classification: Client Software
Component: Dialogs (show other bugs)
: Trunk
: x86 Windows 7
: -- minor (vote)
: 2.1
Assigned To: Liu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-22 13:28 PDT by Stefan Sitter
Modified: 2012-11-02 08:15 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add debug cal.errors, modify origin in matchCustomReminderToMenuitem() (7.48 KB, patch)
2012-10-17 07:36 PDT, Liu
no flags Details | Diff | Review
The same modifications,but remove debuggers (2.67 KB, patch)
2012-10-24 07:52 PDT, Liu
no flags Details | Diff | Review
Use if-else to get the correct origin value in matchCustomReminderToMenuitem(reminder) (2.95 KB, patch)
2012-10-26 00:22 PDT, Liu
philipp: review+
Details | Diff | Review

Description Stefan Sitter 2010-04-22 13:28:38 PDT
Lightning 1.0b2pre (BuildID: 20100422035110) with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.5pre) Gecko/20100422 Lanikai/3.1b2pre

Reminder "0 minutes before" is shown as "The moment the events starts" after restart

Steps To Reproduce:
  1. Create new Thunderbird profile and install Lightning
  2. Create new event in the default calendar
  3. Select "0 minutes before" from reminder dropdown menu
  4. Save event and close dialog
  5. Reopen the event for editing and check the reminder settings
  6. Restart Thunderbird
  7. Reopen the event for editing and check the reminder settings

Actual results:
  After creation the dialog shows:
    Reminder: [0 minutes before  |v]
  After restart the dialog shows:
    Reminder: [Custom...         |v] The moment the events starts

Expected results:
Always show the default entry from the dropdown menu.
Comment 1 Liu 2012-08-18 05:12:54 PDT
Solving this could be a good start for a beginner like me to contribute.. Could u please give any guide about how to get to start?
Comment 2 Philipp Kewisch [:Fallen] 2012-08-18 15:29:24 PDT
To get started, you should first get Thunderbird built with Lightning included. Se https://developer.mozilla.org/En/Simple_Thunderbird_build for details, make sure to set up your mozconfig with --enable-calendar before you start the build.

The starting point for this can be found here: http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-dialog-utils.js#558

You need to debug through it (using printf-style debugging with cal.ERROR(), sorry) and find out why its not matching the alarm to the right menuitem. The function likely needing a fix is here: http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-dialog-utils.js#516
Comment 3 Liu 2012-08-18 19:04:03 PDT
Thanks. I've built thunderbird with lightning, now I will go to work on this :)
Comment 4 Liu 2012-09-09 10:29:57 PDT
Hi Philipp, should the expected result be that after reopen, the reminder dialog shows "no reminder"? Or showing the selection ("0 minutes before", in your description) made before the reopen? Thanks.
Comment 5 Philipp Kewisch [:Fallen] 2012-09-17 06:04:22 PDT
It should show "0 minutes before" selected in the dropdown.
Comment 6 Liu 2012-10-17 06:09:14 PDT
Hi Fallen
just wondering whether it is necessary to change this back, since "0 minute before"
is the same with "the moment the event starts".

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> It should show "0 minutes before" selected in the dropdown.
Comment 7 Liu 2012-10-17 07:36:23 PDT
Created attachment 672303 [details] [diff] [review]
add debug cal.errors, modify origin in matchCustomReminderToMenuitem()

Hi Fallen
I found the problem occurs when judging whether the reminder belongs to one of 
those in the menu list. Originally the attribute offset must be less than 0 (marked as "before") to pass the check, otherwise the reminder will be cast to a custom reminder. However "0 minute before" has an offset of 0, which makes it fail to pass the check and be cast into a custom reminder. Now I've changed how 
it check the offset, and allow reminder with offset not positive (include 0) to pass, and the bug seems to be fixed by this. :)
Comment 8 Stefan Sitter 2012-10-17 13:24:02 PDT
Hi Liu, I'd like to ask you to attach a proper patch. Your current patch looks fine on the first glance but adds many error/debug messages that should not be part of it.
Comment 9 Liu 2012-10-17 22:45:19 PDT
Thank you for the advice, I think I will exclude them in the next patch :)

(In reply to Stefan Sitter from comment #8)
> Hi Liu, I'd like to ask you to attach a proper patch. Your current patch
> looks fine on the first glance but adds many error/debug messages that
> should not be part of it.
Comment 10 Liu 2012-10-24 07:52:54 PDT
Created attachment 674660 [details] [diff] [review]
The same modifications,but remove debuggers

This patch can fix the bug, and the former added debuggers are removed.
Comment 11 Liu 2012-10-24 10:35:47 PDT
Hi Fallen

I found the problem is exactly at
 let origin = (reminder.offSet.isNegative?"before":"after");
It seems if the duration is 0(o minutes before), it is treated as not negative and 
hence the origin to be "after", which actually should be "before", so I was trying 
to use "isPositive" to include duration 0 into the "before" origin. Since we can not use isPositive, is there anything to distinguish 0 duration from positive duration?
Comment 12 Liu 2012-10-26 00:22:18 PDT
Created attachment 675481 [details] [diff] [review]
Use if-else to get the correct origin value in matchCustomReminderToMenuitem(reminder)

matchCustomReminderToMenuitem(reminder) is used when load and match reminders. "0 minutes before" has no menuitem  match caused by "origin = (reminders.offset.isNegative?"before":"after")", which makes its origin a "after". The patch rewrite part of the function, to get "0 minutes before" a origin of "before", and therefore get a correct menuitem match for it.
Comment 13 Philipp Kewisch [:Fallen] 2012-11-02 08:11:40 PDT
Comment on attachment 675481 [details] [diff] [review]
Use if-else to get the correct origin value in matchCustomReminderToMenuitem(reminder)

Review of attachment 675481 [details] [diff] [review]:
-----------------------------------------------------------------

Aside from a few indentation fixes this looks fine, r=philipp

I will fix them before checkin.
Comment 14 Philipp Kewisch [:Fallen] 2012-11-02 08:15:49 PDT
Pushed to comm-central changeset ed797863fdae

Note You need to log in before you can comment on or make changes to this bug.