Closed Bug 1025976 Opened 6 years ago Closed 4 years ago

Remove unused folder flag GotNew

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 48.0

People

(Reporter: rkent, Assigned: bogdan.stefan3, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

As far as I can tell, the folder flag GotNew is set and cleared in a few places in mailnews, but never actually used for anything. Rather than try to fix it or test it in bug 1025113, I'd like to just remove it. There are already too many other indicators of new messages floating around.
Blocks: 1025113
Mentor: mkmelin+mozilla
Whiteboard: [good first bug]
Attached patch Bug 1025976 patch (obsolete) — Splinter Review
I think I removed the flag completely.
Ran the unit tests successfully.

This is my first Thunderbird bug, is everything ok(the patch submission)?
Didn't look in detail yet, but one thing you should do is rev the uuid for the nsMsgFolderFlags.idl as it's changed.
(Generate a new uuid, on linux you can use the uuidgen command)

Then see http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed
Assignee: nobody → bogdan.stefan3
Attached patch 1025976 patch v2 (obsolete) — Splinter Review
How's this?

So every time a idl file is modified, i need to change its UUID?
Attachment #8728050 - Flags: review?(mkmelin+mozilla)
Attachment #8727996 - Attachment is obsolete: true
Comment on attachment 8728050 [details] [diff] [review]
1025976 patch v2

Review of attachment 8728050 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this looks good. r=mkmelin
Thx Bodgan!
Attachment #8728050 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
OS: Windows 7 → All
Hardware: x86_64 → All
Isn't the style in that file to NOT remove unused flags, but rename them to UnusedX ?
Status: NEW → ASSIGNED
Flags: needinfo?(rkent)
(In reply to :aceman from comment #5)
> Isn't the style in that file to NOT remove unused flags, but rename them to
> UnusedX ?

Yes that would be preferable.
Flags: needinfo?(rkent)
Bogdan, please update the patch to just rename the GotNew to Unused7 (or what number comes next) and update the comment above it to say what that constant was used for in the past. See Unused4 above.
It is so that we do not erroneosly reuse that flag value for other purposes. Existing folders may still contain that value it we just need to ignore it and not suddenly make it mean something else.
Flags: needinfo?(bogdan.stefan3)
Keywords: checkin-needed
Attached patch 1025976 patch (obsolete) — Splinter Review
Here it is. Sorry for the mistake.
Attachment #8728050 - Attachment is obsolete: true
Flags: needinfo?(bogdan.stefan3)
Attachment #8735100 - Flags: review?(acelists)
Comment on attachment 8735100 [details] [diff] [review]
1025976 patch

Review of attachment 8735100 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, thanks. I haven't run the tests but only checked the change in nsMsgFolderFlags.idl. I assume Magnus did review it fully for the first r+.

::: mailnews/base/public/nsMsgFolderFlags.idl
@@ +76,5 @@
>    /// This used to be used for virtual newsgroups
>    const nsMsgFolderFlagType Unused1         = 0x00008000;
>    /// Used to be for categories
>    const nsMsgFolderFlagType Unused4         = 0x00010000;
> +  /// Used to be for new msgs in a folder    

Please remove the trailing whitespace you add here.
Attachment #8735100 - Flags: review?(acelists) → review+
Attached patch 1025976 patchSplinter Review
Ugh, Atom did it.

Well I've run the tests the first time and they are fine.
I guess Magnus did it too.
Attachment #8735100 - Attachment is obsolete: true
Attachment #8735111 - Flags: review?(acelists)
Comment on attachment 8735111 [details] [diff] [review]
1025976 patch

Review of attachment 8735111 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #8735111 - Flags: review?(acelists) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
You need to log in before you can comment on or make changes to this bug.