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 Desktop
Packages, which is subscribed to thunderbird in Ubuntu.
https://bugs.launchpad.net/bugs/237341
Title:
mozilla-thunderbird locates 2/6/2008 as last week in 4/6/2008
Status in Mozilla Thunderbird Mail and News:
Confirmed
Status in “thunderbird” package in Ubuntu:
Triaged
Bug description:
Binary package hint: mozilla-thunderbird
Hi,
Thunderbird version 2.0.0.14 (20080502)
Folowing this menus (Translating from my menus localiced in ES_es)
View => Order => Desc.
View => Order => Group
Today is wensday 04/jun/2008, and monday 04/jun/2008 appears in last
week.
ProblemType: Bug
Architecture: i386
Date: Wed Jun 4 14:46:46 2008
Dependencies:
DistroRelease: Ubuntu 7.10
NonfreeKernelModules: vmnet vmmon
Package: mozilla-thunderbird None
PackageArchitecture: all
SourcePackage: thunderbird
Uname: Linux Pesc11-128 2.6.22-14-generic #1 SMP Tue Feb 12 07:42:25 UTC 2008
i686 GNU/Linux
To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/237341/+subscriptions
--
Mailing list: https://launchpad.net/~desktop-packages
Post to : desktop-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~desktop-packages
More help : https://help.launchpad.net/ListHelp
