Closed Bug 436044 Opened 16 years ago Closed 15 years ago

nsMsgFolderFlags.h should be an idl file

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: standard8, Assigned: neil)

References

Details

Attachments

(9 files, 1 obsolete file)

http://mxr.mozilla.org/seamonkey/source/mailnews/base/public/nsMsgFolderFlags.h should be an idl file. All the items it #defines are numbers, these can be constants in an interface. This will prevent us needing to do things like:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/base/content/commandglue.js&rev=1.91&mark=57-67#57

(/* keep in sync with nsMsgFolderFlags.h */)
Blocks: 436051
This doesn't convert any of the callers - someone else can do that ;-)
Attachment #323270 - Flags: superreview?(dmose)
Attachment #323270 - Flags: review?(bugzilla)
Oh, and the funny comments are apparently what doxygen works best with.
Comment on attachment 323270 [details] [diff] [review]
Just the "rename"

+/** Flags about a folder or a newsgroup.  Used in the MSG_FolderLine struct;
+ *  also used internally in libmsg (the `flags' slot in MSG_Folder).  Note that
+ *  these don't have anything to do with the above MSG_FLAG flags; they belong
+ *  to different objects entirely.
+ */

I know this is copy and paste, but this comment is incorrect and doesn't really make sense.

Firstly we don't have a MSG_FolderLine struct anymore, secondly, there are no MSG_FLAG flags above, maybe that bit should reference the ones in MailNewsTypes.h? (another candidate for this switch).

+  const unsigned long Newsgroup       = 0x00000001;

Why not typedef these like we do in other places (e.g. MailNewsTypes2.idl)?

+  /** This folder is a newsgroup folder. */

For single line comments like this, does doxygen cope with just // ?

