Closed Bug 463050 Opened 11 years ago Closed 11 years ago

Slight improvements to calIAlarm

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

(Keywords: l12y)

Attachments

(2 files, 1 obsolete file)

Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
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.
Attachment #346266 - Flags: review?(daniel.boelzle)
Attached patch Fix - v2 β€” β€” Splinter Review
Attachment #346266 - Attachment is obsolete: true
Attachment #346310 - Flags: review?(daniel.boelzle)
Attachment #346266 - Flags: review?(daniel.boelzle)
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
Attachment #346310 - Flags: review?(daniel.boelzle) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/2247db027f7f>

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
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
Reopening because of comment 4. Philipp, could you please address this? Thanks.
Status: RESOLVED → REOPENED
Keywords: l12y
Resolution: FIXED → ---
Attached patch l10n fix - v1 β€” β€” Splinter Review
Attachment #352832 - Flags: review?(daniel.boelzle)
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 ?
(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.
Attachment #352832 - Flags: review?(daniel.boelzle) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/610f5fc83b1f>

-> FIXED
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.