Use cal prefix for all calUtils functions

RESOLVED FIXED in 5.7

Status

Calendar
General
P1
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

7 months ago
The upcoming patch will make sure that all current calUtils functions use the cal. prefix. While we may eventually decide differently based on bug 905097 comment 1, I think it would be a good idea to stay consistent and always use the cal. prefix (besides, I already had most of it written).
(Assignee)

Comment 1

7 months ago
Created attachment 8847100 [details] [diff] [review]
Use cal prefix - v1
Attachment #8847100 - Flags: review?(makemyday)
(Assignee)

Comment 2

7 months ago
Created attachment 8847102 [details] [diff] [review]
Use cal prefix - v1

Fixed commit message
Attachment #8847100 - Attachment is obsolete: true
Attachment #8847100 - Flags: review?(makemyday)
(Assignee)

Comment 3

7 months ago
Created attachment 8847103 [details] [diff] [review]
Remove calUtils.js include - v1
Attachment #8847103 - Flags: review?(makemyday)
(Assignee)

Updated

7 months ago
Blocks: 1347211
(Assignee)

Comment 4

7 months ago
Created attachment 8847166 [details] [diff] [review]
Use cal prefix - v2

And now without accidentally adding calendar-event-dialog back in
Attachment #8847102 - Attachment is obsolete: true
Attachment #8847166 - Flags: review?(makemyday)
(Assignee)

Comment 5

7 months ago
Created attachment 8847178 [details] [diff] [review]
Use cal prefix - v3
Attachment #8847166 - Attachment is obsolete: true
Attachment #8847166 - Flags: review?(makemyday)
Attachment #8847178 - Flags: review?(makemyday)

Comment 6

7 months ago
Comment on attachment 8847178 [details] [diff] [review]
Use cal prefix - v3

Review of attachment 8847178 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the nits fixed.

As we now have several cal.calSomething names (like cal.calGetString), can we please consolidate the names when moving over from calUtils to calUtils.jsm to cal.something?

::: calendar/base/content/calendar-base-view.xml
@@ +245,2 @@
>          let alarmService = Components.classes["@mozilla.org/calendar/alarm-service;1"]
>                             .getService(Components.interfaces.calIAlarmService);

Can you enlarge the indentation here to align the dots?

@@ +552,5 @@
>        <method name="deleteItemsFromCalendar">
>          <parameter name="aCalendar"/>
>          <body><![CDATA[
>            /* This method must be implemented in subclasses. */
> +          throw Components.results.NS_cal.ERROR_NOT_IMPLEMENTED;

Let's leave it with NS_ERROR_NOT_IMPLEMENTED ;-)

::: calendar/base/content/calendar-statusbar.js
@@ +103,1 @@
>                                                   [this.mCurIndex, this.mCalendarCount]);

indentation.
Attachment #8847178 - Flags: review?(makemyday) → review+

Updated

7 months ago
Attachment #8847103 - Flags: review?(makemyday) → review+
(Assignee)

Comment 7

6 months ago
(In reply to [:MakeMyDay] from comment #6)
> Comment on attachment 8847178 [details] [diff] [review]
> Use cal prefix - v3
> 
> Review of attachment 8847178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the nits fixed.
> 
> As we now have several cal.calSomething names (like cal.calGetString), can
> we please consolidate the names when moving over from calUtils to
> calUtils.jsm to cal.something?
Yes, this will happen in another patch.


> > +          throw Components.results.NS_cal.ERROR_NOT_IMPLEMENTED;
> 
> Let's leave it with NS_ERROR_NOT_IMPLEMENTED ;-)
Ha! Great catch :)

Will fix these issues before checkin!
Priority: -- → P1
Created attachment 8874551 [details] [diff] [review]
Updated and Combined patch

After talking to Philipp, I updated and combined the patches to a check-in ready version.

Philipp, please set the checkin-needed keyword if you think it can land.
Attachment #8847103 - Attachment is obsolete: true
Attachment #8847178 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8874551 - Flags: review+
Created attachment 8875458 [details] [diff] [review]
Updated and Combined patch, v2

Updated the patch after some try-server runs
Attachment #8874551 - Attachment is obsolete: true
Attachment #8875458 - Flags: review+
Duplicate of this bug: 905101
After talking to Philipp over IRC, I set the checkin-needed keyword myself.
Flags: needinfo?(philipp)
Keywords: checkin-needed

Comment 12

5 months ago
https://hg.mozilla.org/comm-central/rev/4bafb6c2ddaf96fce9afed6ad76793224927207e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.7

Comment 13

4 months ago
Add-on Lightning Calendar Tabs is now broken:
ReferenceError: getPrefSafe is not defined       multiweek_tabs.js:39:3
ReferenceError: getDateFormatter is not defined  month_tabs.js:44:7

Looks like they need to prepend cal.getDateFormatter, but getPrefSafe seems to have disappeared.

Comment 14

4 months ago
BTW, I fixed the add-on by adding "cal." at month_tabs.js. Multiweek tabs still don't work since getPrefSafe was removed ages ago, I didn't look where, but it's mentioned in bug 1032172.

Comment 15

4 months ago
Bug 923026 marked getPrefSafe() as deprecated in Lighting 3.2 about 3 years ago. Please report errors to the extension author.

Comment 16

4 months ago
I did ;-). As I said in comment #14, I've fixed the function I need myself. Sorry about the noise.
You need to log in before you can comment on or make changes to this bug.