+%{C++
+
+#define MSG_FOLDER_FLAG_NEWSGROUP        nsMsgFolderFlags::Newsgroup

Are these intended just until we've swapped all the callers?
(In reply to comment #3)
>(From update of attachment 323270 [details] [diff] [review])
>>+/** Flags about a folder or a newsgroup.  Used in the MSG_FolderLine struct;
>I know this is copy and paste, but this comment is incorrect
OK, I trimmed the comment to the first sentence.

>>+  const unsigned long Newsgroup       = 0x00000001;
>Why not typedef these like we do in other places (e.g. MailNewsTypes2.idl)?
They're now typedef unsigned long nsMSgFolderFlagType;

>>+  /** This folder is a newsgroup folder. */
>For single line comments like this, does doxygen cope with just // ?
No, but maybe /// works - asking Joshua Cranmer (who originally doxygenned it).

>>+%{C++
>Are these intended just until we've swapped all the callers?
Right, they're to aid transition.
(In reply to comment #4)
> >>+  /** This folder is a newsgroup folder. */
> >For single line comments like this, does doxygen cope with just // ?
> No, but maybe /// works - asking Joshua Cranmer (who originally doxygenned it).

Use whichever you prefer then.

> >>+%{C++
> >Are these intended just until we've swapped all the callers?
> Right, they're to aid transition.

Can we have an XXX comment in there please then? (although I guess I may follow up with the transition fairly quick).
Fixed as per my previous reply.
Comment on attachment 323270 [details] [diff] [review]
Just the "rename"

r=me with the new idl file.
Attachment #323270 - Flags: review?(bugzilla) → review+
Comment on attachment 323270 [details] [diff] [review]
Just the "rename"

rs=dmose
Attachment #323270 - Flags: superreview?(dmose) → superreview+
I've checked in my attachments, so now we can update the callers.
So, the flags only appear to be used in this one test.
Attachment #325988 - Flags: review?(bugzilla)
This was almost search-and-replace.
Attachment #326051 - Flags: superreview?(bienvenu)
Attachment #326051 - Flags: review?(bienvenu)
Attachment #326051 - Flags: superreview?(bienvenu)
Attachment #326051 - Flags: superreview+
Attachment #326051 - Flags: review?(bienvenu)
Attachment #326051 - Flags: review+
Attached patch Odds and endsSplinter Review
Asking Mnyromyr for review to "test the waters" regarding JS changes ;-)
Asking bienvenu for rs for the trivial comment changes.
Attachment #326215 - Flags: superreview?(bienvenu)
Attachment #326215 - Flags: review?(mnyromyr)
Attachment #325988 - Flags: review?(bugzilla) → review+
Reassigning to Neil, seeing as he seems to have got there before me.
Assignee: bugzilla → neil
Comment on attachment 326215 [details] [diff] [review]
Odds and ends

rs=bienvenu for comment changes.
Attachment #326215 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 326215 [details] [diff] [review]
Odds and ends

r=me for starters.
Attachment #326215 - Flags: review?(mnyromyr) → review+
Attachment #328689 - Flags: review?(mnyromyr)
Comment on attachment 328689 [details] [diff] [review]
mailnews/base/resources/content

While this is kind of traditional:

>+  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;

this definitely should have the k prefix:

>+        const Virtual = Components.interfaces.nsMsgFolderFlags.Virtual;

r=me with that.
Attachment #328689 - Flags: review?(mnyromyr) → review+
Product: Core → MailNews Core
Depends on: 455549
Attached patch Queye?Splinter Review
Those long lines are a bitch to review, aren't they?
Attachment #352996 - Flags: superreview?(neil)
Attachment #352996 - Flags: review?(neil)
Comment on attachment 352996 [details] [diff] [review]
Queye?

It sucks that we don't get JS strict warnings for these any more :-(
Attachment #352996 - Flags: superreview?(neil)
Attachment #352996 - Flags: superreview+
Attachment #352996 - Flags: review?(neil)
Attachment #352996 - Flags: review+
Attached patch Several more (obsolete) — Splinter Review
Having just corrected someone else's typo, I'm nearly certain to have made one of my own, but it's not popping out at me from a "careful" examination at 2am.
Attachment #353004 - Flags: superreview?(neil)
Attachment #353004 - Flags: review?(bugzilla)
Comment on attachment 353004 [details] [diff] [review]
Several more

This is mail/ code, so not my area :-)

>-            const outFolderFlagMask = MSG_FOLDER_FLAG_SENTMAIL |
>-              MSG_FOLDER_FLAG_DRAFTS | MSG_FOLDER_FLAG_QUEUE |
>-              MSG_FOLDER_FLAG_TEMPLATES;
>+            const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
>+            const outFolderFlagMask = nsMsgFolderFlags.SentMail |
>+              nsMsgFolderFlags.Drafts | nsMsgFolderFlags.Queue |
>+              nsMsgFolderFlags.Templates;
Is the 4-space continuation indent an if () thing?

>-            if (aFolder.flags  & MSG_FOLDER_FLAG_VIRTUAL)
>+            if (aFolder.flags  & Components.interfaces.nsMsgFolderFlags.Virtual)
Might want to kill that double space before & ;-)
Attachment #353004 - Flags: superreview?(neil)
Mostly mail, which is why I didn't have you r, but the patch turns suite at the end.

Is 4-space continuation indent *4*-space, or the file's normal indent, or double the file's normal indent? I looked through everyone's JS style guides the other month, and I don't think I found any mention.

Personally, I found the existing whitespace of that same set of flags two hunks up the patch far far worse (when I first saw it, I thought I must be seeing a tab).
Attachment #353004 - Flags: superreview+
Comment on attachment 353004 [details] [diff] [review]
Several more

>-    var inboxFolder = rootMsgFolder.getFolderWithFlags(0x1000);
Ah right, I forgot to look for numeric values...

>-          // Sigh... why aren't these in an IDL somewhere public?
Aww, 1 day too early ;-)

>-                var inboxFolder = rootMsgFolder.getFolderWithFlags(0x1000);
I see I forgot to look for numeric values (I should have switched to IDL constants first and then changed the method signatures, which would have forced me to check). This case is especially bad since I later changed both the previous and following lines without noticing...

>-const MSG_FOLDER_FLAG_TRASH = 0x0100;
Ah yes, this was in mail/ at the time so I ignored it. Thanks for picking it up.

>-            if (aFolder.flags  & MSG_FOLDER_FLAG_VIRTUAL)
We've obviously never used this binding in suite but maybe someone will want to port whatever feature of yours it is that uses it.
Comment on attachment 353004 [details] [diff] [review]
Several more

I thought that looked like an easy patch. Unfortunately it doesn't work. I've only started up TB so far and go the following errors, and the UI isn't working well:

JavaScript error: chrome://messenger/content/commandglue.js, line 779: redeclaration of const nsMsgFolderFlags
JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 1230: gMsgFolderSelected is not defined
JavaScript error: chrome://glodaquilla/content/glodaquillaOverlay.js, line 106: gDBView is not defined
JavaScript error: chrome://messenger/content/msgMail3PaneWindow.js, line 197: gMsgFolderSelected is not defined
JavaScript error: chrome://messenger/content/msgMail3PaneWindow.js, line 197: gMsgFolderSelected is not defined
JavaScript error: chrome://messenger/content/msgMail3PaneWindow.js, line 280: viewDebug is not defined

>+  const kVirtualFlag = Components.interfaces.nsMsgFolderFlags.Virtual;
>+  ShowMenuItem("folderPaneContext-compact", (numSelected <=1) && folder.canCompact && !(folder.flags & kVirtualFlag));

nit: please add a space after the <=

>+  {
>+    const kFavoriteFlag = Components.interfaces.nsMsgFolderFlags.Favorite;
>+    document.getElementById(menuItemId)
>+            .setAttribute('checked',folder.getFlag(kFavoriteFlag));
>+  }

nit: space after the comma please.
Attachment #353004 - Flags: review?(bugzilla) → review-
Comment on attachment 353004 [details] [diff] [review]
Several more

>+        const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
>+        if (msgFolder == msgWindow.openFolder &&
>+            !(folderFlags & nsMsgFolderFlags.Virtual) &&
>+            !(gPrevFolderFlags & nsMsgFolderFlags.Virtual))
>         {
>             return;
>         }
>         else
>         {
>+            const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
Ah yes, this would be the duplicate declaration.
Attached patch Less brokenSplinter Review
Oof, breaking commandglue does break things. I even know why I did, because I remember staring in horror at that giant else-after-return with the four-space indent, and deciding to just run away, but unless that was the start of my spell of build-SeaMonkey-test-unchanged-Thunderbird, I don't know why I attached it after I did it.
Attachment #353004 - Attachment is obsolete: true
Attachment #353950 - Flags: review?(bugzilla)
Attachment #353950 - Flags: review?(bugzilla) → review+
Attached patch One moreSplinter Review
Where by "done now?" I mean "will we be done once I get this one last one that was hiding behind the refrigerator?"
Attachment #354440 - Flags: superreview?(neil)
Attachment #354440 - Flags: review?(neil)
Attachment #354440 - Flags: superreview?(neil)
Attachment #354440 - Flags: superreview+
Attachment #354440 - Flags: review?(neil)
Attachment #354440 - Flags: review+
Comment on attachment 354440 [details] [diff] [review]
One more

Thanks for spotting this one.
No longer depends on: 455549
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
You need to log in before you can comment on or make changes to this bug.