Status

enhancement
RESOLVED FIXED
11 months ago
3 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

11 months ago
It has an enormous amount of spelling mistakes, see attachment 8960495 [details] [diff] [review].

When is a good time to do this?
Flags: needinfo?(philipp)
Flags: needinfo?(mschroeder)
Flags: needinfo?(makemyday)

Comment 1

11 months ago
Thanks for taking care of that, much apprechiated. For changes in ical.js, you would need to do a pull request on Github [1] since that is imported from upstream (where it consists of multiple files). You have also changes in strings in that patch, that might require and update of the string name. Can you split all changes to .properties or .dtd files to a separate patch?

Since it looks that this is a rerunable scripted correction, I think we should hold that back still for 1 to 2 months to make backporting patches for ESR more easy.

[1] https://github.com/mozilla-comm/ical.js/
Assignee

Comment 2

11 months ago
(In reply to [:MakeMyDay] from comment #1)
> You have also changes
> in strings in that patch, that might require and update of the string name.
I don't think fixing an obvious spelling mistake (mostly in comments, but also occur(r)ed, transfer(r)ed) needs a new string.

OK, I'll revisit this in two months.
Flags: needinfo?(philipp)
Flags: needinfo?(mschroeder)
Assignee

Comment 3

3 months ago

I needed a patch to land, so I grabbed three directories from the original patch. No .properties or .dtd files and no ical.js.

Flags: needinfo?(makemyday)
Keywords: leave-open
Assignee

Comment 4

3 months ago

Totally boring.

Geoff, ical.js is just one file, right? calendar/base/modules/ical.js

And I guess we don't want to mess with libical either. Can you please take a look at the original patch in attachment 8960495 [details] [diff] [review] and tell me what not to touch.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9053043 - Flags: review?(geoff)

Comment 5

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d1aa51f98737
fix typos in calendar/providers, resources, test using codespell. rs=typo-fix,comment-only
Comment on attachment 9053043 [details] [diff] [review]
Fix spelliing in providers, resources, test

What a good job we have computers to teach us how to spell.
Attachment #9053043 - Flags: review?(geoff) → review+

(In reply to Jorg K (GMT+1) from comment #4)

Created attachment 9053043 [details] [diff] [review]
Fix spelliing in providers, resources, test

Totally boring.

Geoff, ical.js is just one file, right? calendar/base/modules/ical.js

Yes, that's right.

And I guess we don't want to mess with libical either.

As I understand it, our copy of libical is fair game for stuff like this. Our fork is ancient and I don't believe there are any plans to update it. Apparently not.

Can you please take a look at the original patch in attachment 8960495 [details] [diff] [review] and tell me what not to touch.

I had a quick look. Nothing jumped out at me other than the two exceptions already mentioned. And this, which really should be fixed:

-                  commmand="calendar_publish_calendar_command"
+                  command="calendar_publish_calendar_command"
Assignee

Comment 8

3 months ago

Mostly comments, some typos in strings, surely translators don't need to know we fixed them.

Attachment #9053975 - Flags: review?(geoff)

Comment 9

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/303c1d12c1f6
fix typos in calendar/locales using codespell. rs=typo-fix,comment-only
Assignee

Comment 10

3 months ago

Second last one, then there's only calendar/base left, that has the most code and mistakes. Note that I'm not running codespell again, this is the old patch rebased. So the last pass will be to check that no new errors have crept in.

Attachment #9053986 - Flags: review?(geoff)
Assignee

Comment 11

3 months ago
Comment on attachment 9053986 [details] [diff] [review]
calendar/itip and calendar/lightning

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

::: calendar/lightning/content/lightning-menus.xul
@@ -382,5 @@
>                    observes="calendar_import_command"/>
>          <menuitem id="ltnPublishCalendar"
>                    label="&calendar.publish.label;"
>                    accesskey="&calendar.publish.accesskey;"
> -                  commmand="calendar_publish_calendar_command"

So what did or didn't work here? Any idea?
Attachment #9053975 - Flags: review?(geoff) → review+
Attachment #9053986 - Flags: review?(geoff) → review+

Well that's weird. It has a commmand attribute, but an overlay adds oncommand="goDoCommand('calendar_publish_calendar_command')".

No, that's not right. Yes it is, the oncommand attribute is copied from the <command> because of the observes attribute. XUL is hard. :-(

Assignee

Comment 13

3 months ago

Well, should the misspelled commmmmmand just be removed?

Flags: needinfo?(geoff)

Comment 14

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6483318f4089
fix typos in calendar/itip and calendar/lightning using codespell. r=darktrojan DONTBUILD

No, just fix the spelling, which you have already.

Flags: needinfo?(geoff)
Assignee

Comment 16

3 months ago

Plenty of new errors in non-base files :-(

Attachment #9054576 - Flags: review?(geoff)
Assignee

Comment 17

3 months ago
Posted patch 1480919-cal-base.patch (obsolete) — Splinter Review

That fixes everything apart from libical and ical.js.

Attachment #9054578 - Flags: review?(geoff)
Comment on attachment 9054576 [details] [diff] [review]
1480919-recheck-no-base.patch

> dependendent

Hah! :-)
Attachment #9054576 - Flags: review?(geoff) → review+
Comment on attachment 9054578 [details] [diff] [review]
1480919-cal-base.patch

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

A few things still to fix in this one.

::: calendar/base/content/calendar-common-sets.xul
@@ +203,5 @@
>          </menupopup>
>        </menu>
>        <menuseparator id="calendar-menuseparator-before-delete"/>
>        <!-- the label and accesskey of the following menuitem is set during runtime,
> +           and depends on whether the item is a task or an event-->

Please add a space before the --> while you're here. I should ask you to turn all these comments in to proper sentences with capital letters and full stops, but that's not what you're here for.

::: calendar/base/content/calendar-dnd-listener.js
@@ +394,1 @@
>          // notification to partticipants if required

partticipants?

::: calendar/base/content/calendar-item-editing.js
@@ +752,5 @@
>              }
>              let attendee = getParticipant(oldItem);
>              if (attendee) {
>                  // skip this item if the partstat for the participant hasn't
> +                // changed. otherwise we would always perform updade operations

updade?

Did you run codespell again after making these changes?

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +165,1 @@
>              // in order to visit each minimonth in turn.

This still doesn't make sense. "iterate through all" perhaps? Or just cut out the middle and go for "Now visit each minimonth in turn."
Attachment #9054578 - Flags: review?(geoff) → review+

Comment 20

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe9337e031b8
fix new typos in calendar/ except calendar/base using codespell. r=darktrojan
Assignee

Updated

3 months ago
Keywords: leave-open
Assignee

Comment 21

3 months ago
Attachment #9054578 - Attachment is obsolete: true
Attachment #9054674 - Flags: review+
Assignee

Comment 22

3 months ago

Calendar comments frequently/mostly are not English sentences starting with upper case and ending with a full stop, not even new comments being added, and they pass review.

I ran codespell again and it didn't pick up "updade" and "partticipants". So I fixed it manually together with " -->" and "iterate".

Comment 23

3 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5005f4ee1063
fix typos in calendar/base using codespell. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Assignee

Updated

3 months ago
Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.