436044 - nsMsgFolderFlags.h should be an idl file

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

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 */)

Comment 1

[[e-mail]]

Attachment: Just the "rename"

This doesn't convert any of the callers - someone else can do that ;-)

Comment 2

[[e-mail]]

Oh, and the funny comments are apparently what doxygen works best with.

Comment 3

[Mark Banner (:standard8)]

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?

Comment 4

[[e-mail]]

(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.

Comment 5

[Mark Banner (:standard8)]

(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).

Comment 6

[[e-mail]]

Attachment: Updated .idl file (not a diff)

Fixed as per my previous reply.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 323270 [details] [diff] [review]
Just the "rename"

r=me with the new idl file.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 323270 [details] [diff] [review]
Just the "rename"

rs=dmose

Comment 9

[[e-mail]]

I've checked in my attachments, so now we can update the callers.

Comment 10

[[e-mail]]

Attachment: Part 1 - fix tests

So, the flags only appear to be used in this one test.

Comment 11

[[e-mail]]

Attachment: Part 2 - C++ code

This was almost search-and-replace.

Comment 12

[[e-mail]]

Attachment: Odds and ends

Asking Mnyromyr for review to "test the waters" regarding JS changes ;-)
Asking bienvenu for rs for the trivial comment changes.

Comment 13

[Karsten Düsterloh]

Comment on attachment 326215 [details] [diff] [review]
Odds and ends

What about these: <http://mxr-test.landfill.bugzilla.org/seamonkey/search?string=MSG_FOLDER_FLAG_VIRTUAL&find=mailnews.*.js&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=seamonkey>, especially commandglue.js?
(And I didn't check any others of these constants.)

Comment 14

[Mark Banner (:standard8)]

Reassigning to Neil, seeing as he seems to have got there before me.

Comment 15

[David :Bienvenu]

Comment on attachment 326215 [details] [diff] [review]
Odds and ends

rs=bienvenu for comment changes.

Comment 16

[Karsten Düsterloh]

Comment on attachment 326215 [details] [diff] [review]
Odds and ends

r=me for starters.

Comment 17

[[e-mail]]

Attachment: mailnews/base/resources/content

Comment 18

[Karsten Düsterloh]

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.

Comment 19

[Phil Ringnalda (:philor)]

Attachment: Queye?

Those long lines are a bitch to review, aren't they?

Comment 20

[[e-mail]]

Comment on attachment 352996 [details] [diff] [review]
Queye?

It sucks that we don't get JS strict warnings for these any more :-(

Comment 21

[Phil Ringnalda (:philor)]

Attachment: Several more

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.

Comment 22

[[e-mail]]

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 & ;-)

Comment 23

[Phil Ringnalda (:philor)]

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).

Comment 24

[[e-mail]]

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 25

[Mark Banner (:standard8)]

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.

Comment 26

[[e-mail]]

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.

Comment 27

[Phil Ringnalda (:philor)]

Attachment: Less broken

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.

Comment 28

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/9a39789d1cad

We done now?

Comment 29

[Phil Ringnalda (:philor)]

Attachment: One more

Where by "done now?" I mean "will we be done once I get this one last one that was hiding behind the refrigerator?"

Comment 30

[[e-mail]]

Comment on attachment 354440 [details] [diff] [review]
One more

Thanks for spotting this one.

Comment 31

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/08c2b8f79bc3

Are we there yet? Are we?