The default bug view has changed. See this FAQ.

Use nsMsgFolderFlags::SpecialUse constant instead of enumerating all the individual values at various places

RESOLVED FIXED in Thunderbird 20.0


MailNews Core
4 years ago
4 years ago


(Reporter: aceman, Assigned: aceman)


Thunderbird 20.0

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

10.91 KB, patch
: review+
Details | Diff | Splinter Review


4 years ago
There already exists a separate constant of SpecialUse at that covers all the special folder flags. There are still some places that do not use it but instead test against all the individual constants. E.g.:

This one is different but Inbox can be excluded from SpecialUse:

Converting them may future proof the spots. Or is the enumeration open-coded intentionally?


4 years ago
Flags: needinfo?
As far as I know it's not open coded on purpose, though I'd like to hear from Standard8. I've made similar changes in a couple of places.
Flags: needinfo? → needinfo?(mbanner)
I think this was something that was just added to make the checks easier, but wasn't actually spread to existing instances. I think it'd be fine cleanup to change those instances.
Flags: needinfo?(mbanner)

Comment 3

4 years ago
Created attachment 684099 [details] [diff] [review]

I've added comments at the spots where I was not sure if the enumerated list really needs to be missing 1 of the special flags.
Attachment #684099 - Flags: review?(mbanner)


4 years ago
Comment on attachment 684099 [details] [diff] [review]

Review of attachment 684099 [details] [diff] [review]:

Generally looks fine, I think I've commented in all the places you wanted feedback on, so making this an f+

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +3725,5 @@
>                                                int32_t newValue)
>  {
>    if (aProperty == mFolderFlagAtom)
>    {
> +    // Previously nsMsgFolderFlags::Queue was not included here. Should it really be excluded?

Queue is really a special special folder, but its not clear to me the functions that get called from here would do the right thing, so I think we should explicitly exclude it for now, and if we need it later, it'll probably be easier to pick up on.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +1779,5 @@
>    bool isServer;
>    GetIsServer(&isServer);
> +  // nsMsgFolderFlags::Queue was previously excluded from the list of special

It can't actually exist on IMAP at the moment, but in any case, we wouldn't want it deletable.

::: mailnews/local/src/nsMovemailIncomingServer.cpp
@@ +106,5 @@
>      nsCOMPtr<nsIMsgLocalMailFolder> localFolder =
>          do_QueryInterface(rootFolder, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // Leave nsMsgFolderFlags::Queue included too?

Yep, that's fine.
Attachment #684099 - Flags: review?(mbanner) → feedback+

Comment 5

4 years ago
Created attachment 695195 [details] [diff] [review]
patch v2

Thanks for the answers. I updated the patch accordingly.
Attachment #684099 - Attachment is obsolete: true
Attachment #695195 - Flags: review?(mbanner)
Comment on attachment 695195 [details] [diff] [review]
patch v2

Review of attachment 695195 [details] [diff] [review]:

Looks good, thanks.
Attachment #695195 - Flags: review?(mbanner) → review+


4 years ago
Keywords: checkin-needed
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in before you can comment on or make changes to this bug.