Comment on attachment 8337301
fix bucket groups to be based off of a calendar week concept
Review of attachment 8337301:
-----------------------------------------------------------------
This isn't a full review; just a quick pass. David Bienvenu is unlikely
to be super-response, since I believe he works at Google now. I've
redirected the review to someone who might be a better pick (sorry if I
redirected wrong!).
::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +178,3 @@
> lastWeek=Last Week
> twoWeeksAgo=Two Weeks Ago
> older=Older
We might need to change the names of these strings to force localizers
to update them; after this patch, lastWeek doesn't refer to the same
thing. A localizer might have translated that to something like "Within
the last 7 days", which is accurate for how the code works pre-patch,
but would be wrong post-patch.
::: mailnews/base/src/nsMsgGroupView.cpp
@@ -135,5 @@
> - int64_t GMTLocalTimeShift = currentExplodedTime.tm_params.tp_gmt_offset +
> - currentExplodedTime.tm_params.tp_dst_offset;
> - GMTLocalTimeShift *= PR_USEC_PER_SEC;
> - currentTime += GMTLocalTimeShift;
> - dateOfMsg += GMTLocalTimeShift;
Why'd you remove the time-shifting? Isn't that important, since we want
to figure out when midnight is in local time, not GMT?
@@ +135,5 @@
> + // the localization for first day of calendar
> + int64_t todayMidnight = currentTime - currentTime % PR_USEC_PER_DAY;
> + int64_t yesterday = todayMidnight - PR_USEC_PER_DAY;
> + int64_t thisWeek = todayMidnight - (PR_USEC_PER_DAY *
> + (currentExplodedTime.tm_wday + 0));
Why the "+ 0" here?
@@ +137,5 @@
> + int64_t yesterday = todayMidnight - PR_USEC_PER_DAY;
> + int64_t thisWeek = todayMidnight - (PR_USEC_PER_DAY *
> + (currentExplodedTime.tm_wday + 0));
> + int64_t lastWeek = thisWeek - (PR_USEC_PER_DAY * 7);
> + int64_t twoWeeks = lastWeek - (PR_USEC_PER_DAY * 7);
Nit: I'd call this lastTwoWeeks to be clearer.
@@ +778,5 @@
> if (m_kOldMailString.IsEmpty())
>
> m_kOldMailString.Adopt(GetString(NS_LITERAL_STRING("older").get()));
> aValue.Assign(m_kOldMailString);
> break;
> + case Invalid:
I don't think we really need to add this here; the default case will
catch it.
::: mailnews/base/src/nsMsgGroupView.h
@@ +46,5 @@
> virtual void InternalClose();
> nsMsgGroupThread *AddHdrToThread(nsIMsgDBHdr *msgHdr, bool *pNewThread);
> virtual nsresult HashHdr(nsIMsgDBHdr *msgHdr, nsString& aHashKey);
> +
> + enum AgeBucket_t { Invalid, Today, Yesterday, ThisWeek, LastWeek,
> TwoWeeksAgo, Older };
I think it would make more sense to put this at the top of the
protected: list, and also to put each value on its own line.
I'm not sure what Mozilla's standard for naming enums is; I don't think
it's Foo_t though.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/237341
Title:
mozilla-thunderbird locates 2/6/2008 as last week in 4/6/2008
To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/237341/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
