[Bug 237341]

[Jim]

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 Mozilla
Bugs, 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

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/237341/+subscriptions