463050 - Slight improvements to calIAlarm

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

To further push bug 353492, this patch contains improvements to calIAlarm that are needed. This may not be the final state (esp. w.r.t. the use of the |item| member), but it does fix a few issues.

This patch requires the iterators patch from bug 463047 and also takes care of alignment in locales/jar.mn.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Comment 2

[Daniel Boelzle [:dbo]]

Comment on attachment 346310 [details] [diff] [review]
Fix - v2

>-    _getAlarmDate: function cA_getAlarmDate() {
>-        var itemAlarmDate;
>+    _getAlarmDate: function cA__getAlarmDate() {
>+        let itemAlarmDate;
>         if (isEvent(this.mItem)) {
>             switch (this.related) {
>                 case Components.interfaces.calIAlarm.ALARM_RELATED_START:

Admittdedly a bit off-topic, but IMO we should try to remove the caclic reference between item and alarm object, and pass in the item object for alarm date calculation etc.

>-        for (var i = 0; i < this.attachments.length; i++) {
>-            var attachment = this.attachments[i];
>-            var attachmentProp = icssvc.createIcalProperty("ATTACH");
>+        for (let i = 0; i < this.attachments.length; i++) {

why no for each loop?

>-        for (var attendeeProp = aComp.getFirstProperty("ATTENDEE");
>-             attendeeProp;
>-             attendeeProp = aComp.getNextProperty("ATTENDEE")) {
>-            // XXX this.addAttendee(attendeeProp.value);
>+        for each (let attendee in cal.icalPropIterator(aComp, "ATTENDEE")) {
>+            // XXX this.addAttendee(attendee);

name has changed to cal.ical.propertyIterator.

>+        for each (let attach in cal.icalPropIterator(aComp, "ATTACH")) {
>+            // XXX this.addAttachment(attach);

dto.

>-        for (var prop = aComp.getFirstProperty("ANY");
>-             prop;
>-             prop = aComp.getNextProperty("ANY")) {
>+        for (let prop in cal.icalPropertyIterator(aComp)) {

dto.


>+
>+                for (let paramName in cal.icalParamIterator(prop)) {

cal.ical.paramIterator

r=dbo with that resolved

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

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

-> FIXED

Comment 4

[Vlado Valastiak [:wladow]]

Comment on attachment 346310 [details] [diff] [review]
Fix - v2

>+reminderCustomRelationBefore=before
>+reminderCustomRelationAfter=after
>+reminderCustomOriginBeginEvent=the event starts
>+reminderCustomOriginEndEvent=the event ends
>+reminderCustomOriginBeginTask=the task starts
>+reminderCustomOriginEndTask=the task ends
>+

This is not ideal, because we have different forms for CustomOrigin depending on CustomRelation. Please avoid joining parts of sentences where possible. 

So, I'm suggesting something like this:
> reminderCustomOriginBeginBeforeEvent=before the event starts
> reminderCustomOriginBeginAfterEvent=after the event starts
> reminderCustomOriginEndBeforeEvent=before the event ends
> reminderCustomOriginEndAfterEvent=after the event ends
> reminderCustomOriginBeginBeforeTask=before the task starts
> reminderCustomOriginBeginAfterTask=after the task starts
> reminderCustomOriginEndBeforeTask=before the task ends
> reminderCustomOriginEndAfterTask=after the task ends

Comment 5

[Simon Paquet [:sipaq]]

Reopening because of comment 4. Philipp, could you please address this? Thanks.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: l10n fix - v1

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Does the wording on your new reminderCustomOrigin strings also change depending on if plural form is used? 

i.e should I expand even further and use

reminderCustomTitleBeginBeforeEvent=#1 before the event starts;#1 before the event starts

and then insert the result of the reminderCustomUnitMinutes into #1 ?

Comment 8

[Vlado Valastiak [:wladow]]

(In reply to comment #7)
> Does the wording on your new reminderCustomOrigin strings also change depending
> on if plural form is used? 

No, this patch is good enough, at least for sk. thx.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

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

-> FIXED