849525 - Clean up mailnews/subscribe.{js|xul}

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

I noticed a lot of \tabs and trailing whitespace in our version of subscribe.{js|xul} compared to Thunderbird. I've opened this bug to do two things:
1. Fix whitespace issues.
2. Port some minor tweaks from Thunderbird's version.

Comment 1

[Philip Chee]

Attachment: Part 1v1 fix whitespace.

requesting r+moa from module owner.

Comment 2

[Philip Chee]

> Created attachment 723086 [details] [diff] [review]
> Part 1v1 fix whitespace.

To clarify, this patch:
1. Fixes whitespace issues including \tabs, trailing whitespace, and
2. Some comment only fixes (punctuation, capitalization)

3. Bug 407956 changed the code but someone forgot to change the following comment.
> -    // if the "subscribed" atom is in the list of properties
> +    // If the "subscribed" string is in the list of properties

Comment 3

[Philip Chee]

Attachment: Patch v2.1 Port stuff from Thunderbird.

Part 2:

1. Adapt fix for TB Bug 441705 (SetUpServerMenu is no longer used).

2. Simplify code in function SubscribeOnLoad() without (I hope) having changed the logic.
2.1. Removed the try/catch block. I've no idea if this is still necessary.
2.2. Fold the remainder of SetUpServerMenu() into SubscribeOnLoad().

3. Port the fix from TB Bug 360510 (Make subsription checkboxes accessible).

> -                                    <treecell properties="Subscribed-?Subscribed Subscribable-?Subscribable"/>
> +                                    <treecell value="?Subscribed"
> +                                              properties="Subscribed-?Subscribed Subscribable-?Subscribable"/>

4. Also remove some useless dump() statments.

Comment 4

[Karsten Düsterloh]

(In reply to Philip Chee from comment #2)
> To clarify, this patch:
> 1. Fixes whitespace issues including \tabs, trailing whitespace, and

I'm not convinced that this is useful enough to justify the change history.
Do you have a striking reason apart from the code looking ugly currently?

I'd be okay with 2+3.

+    // If the "subscribed" string is in the list of properties
+    // we are subscribed.

Shouldn't that have a comma behind "properties", then?

Comment 5

[Karsten Düsterloh]

Comment on attachment 723092 [details] [diff] [review]
Patch v2.1 Port stuff from Thunderbird.

r=me by inspection.

Comment 6

[Philip Chee]

>> To clarify, this patch:
>> 1. Fixes whitespace issues including \tabs, trailing whitespace, and
> I'm not convinced that this is useful enough to justify the change history.

There isn't much Hg change history to worry about:
http://hg.mozilla.org/comm-central/filelog/93e89494abfd/suite/mailnews/subscribe.js

> Do you have a striking reason apart from the code looking ugly currently?
Well the \tabs make it difficult (for me) to figure out the indentation.

Also TB did some clean up in this file and others in 2007 see:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mail/base/content/subscribe.js&rev=1.10&mark=1.10
"remove tabs and some trailing white space from thunderbird's xul and js files. purely white space changes."

Which makes trying to diff the TB and SM versions to see what changes are significant rather more painful.

> +    // If the "subscribed" string is in the list of properties
> +    // we are subscribed.
> 
> Shouldn't that have a comma behind "properties", then?
Fixed locally.

Comment 7

[Philip Chee]

Attachment: Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr [check-in comment 9]

This patch:
Port some improvements from Thunderbird.
Comment only fixes (punctuation, capitalization).
Remove some useless dump() statments.

But doesn't have the whitespace changes in an earlier patch.

Carrying forward r+. Requesting moa/sr

Comment 8

[Karsten Düsterloh]

Comment on attachment 727563 [details] [diff] [review]
Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr [check-in comment 9]

(In reply to Philip Chee from comment #6)
> >> To clarify, this patch:
> >> 1. Fixes whitespace issues including \tabs, trailing whitespace, and
> > I'm not convinced that this is useful enough to justify the change history.
> 
> There isn't much Hg change history to worry about

Okay, indeed, granted.
So, if you really feel pain enough, feel free to whitespace-clean-up the entire file, but include correcting the bracing style to "own line" then as well.

moa=me, plus rs=me for the cleanup.

Comment 9

[Philip Chee]

Pushed
http://hg.mozilla.org/comm-central/rev/34a222f3e8cb (whitespace fixes)
http://hg.mozilla.org/comm-central/rev/d04c9f8a73ad (port Thunderbird improvements)

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Missed at least one occurrence of "row" -> "aRow":
http://mxr.mozilla.org/comm-central/source/suite/mailnews/subscribe.js#327

-> Trying to check a box results in a message in the error console:

Error: ReferenceError: row is not defined
Source File: chrome://messenger/content/subscribe.js
Line: 327

(Reported by Hartmut Figge on m.s.seamonkey)

Comment 11

[Philip Chee]

Attachment: Patch v3.1 aRow rs=bustage fix [check-in comment 11].

> Missed at least one occurrence of "row" -> "aRow":
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/subscribe.js#327
> 
> -> Trying to check a box results in a message in the error console:
> 
> Error: ReferenceError: row is not defined
> Source File: chrome://messenger/content/subscribe.js
> Line: 327
> 
> (Reported by Hartmut Figge on m.s.seamonkey)

Pushed a bustage fix. rs=bustage

http://hg.mozilla.org/comm-central/rev/0dc685246f2b