389540 - eliminate superfluous bindings in recurrence dialog implementation

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Michael Büttner (no reviews TFN)]

The current implementation of the recurrence dialog consists out of the following files:

- sun-calendar-event-dialog-recurrence.js
- sun-calendar-event-dialog-recurrence.xml
- sun-calendar-event-dialog-recurrence.xul

The xml-file (see [1]) contains a single binding 'recurrence-page' which is used only in the respective xul-file (s-c-e-d-recurrence.xul). The binding is no longer necessary and all the xul-content can live in ...-recurrence.xul.

The design of the dialog changed several times and one particular iteration used tabs to group the content of the dialog into larger chunks. I knew that the design won't be final and that's why I initially implemented the larger chunks in bindings to quickly being able to shuffle the content around. This is no longer necessary and we can get rid of some unnecessary code.

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

Comment 1

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

This patch eliminates the file 'sun-calendar-event-dialog-recurrence.xml' and distributes the content to '...-dialog-recurrence.js' and '...-dialog-recurrence.xul', thus removing the large binding in favor of plain xul. Philipp, I'm also interested in general comment regarding the code, since apart from moving the code around this patch doesn't do much harm. Basically I adjusted the pieces that needed adjustment and I removed some code that was needed ages ago but is just superfluous these days.

Furthermore, there are still some bindings. I'll list them here for completeness:

- ...recurrence-preview.xml
    -> recurrence-calendar [will be eliminated with bug #389535]
    -> recurrence-preview
- ...recurrence-datepicker.xml
    -> datepicker-box
    -> datepicker-weekday [will be enhanced with bug #378172]
    -> datepicker-monthday [will be enhanced with bug #378172]

I think 'recurrence-preview' as well as the datepicker bindings are valuable bindings in itself as they represent a self-contained control. Obviously, they are currently just used in the recurrence dialog but I'd argue against converting them to plain xul. Admittedly, we could move these bindings to a more appropriate place, but this can be done when the dialog itself moves to base/content. Philipp, what's your opinion on that?

Comment 2

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

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

Looks good. I agree that we can keep the remaining bindings, codewise I have nothing to add. Just some minor nits:

>+function splitRecurrenceRules(recurrenceInfo) {
I think this needs to go into some util file, it is used in sun-calendar-event-dialog.js, ...-recurrence.js and I believe also in the summary dialog.

>+              </radiogroup>
>+            </box>
>+            <!-- Weekly -->
>+            <vbox>
If you add a blank line before the other comments, also do so here.



r=philipp

Comment 3

[Michael Büttner (no reviews TFN)]

(In reply to comment #2)
> >+function splitRecurrenceRules(recurrenceInfo) {
> I think this needs to go into some util file, it is used in
> sun-calendar-event-dialog.js, ...-recurrence.js and I believe also in the
> summary dialog.
Bug 361977 will move this function to the shared utility file, so I'm leaving this as is for now.

Comment 4

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 5

[Andreas Treumann]

Verified in latest nightly build 20080114 -> task is fixed.