nsMsgMessageFlags.h should be an idl file

RESOLVED FIXED in Thunderbird 3.0b3

Status

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: rain1, Assigned: rain1)

Tracking

Trunk
Thunderbird 3.0b3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

This bug is in the spirit of bug 436044. Even though the message flags seem to be less used than the folder flags in JS, there's really no reason not to.

JS files with MSG_FLAG_*: http://mxr.mozilla.org/mozilla/search?string=MSG_FLAG_&find=^.*\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla
(there might be more instances, where the value is directly used)
Product: Core → MailNews Core
Ugh, duplicate definitions are a pain. :(
Assignee: nobody → sid.bugzilla
Status: NEW → ASSIGNED
Attachment #356948 - Flags: superreview?(neil)
Attachment #356948 - Flags: review?(bienvenu)
Attachment #356948 - Flags: superreview?(neil) → superreview+
Attachment #356948 - Flags: review?(bienvenu) → review+
Keywords: checkin-needed
A bit of bitrot is fixed.
Attachment #356948 - Attachment is obsolete: true
Attachment #357114 - Flags: superreview+
Attachment #357114 - Flags: review+
Comment on attachment 357114 [details] [diff] [review]
[checked in] Updated patch to fix bitrot

Checked in: http://hg.mozilla.org/comm-central/rev/c263cff9191d
Attachment #357114 - Attachment description: Updated patch to fix bitrot → [checked in] Updated patch to fix bitrot
Posted patch Step 2: replace C++ users (obsolete) — Splinter Review
Pretty large patch, but just a bunch of global search and replace operations.
Attachment #357665 - Flags: superreview?(bugzilla)
Attachment #357665 - Flags: review?(bugzilla)
Comment on attachment 357665 [details] [diff] [review]
Step 2: replace C++ users

My Mac build fails with:

/Users/moztest/comm/main/src/mailnews/import/applemail/src/nsEmlxHelperUtils.mm: In static member function ‘static nsresult nsEmlxHelperUtils::ConvertToMozillaStatusFlags(const char*, const char*, PRUint32*)’:
/Users/moztest/comm/main/src/mailnews/import/applemail/src/nsEmlxHelperUtils.mm:80: error: ‘MSG_FLAG_READ’ was not declared in this scope
/Users/moztest/comm/main/src/mailnews/import/applemail/src/nsEmlxHelperUtils.mm:82: error: ‘MSG_FLAG_FORWARDED’ was not declared in this scope
/Users/moztest/comm/main/src/mailnews/import/applemail/src/nsEmlxHelperUtils.mm:84: error: ‘MSG_FLAG_REPLIED’ was not declared in this scope
/Users/moztest/comm/main/src/mailnews/import/applemail/src/nsEmlxHelperUtils.mm:86: error: ‘MSG_FLAG_MARKED’ was not declared in this scope
make[4]: *** [nsEmlxHelperUtils.o] Error 1
make[3]: *** [applemail/src_libs] Error 2
make[2]: *** [import_libs] Error 2
make[1]: *** [libs_tier_app] Error 2
make: *** [tier_app] Error 2


-    if (!(flags & MSG_FLAG_READ)) unreadDelta++;
-    if (flags & MSG_FLAG_NEW) newDelta++;
+    if (!(flags & nsMsgMessageFlags::Read)) unreadDelta++;
+    if (flags & nsMsgMessageFlags::New) newDelta++;

Please put the variable increments on a new line.

-    if (unreadDelta == -1 && aOldFlags & MSG_FLAG_NEW)
+    if (unreadDelta == -1 && aOldFlags & nsMsgMessageFlags::Read)

This doesn't look right...

-    case MSG_FLAG_FORWARDED|MSG_FLAG_REPLIED:
+    case nsMsgMessageFlags::Forwarded|nsMsgMessageFlags::Replied:

nit: space either side of the | please.

   virtual void	UpdateFolderFlag(nsIMsgDBHdr *msgHdr, PRBool bSet, 
-                                  MsgFlags flag, nsIOutputStream **ppFileStream);
-  virtual PRBool SetHdrFlag(nsIMsgDBHdr *msgHdr, PRBool bSet, MsgFlags flag);
+                                 nsMsgMessageFlagType flag, nsIOutputStream **ppFileStream);

nit: nsMsgMessageFlagType should align with the start of nsIMsgDBHdr on the line above.

 // IMAP does not set local file flags, override does nothing
 void	nsImapMailDatabase::UpdateFolderFlag(nsIMsgDBHdr * /* msgHdr */, PRBool /* bSet */,
-                                              MsgFlags /* flag */, nsIOutputStream ** /* ppFileStream */)
+                                           nsMsgMessageFlagType /* flag */, nsIOutputStream ** /* ppFileStream */)

Can you drop the tab in the UpdateFolderFlag line so that your new line aligns properly please.

         PRUint32 msgFlags = 0;
         qhdr->GetFlags(&msgFlags);
