The default bug view has changed. See this FAQ.

Clean up mailnews/subscribe.{js|xul}

RESOLVED FIXED in seamonkey2.20

Status

SeaMonkey
MailNews: General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.20

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 723086 [details] [diff] [review]
Part 1v1 fix whitespace.

requesting r+moa from module owner.
Attachment #723086 - Flags: superreview?(mnyromyr)
Attachment #723086 - Flags: review?(mnyromyr)
(Assignee)

Comment 2

4 years ago
> 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
(Assignee)

Comment 3

4 years ago
Created attachment 723092 [details] [diff] [review]
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.
Attachment #723092 - Flags: review?(mnyromyr)

Comment 4

4 years ago
(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

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

r=me by inspection.
Attachment #723092 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 6

4 years ago
>> 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.
Flags: needinfo?(mnyromyr)
(Assignee)

Comment 7

4 years ago
Created attachment 727563 [details] [diff] [review]
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
Attachment #723086 - Attachment is obsolete: true
Attachment #723092 - Attachment is obsolete: true
Attachment #723086 - Flags: superreview?(mnyromyr)
Attachment #723086 - Flags: review?(mnyromyr)
Attachment #727563 - Flags: superreview?(mnyromyr)
Attachment #727563 - Flags: review+

Comment 8

4 years ago
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.
Attachment #727563 - Flags: superreview?(mnyromyr) → superreview+
Flags: needinfo?(mnyromyr)
(Assignee)

Comment 9

4 years ago
Pushed
http://hg.mozilla.org/comm-central/rev/34a222f3e8cb (whitespace fixes)
http://hg.mozilla.org/comm-central/rev/d04c9f8a73ad (port Thunderbird improvements)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → seamonkey2.20
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)
(Assignee)

Comment 11

4 years ago
Created attachment 745491 [details] [diff] [review]
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
Attachment #745491 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #745491 - Attachment description: Patch v3.1 aRow rs=bustage fix. → Patch v3.1 aRow rs=bustage fix [check-in comment 11].
(Assignee)

Updated

4 years ago
Attachment #727563 - Attachment description: Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr → Patch v2.2 Port stuff from Thunderbird. r=Mnyromyr [check-in comment 9]
You need to log in before you can comment on or make changes to this bug.