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

Status

MailNews Core
Database
--
trivial
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 20.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

10.91 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
There already exists a separate constant of SpecialUse at http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgFolderFlags.idl#106 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.:
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#3347
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1102
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#1783
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsNoIncomingServer.cpp#62
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3IncomingServer.cpp#436

This one is different but Inbox can be excluded from SpecialUse:
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerUnixIntegration.cpp#668

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

Updated

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

Comment 3

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

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

Updated

4 years ago
Status: NEW → ASSIGNED
Comment on attachment 684099 [details] [diff] [review]
patch

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+
(Assignee)

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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ca7a389c26dc
Status: ASSIGNED → RESOLVED
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.