385896 - [Proto] reminder '1 week before' doesn't work

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Andreas Treumann]

STEPS TO REPRODUCE:
===================

- create a new event
- set the reminder '1 week before'
- save the event
- open the edit dialog again and check the reminder setting

RESULT:
=======

- the reminder setting is now '2 days before'

EXPECTED RESULT:
================

- the reminder setting should be '1 week before'

REPRODUCIBLE:
=============

- always


If I use the 'Custom' reminder dialog and create the reminder '7 days before' then the reminder is changed to '45 minutes before'

Comment 1

[cmtalbert]

Marking blocking, we can't have broken UI for 0.7.  Since Mickey's fix queue is quite lengthy, I would invite any developer to snatch this one and reassign it.  It should be a pretty simple fix.

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: rev0 - assume that 1 week equals 7 days

Comment 3

[Michael Büttner (no reviews TFN)]

Comment on attachment 273992 [details] [diff] [review]
rev0 - assume that 1 week equals 7 days

>                             <menuitem label="&event.reminder.1week.before.label;"
>-                                      length="2"
>+                                      length="7"
Damn typo, thanks for having spotted this. But unfortunately the solution is a bit more complex than fixing the wrong 'length'-attribute. Basically it works as expected, but the dialog comes up with the information that a custom reminder has been set as "7 days before the events starts". This is not the correct behavior, as the dialog should just use the "1 week" predefined reminder setting.

There's just part of the solution missing. Please have a look at [1] where the alarm offset of the item gets matched against the predefined reminders. In case we have used the "1 week" reminder, the alarm offset has been set to 1 week instead of 7 days. So we just need an additional 'if'-clause that could look like this:

          } else if(unit == "days" && item.alarmOffset.weeks == length/7) {
            matchingItem = menuitem;
            break;
          }

I didn't test this, but it should come close...

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/prototypes/wcap/sun-calendar-event-dialog.js#965

Comment 4

[Stefan Sitter [:ssitter]]

I currently don't have access to my developer computer. Feel free to take over the bug and submit a new patch.

Comment 5

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

Fixed damn typo (thanks to stefan for spotting this) and adapted item matching code.

Comment 6

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

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

Code is fine, works as advertised. r=philipp

Comment 7

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

Oh I almost forgot, the obligatory style nit ;)

>+                    item.alarmOffset.weeks*7 == length) {

spaces before and after * operator

Comment 9

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 10

[Andreas Treumann]

Verified in Lightning (build 2007082204) and Sunbird (build 20070822) -> task is fixed.