-        if ( msgFlags & MSG_FLAG_OFFLINE )
+        if ( msgFlags & nsMsgMessageFlags::Offline )
           continue;

nit: drop the extra spaces please.

-      PRUint32 mask = MSG_FLAG_READ | MSG_FLAG_REPLIED | MSG_FLAG_MARKED | MSG_FLAG_IMAP_DELETED | MSG_FLAG_LABELS;
+      PRUint32 mask = nsMsgMessageFlags::Read | nsMsgMessageFlags::Replied | nsMsgMessageFlags::Marked | nsMsgMessageFlags::IMAPDeleted | nsMsgMessageFlags::Labels;

nit: this line needs wrapping.
Attachment #357665 - Flags: superreview?(bugzilla)
Attachment #357665 - Flags: review?(bugzilla)
Attachment #357665 - Flags: review-
Posted patch Step 2, revision 2 (obsolete) — Splinter Review
Nits fixed, and bustage hopefully fixed as well.
Attachment #357665 - Attachment is obsolete: true
Attachment #358258 - Flags: superreview?(bugzilla)
Attachment #358258 - Flags: review?(bugzilla)
Attachment #358258 - Flags: superreview?(bugzilla)
Attachment #358258 - Flags: superreview+
Attachment #358258 - Flags: review?(bugzilla)
Attachment #358258 - Flags: review+
Comment on attachment 358258 [details] [diff] [review]
Step 2, revision 2

> nsresult nsMsgDBView::FetchStatus(PRUint32 aFlags, nsAString &aStatusString)
> {
>-  if(aFlags & MSG_FLAG_REPLIED)
>+  if(aFlags & nsMsgMessageFlags::Replied)
>     aStatusString = kRepliedString;
>-  else if(aFlags & MSG_FLAG_FORWARDED)
>+  else if(aFlags & nsMsgMessageFlags::Forwarded)
>     aStatusString = kForwardedString;
>-  else if(aFlags & MSG_FLAG_NEW)
>+  else if(aFlags & nsMsgMessageFlags::New)
>     aStatusString = kNewString;
>-  else if(aFlags & MSG_FLAG_READ)
>+  else if(aFlags & nsMsgMessageFlags::Read)
>     aStatusString = kReadString;

nit: please can you insert spaces after the if.

>-  if (flags & MSG_FLAG_ELIDED || !(m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay) || !(flags & MSG_VIEW_FLAG_HASCHILDREN))
>-    return NS_OK;
>-  flags  |= MSG_FLAG_ELIDED;
>+  if (flags & nsMsgMessageFlags::Elided || !(m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay) || !(flags & MSG_VIEW_FLAG_HASCHILDREN))
>+    return NS_OK;
>+  flags  |= nsMsgMessageFlags::Elided;

nit: just one space after flags please.

>   PRInt32 numChildren = CountExpandedThread(index);
> 
>-  *expansionDelta = (flags & MSG_FLAG_ELIDED) ? 
>+  *expansionDelta = (flags & nsMsgMessageFlags::Elided) ? 

nit: please drop the extra space from the end of the line
Posted patch step 2, revision 3 (obsolete) — Splinter Review
more nits fixed
Attachment #358258 - Attachment is obsolete: true
Attachment #358836 - Flags: superreview+
Attachment #358836 - Flags: review+
Keywords: checkin-needed
oops, missed one if(
Attachment #358836 - Attachment is obsolete: true
Attachment #358837 - Flags: superreview+
Attachment #358837 - Flags: review+
Attachment #358836 - Attachment description: [to checkin] step 2, revision 3 → step 2, revision 3
Comment on attachment 358837 [details] [diff] [review]
[checked in] step 2, revision 4

Checked in: http://hg.mozilla.org/comm-central/rev/0e339aad00cf
Attachment #358837 - Attachment description: [to checkin] step 2, revision 4 → [checked in] step 2, revision 4
Posted patch JS usesSplinter Review
Surprising that it would be this easy after the variety of subtle ways we hid what we were using with nsMsgFolderFlags, but these are all I can find.
Attachment #368735 - Flags: superreview?(neil)
Attachment #368735 - Flags: review?(bugzilla)
Comment on attachment 368735 [details] [diff] [review]
JS uses

> function SelectedMessagesAreDeleted()
> {
>-    try {
>-        return gDBView.hdrForFirstSelectedMessage.flags & MSG_FLAG_IMAP_DELETED;
>-    }
>-    catch (ex) {
>-        return 0;
>-    }
>+    return gDBView && gDBView.numSelected &&
>+           (gDBView.hdrForFirstSelectedMessage.flags &
>+            Components.interfaces.nsMsgMessageFlags.IMAPDeleted);
> }
Thanks ;-)
Attachment #368735 - Flags: superreview?(neil) → superreview+
Attachment #368735 - Flags: review?(bugzilla) → review+
http://hg.mozilla.org/comm-central/rev/ceb88f1ecea4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
You need to log in before you can comment on or make changes to this bug.