Closed Bug 385896 Opened 17 years ago Closed 17 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: andreas.treumann, Assigned: michael.buettner)

References

Details

(Whiteboard: [patch in hand])

Attachments

(1 file, 1 obsolete file)

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'
Flags: blocking-calendar0.7?
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.
Flags: blocking-calendar0.7? → blocking-calendar0.7+
Assignee: michael.buettner → ssitter
Status: NEW → ASSIGNED
Attachment #273992 - Flags: review?(michael.buettner)
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
Attachment #273992 - Flags: review?(michael.buettner) → review-
I currently don't have access to my developer computer. Feel free to take over the bug and submit a new patch.
Assignee: ssitter → nobody
Status: ASSIGNED → NEW
Attached patch patch v1Splinter Review
Fixed damn typo (thanks to stefan for spotting this) and adapted item matching code.
Assignee: nobody → michael.buettner
Attachment #273992 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #275443 - Flags: review?(philipp)
Whiteboard: [patch in hand]
Comment on attachment 275443 [details] [diff] [review]
patch v1

Code is fine, works as advertised. r=philipp
Attachment #275443 - Flags: review?(philipp) → review+
Oh I almost forgot, the obligatory style nit ;)

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

spaces before and after * operator
Keywords: checkin-needed
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → 0.7
Verified in Lightning (build 2007082204) and Sunbird (build 20070822) -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: