453138 - Display 'Last day of the month' recurrence rule as clear text (missing bits and pieces from bug 388418)

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Martin Schröder [:mschroeder]]

Attachment: Old patch from bug 388418

Bug 388418, comment 26 has patch (I also attached to this bug) which never got checked in. It adds the missing bits and pieces to display the 'Last day of
the month' recurrence rule as clear text.

Comment 1

[Stefan Sitter [:ssitter]]

Martin, could you provide an updated patch?

Comment 2

[Martin Schröder [:mschroeder]]

Attachment: Patch v1

Comment 3

[Stefan Sitter [:ssitter]]

Comment on attachment 346017 [details] [diff] [review]
Patch v1

From a quick test yesterday the patch seems to work fine. 

-repeatDetailsRuleMonthly1=the %1$S %2$S
+repeatDetailsRuleMonthly1=the %1$S %2$S of every month

I think the property name should be changed to ensure that localization teams pick up the change.

Comment 4

[Berend Cornelius]

Comment on attachment 346017 [details] [diff] [review]
Patch v1

Patch looks good. 
Just some comments that you may probably want to consider:
>+                                ruleString = calGetString(
>+                                  "calendar-event-dialog",
>+                                  "repeatDetailsRuleMonthly7");

The wrapping could be improved at some places. See also
https://wiki.mozilla.org/Calendar:Style_Guide#Wrapping
(I know of course that you have not introduced this)
The name of the file “calendar-dialog-utils.js” is actually not valid anymore as the code is also used in other places outside the dialog. But on the other hand with its current naming it should be moved to “base/content/dialogs”. Referring to resources of calendar-dialog-properties is for the similarily  not very elegant anymore. We should either move it to the dialogs folder or restructure the code of this file. Probably we should just take the first option.

The use of the 'interval' attribute is not really clear. I suggest there should be more comments added to the idl file or/and to the source file.
r=berend

Comment 5

[Martin Schröder [:mschroeder]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/aa6e68112fd2>

-> FIXED(In reply to comment #4)

> (From update of attachment 346017 [details] [diff] [review])
> The wrapping could be improved at some places.

We should do this in a new bug report because the changes would be spread all over the file.

> The name of the file “calendar-dialog-utils.js” is actually not valid anymore
> as the code is also used in other places outside the dialog. But on the other
> hand with its current naming it should be moved to “base/content/dialogs”.
> Referring to resources of calendar-dialog-properties is for the similarily  not
> very elegant anymore. We should either move it to the dialogs folder or
> restructure the code of this file. Probably we should just take the first
> option.

Needs a spin-off bug.

> The use of the 'interval' attribute is not really clear. I suggest there should
> be more comments added to the idl file or/and to the source file.

Berend, please file a spin-off bug.

Comment 6

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".