Closed Bug 403882 Opened 12 years ago Closed 10 years ago

Invite Attendees Dialog: all-day doesn't disable time picker

Categories

(Calendar :: Dialogs, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: bv1578)

Details

Attachments

(1 file)

1. Open event dialog.
2. Open attendee dialog (press "Invite Attendees").
3. Check all-day.
4. Close attendee dialog.
=> All-day flag is checked, but the time picker is still enabled.
Component: General → Dialogs
QA Contact: general → dialogs
Summary: Event Dialog: all-day doesn't disable time picker → Invite Attendees Dialog: all-day doesn't disable time picker
Version: unspecified → Trunk
Attached patch patchSplinter Review
As far as I can see, it seems that the function "updateAllDay()"  (which updates the timepickes status) is never execute when it is called [1] from inside "updateDateTime()" function  because "gIgnoreUpdate" is always true. In fact it is being set to true and false only at the beginning and the end of that function:
http://tinyurl.com/yb6epjw

When the code returns from Invite Attendees dialog [2], the function updateDateTime() sets the "event-all-day" checkbox and the values for date-timepickers, then "updateAllDay()" should set the disable status, but this doesn't happen for the reason mentioned above.

If the previous analysis is right, this patch deletes the call to  "updateAllDay()" inside "updateDateTime()" (only because it seems useless) and calls the function after "updateDateTime()".


[1] http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog.js#2608

[2] http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog.js#1252
Assignee: nobody → bv1578
Attachment #414579 - Flags: review?(dbo.moz)
Status: NEW → ASSIGNED
Comment on attachment 414579 [details] [diff] [review]
patch

Martin, could you please take over?
Attachment #414579 - Flags: review?(dbo.moz) → review?(mschroeder)
Comment on attachment 414579 [details] [diff] [review]
patch

I would give r+ based on the behavior, but the code seems to have many side-effects and updateAllDay() would not be called on updateDateTime() anymore. Philipp, can you also have a look at this patch? We should consider a refactoring of this code (few documentation, too many line in the file)!
Attachment #414579 - Flags: review?(mschroeder) → review?(philipp)
Comment on attachment 414579 [details] [diff] [review]
patch

I think we should keep the updateAllDay in the updateDateTime() function.

With just the first patch hunk, the bug is also fixed.

r=philipp with that changed, will do so before checkin.
Attachment #414579 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/633f19713dae>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b2
You need to log in before you can comment on or make changes to this bug.