Closed Bug 505981 Opened 10 years ago Closed 3 years ago

The "Last Week" sorting group label is misleading / wrong (Fix label only)

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(thunderbird51 fixed)

RESOLVED FIXED
Thunderbird 51.0
Tracking Status
thunderbird51 --- fixed

People

(Reporter: benoit.leseul, Assigned: alta88)

References

(Blocks 2 open bugs)

Details

(Keywords: polish, Whiteboard: [has l10n impact])

Attachments

(3 files, 3 obsolete files)

As discussed on the l10n newsgroup, the "Last Week" sorting group label in the message list is misleading. It doesn't mean the last calendar week but the last seven days.
http://groups.google.com/group/mozilla.dev.l10n/browse_thread/thread/c597551173c2703

The label could be changed to "Last seven days", or the grouping could be made on calendar weeks instead. John Wilcock proposed to split that in "Earlier this week" and "Last week" based on calendar weeks.
Flags: wanted-thunderbird3?
Keywords: polish
Benoit, since you filed that for Thunderbird and not SeaMonkey, it is covered
by bug 315114 already. Thus, I'd suggest that you either retarget the bug for
SeaMonkey as its counterpart or resolve as duplicate of the Thunderbird bug.
(In reply to comment #1)
> Benoit, since you filed that for Thunderbird and not SeaMonkey, it is covered
> by bug 315114 already.

I don't think that this bug is a dupe of bug 315114. Basically I consider the proposal in bug 315114 as very unintuitive and therefore as WONTFIX.
Duplicate of this bug: 315114
Well, at least you seem to agree that both bugs cover the same topic...  ;-)
Blocks: 386950
As I wrote in bug 315114 comment 12, at least to me the current groups aren't that useful, maybe we could come up with better ones.
I'd suggest that many people split up recent past time using a calendar-week paradigm:

I'd suggest the following categories, similar to those proposed in bug 315114:
• Today
• Yesterday
• Earlier this week (i.e. sometime between the beginning of the current calendar week and the day before yesterday). This category obviously would only exist if today is at least the third day of the week, bearing in mind that different locales start the week on different days. I think this would be more useful than a day-by-day view for earlier than today and yesterday, as by the time we get to, say, Friday, people no longer remember which day at the beginning of the week a message was received. 
• Last week (i.e. the previous calendar week, not the current usage)

For older messages I'm not sure which categories would be best, not even how useful it is to continue defining categories. One could argue for "Earlier this month" (where applicable) and "Last month", for example, or use month names in the same way as Firefox history - but IMO we don't want too fine a granularity here either.
(In reply to comment #6)

> • Earlier this week (i.e. sometime between the beginning of the current
> calendar week and the day before yesterday). 

I'm afraid that the resulting localized string would be awfully long for many languages.
(In reply to comment #7)
> (In reply to comment #6)
> 
> > • Earlier this week (i.e. sometime between the beginning of the current
> > calendar week and the day before yesterday). 
> I'm afraid that the resulting localized string would be awfully long for many
> languages.

Which wouldn't hurt much as there's space enough on the group header.
Summary: The "Last Week" sorting group label is misleading → The "Last Week" sorting group label is misleading / wrong
Whiteboard: [has l10n impact]
How about "Last 7 days", that's 12 characters in English, shorter than the next label "Two Weeks ago" (14 characters). Regardless this is a quick change and hopefully it can be rolled into the next release.
"Last 7 days" would indeed make the current groupings unambiguous (in English at least) and might be a desirable quick fix, but this bug (and to an even greater extent its duplicate bug 315114) has focused on alternative and hopefully more intuitive groupings based on calendar weeks.
My vote would be to make the groupings work logically by calendar week/month, ie: "last week" is the week previous to this one based on the user's selection in Views > "Start the week on".

I also wouldn't mind seeing John's suggestion of "Earlier this week" (or maybe just "This Week" for a shorter string), etc. It should really be based on the largest logical calendar block though, not just on number of days as it currently is.

..Although I wonder why it couldn't be more granular for old email, if done by breaking out more headings under "Old Mail"? ie (hopefully this displays):

[-] Old Mail
 |- [+] 2008
 |- [+] 2009
 |- [-] 2010
     |- [+] January
     |- [+] February
...etc...
I stumbled upon this because I moaned about this in the TB support group: http://groups.google.com/group/mozilla.support.thunderbird/browse_thread/thread/668d78f2962ab1cb

I quite like John Wilcock's suggestion in Comment 6.
Duplicate of this bug: 591345
Duplicate of this bug: 627655
Looking back at this relatively old bug, I wonder whether the lack of agreement on categories (and hence the lack of progress) might be at least partly because the preferred categorisation varies from locale to locale. None of those who complain about the existing categories seem to be English-language users, and that can't be entirely a coincidence. 

It would be perfectly possible in theory to define a structure whereby different locales can choose their own categorisation depending whether the mindset in their locale conceptualises a "week" as last-7-consecutive-days or as a calendar week. 

I've no idea, though, whether the current i18n/l10n structures allow for a choice like this. Maybe this would need to be a (hidden?) preference, with corresponding UI strings to be translated for both paradigms.
(In reply to comment #15)

> I've no idea, though, whether the current i18n/l10n structures allow for a
> choice like this. Maybe this would need to be a (hidden?) preference, with
> corresponding UI strings to be translated for both paradigms.

l10n has no control over this, so it would have to be a pref the localizations could set. But I'm pretty sure I've heard english-language user complain about this, if not in bugzilla, at least in Get Satisfaction issues.
It should be simple to define a logical structure.
A time period has a clearly defined start and end like a day that starts at 0:00 and ends on 23:59:59.999 (shortly before 0:00). If we would classify a time by hours as example 12:21 it would be "this hour", everithing in the range of 11:00...11:59 would be "one hour before" and then "earlier this day".
The same scheme applies to days. If we define the start of the week at Monday(like defined in Lightning) then we could have Thursday as "Today", Wednesday as "Yesterday", Monday and Tuesday as "Earlier this week". With the use of "Last week" we could solve any ordering related problem. If today would be Monday then Monday is "Today", Sunday is "Yesterday", Satterday is in the group "Last week". Everything before the last week could go to "Older Messages".
Sure we could do the same with months or even years ... but this could be overdrawn.
Flags: wanted-thunderbird3?
Duplicate of this bug: 941197
Attached is a first attempt patch to address this bug.
It aligns the buckets > yesterday to week boundaries, and adds a lastWeek bucket.

I need information on where to get the beginning of calendar week localization runtime information

Also, I did the English localization, but none of the others. I would need help with that.

This needs testers to validate that I didn't break something else.
Attachment #8337301 - Flags: review?(mozilla)
Comment on attachment 8337301 [details] [diff] [review]
fix bucket groups to be based off of a calendar week concept

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

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.
Attachment #8337301 - Flags: review?(mozilla) → review?(neil)
Oh, I should explain more about localization: you're not responsible for updating any of the strings except for en-US. Dedicated localizers will come in and update them for all the other locales. However, if we change the meaning of any string in the code, we generally need to change the name of the string too, since that forces the localizers to update the string. If the string name stays the same, they usually won't notice the change*.

* Yeah, our tools could use improvement in this department...
Thanks for the comments.

(In reply to Jim Porter (:squib) from comment #20)
> Comment on attachment 8337301 [details] [diff] [review]
> fix bucket groups to be based off of a calendar week concept
> 
> Review of attachment 8337301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 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!).

Thanks for pointing it to anyone who'll respond.

> 
> ::: 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.

noted. suggestions ?

> 
> ::: 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?

It didn't seem to work correctly.
With it, I had messages that were very near to the bucket boundaries, that sometimes, were on the wrong side of the bucket boundary.

Commented out, all my messages seemed to align correctly at the boundaries.

> 
> @@ +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?

You missed the line above. It's a place holder for the start of calendar week localization offset.

> 
> @@ +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.

Noted.

> 
> @@ +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.

Yeah, it was a Nit on my part, default aside, I recalled that some compilers complain if every instance of an enum is not handled, so I was being pro-active. :)

> 
> ::: 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.

Noted, I briefly looked for coding standard for enums, and couldn't find anything.

I'll wait for more comments before taking any more action.
I don't want to litter this bug with a bunch of back and forth.
Yeah, uncle, after further testing, the time zone adjustments go back in...
Comment on attachment 8337301 [details] [diff] [review]
fix bucket groups to be based off of a calendar week concept

I'm attaching an updated patch to address review comments by :squib
Attachment #8337301 - Attachment is obsolete: true
Attachment #8337301 - Flags: review?(neil)
Ok, I looked into the issue of how to determine the first day of the week. There's a localization file here, used for the datepicker[1] and there's also a pref in Lightning[2]. I'm pretty sure most OSes also specify this somewhere, but we don't detect that. I'm CCing one of the Lightning devs so that we can discuss the correct solution here. It might be useful to add support to Gecko to pull this in from the OS; that could be useful for HTML5 forms as well.

[1] http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global/datetimepicker.dtd
[2] http://mxr.mozilla.org/comm-central/source/calendar/locales/en-US/lightning-l10n.js#9
Flags: needinfo?(philipp)
ha, that is funny!
I've been hunting through the UI and code base to find this info, and I literally just finished a build with preliminary support to pull in the calendar.week.start preference, after which I started it up to find a new email with your last comment.

I do think that getting this from the OS with and override from the app is correct.
To further complicate things, on OS X, I do believe it should be initially influenced by the Calendar app settings, then by thunderbird mechanisms.

Thanks for staying with me.
(In reply to Jim Porter (:squib) from comment #26)
> It might be useful to add
> support to Gecko to pull this in from the OS; that could be useful for HTML5 forms as well.

Absolutely! It's increasingly annoying to get wrong start of week. Bug 164495 for calendar.
Indeed, there is bug 164495 for calendar. What you want to fix first is bug 333938 though. This will allow Lightning/Thunderbird to access the locale info. I'm not sure the first day of the week is available on all platforms, but from skimming that bug it seems windows has an API call for it and linux has nl_localeinfo. This could be available on mac too.

You can of course check if the Lightning pref exists and is set to ease integration for Lightning. The better solution would be to default to checking the OS and making the code extensible enough so that Lightning can hook in and change the behavior. If you have time to do the Lightning patch too that would be super :)
Flags: needinfo?(philipp)
Comment on attachment 8337339 [details] [diff] [review]
fix bucket groups to be based off of a calendar week concept

Sorry for the delay. The code seems reasonable but this needs to have ui-review before I can review it properly.

>+    // TODO - in the calculation for thisWeek, the + 0 should be based of off
>+    // the localization for first day of calendar
If only it were that easy...

As a side note I'd like to compare to Firefox's History sidebar which uses the following groupings:
> Today
> Yesterday
> Last 7 days
> This month (if today is the 8th or later)
> 5 entries, one for each previous month
> Older than 6 months

Out of interest, Outlook seems to be use the following groupings:
> Today
> Yesterday
> 5 entries, one for each previous day
> Last Week (7-13 days ago)
> Two Weeks Ago (14-20 days ago)
> Three Weeks Ago (21-27 days ago)
> Earlier This Month
> Last Month
> Older
What I don't know is whether these groups are based on the day of the week or fixed numbers of days.
Attachment #8337339 - Flags: review?(neil)
Attachment #8337339 - Attachment is obsolete: true
Attached patch lastWeek2.diff (obsolete) — Splinter Review
fix bucket groups to be based off of a calendar week concept.
pull firstDayOfWeek bias from lightning prefs if available.
Attachment #8355581 - Flags: review?(squibblyflabbetydoo)
I think before we do much with this bug, we either need to fix bug 333938 or at least move the "calendar.week.start" pref to Thunderbird. If we do the latter, we should write some migration code so that we copy over the old pref value to the new pref.
This bug has stalled because, I seem not to be able to get any real direct help, without infinite referrals, for 333938.
So, I'll opt for and take a stab at moving the pref, and hopefully make some headway.
Yeah, let's do the pref. That should be a reasonable workaround, and when bug 333938 is fixed, we can just switch over to that and everyone's happy.
Comment on attachment 8355581 [details] [diff] [review]
lastWeek2.diff

r- for now since we want to use a pref to handle the start-of-week.
Attachment #8355581 - Flags: review?(squibblyflabbetydoo) → review-
Can this be pushed to upstream repository? I'm quite surprised to learn that it has not been addressed for almost 5 years.  


(In reply to Jim Porter (:squib) from comment #35)
> Comment on attachment 8355581 [details] [diff] [review]
> lastWeek2.diff
> 
> r- for now since we want to use a pref to handle the start-of-week.
What we are seeing with TB 31.4.0 is this:

Today is Friday but "Last week" starts 7 days ago. That label should be "This Week".
Today is Friday but "Last 2 week" starts 14 days ago. That label should be "Last Week".


Willing to test fix when a fix is available.
Duplicate of this bug: 1296130
Attached patch groupDates.patchSplinter Review
The sole purpose of this patch is to fix the week based groupby bucket strings and confusion with what a 'week' means, ie to reflect what the code actually calculates (includes today). It addresses the original problem in comment 0.
Last Week -> Last 7 Days
Two Weeks Ago -> Last 14 Days

The enhancement of the group date buckets, as in comment 30, should be done elsewhere once a UI consensus is reached and calendar dependencies are sorted; better yet is a customizable grouping mechanism (bug 1179836). The original strings are left in case correct 'week' calculations are implemented.
Attachment #8782501 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8782501 [details] [diff] [review]
groupDates.patch

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

I'm trying to do a few drive-by reviews here. Thinking about it, the new labels don't help since they suggest one group being part of another which is not the case.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +200,2 @@
>  twoWeeksAgo=Two Weeks Ago
> +last14Days=Last 14 Days

That's not right, is it? Last 7 days is by definition a subset of last 14 days, but this is not what is meant here.

I think that yesterday also falls into the "last 7 days". So I think we need to find something better here.

Correct would be:
Yesterday = 1 day old.
"2 to 8 days old" = 7 days or one week before yesterday.
"9 to 15 days old".
Attachment #8782501 - Flags: review-
Hard to find some precise and easy to read language here.

How about:
Today
Yesterday
7 Days Before Yesterday
7 Days Before That
Older

Or:
Today
Yesterday
Up to 7 Days Old (incorrect)
Up to 14 Days Old (incorrect)
Older

Or:
Today
Yesterday
Up to 8 Days Old
Up to 15 Days Old
Older

The second/third option is nice, because you have a progression of old, old, older.
Also, although "Up to 7/8 Days Old" *is* a subset of "Up to 14/15 Days Old", from the context, people shouldn't search for a 5-day-old e-mail in the "Up to 14/15 Days Old" group.
Maybe best:
Today
Yesterday
Up to 8 Days Old
Up to 15 Days Old
Older

That 8 and 15 relates to one and two weeks is immediately clear. Also, there are languages like Spanish and French which use "15" to refer to two weeks: French: quinze jours, Spanish: quincena.
In terms of usability, it is counter intuitive to group email based on the concept of how many days ago.

We, as human being, would organize our works based on week. Like ship event (a.k.a software release), it is common in US defined by the format as 20W16 meaning ship in 20th week of 2016. If you are church people, you will start your day on Sunday after going to Church. So it is quite natural to segment/group things by weeks, instead of days.

I know there will be more work to implement this in week than in the number of days. But it is way much user friendly to group it by weeks, instead of the number of days.
Access to what the start of a week is is actually being worked on now (and partly landed) - bug 1287503. I think we should pick up that and just do the weekly think instead of 7days.
Agree that picking up the start of week would be most usable; ideally detected from local OS with option to override (per comment 27). But it's been over 7 years now on this bug!! Maybe we should go with alta88's suggestion (comment 39) and just clarify the strings one way or another - should be a very easy fix to push. After that, finish arguing about the usability and historical localization implications of various grouping enhancements...

Also, maybe we are overthinking the whole wording thing? For the quick fix, maybe any solution using the word "days" instead of "weeks" will suffice? Users have lived with the current labels for a long time; anything would be an improvement.

The underlying [linguistic] problem is simply that the word "week" carries ambiguous meaning, exacerbated by the fact that outlook uses "start-of-week". Just switch to something with the word "day" and this bug is technically closed while we sort out the long-term solution... I just don't want to end up in purgatory for another 7 years!
We add more code to change a bug into a useless feature. So why bother to waste our time typing?

Adding grouping is for improving usability. If "quick fix" doesn't address the goal, I'd rather leaving this alone or drop this grouping.
I suggested: Today, Yesterday, Up to 8 Days Old, Up to 15 Days Old, Older.
I that not a useful improvement to clarify the current grouping?
Agree that adding more code wouldn't be worthwhile; I thought that updating the existing strings/labels on existing grouping was the only thing needed for a quick patch.

As an end user who switched from outlook to thunderbird, the existing language (mirroring outlook but not working the same) was a little annoying - for a few days I couldn't find emails I was looking for until I figured out that the labels didn't mean what I thought.

Personally I wouldn't consider it a waste of time if TB developers just fixed the misleading nature of the labels. Of course more sophisticated grouping would be even better - that just doesn't seem like something that will get released nearly as fast.
TBH, When I switch from outlook to thunderbird, I found this annoying thing which caused me missed a couple of emails from work. In the end, I disabled the grouping.

Changing label leads to more work in localization. Considering the number of locale, the amount of changing label work is more than typing.

As comment 44 pointed out, bug 1287503 has been fixed. This problem should be able to solve now. But please just don't add how many days old. It is not a solution. I will still disable the grouping.
> ::: mail/locales/en-US/chrome/messenger/messenger.properties
> @@ +200,2 @@
> >  twoWeeksAgo=Two Weeks Ago
> > +last14Days=Last 14 Days
> 
> That's not right, is it? Last 7 days is by definition a subset of last 14
> days, but this is not what is meant here.

It certainly is. If you read comment 39 again, the string is to reflect the code. So 'what is meant here' is not clear, which is the problem.

> 
> I think that yesterday also falls into the "last 7 days". So I think we need
> to find something better here.
> 
> Correct would be:
> Yesterday = 1 day old.
> "2 to 8 days old" = 7 days or one week before yesterday.
> "9 to 15 days old".

This is incorrect, both in what the code calculates and what a bucket string should be, in en.

The code calculates the "Last week" bucket to mean "Within the last 7 days, starting with today (day 1)". "Two weeks ago" means "Within the last 14 days, starting with today (day 1)".

IMO it it better, shorter to say "Last 14 days" than "Between 8 and 14 days ago" or "Up to 14 days old". Using the word "ago" is better than "old".  It is obvious an earlier date will go in an earlier bucket, if it fits.

Finally, to determine the en-US string, it is required that it be decided by a native en-US speaker, in the same way localizations are done by native speakers for the locale.  This is obvious.  It is ridiculous to give any weight to, for example, common french usage for 2 weeks as it is 100% irrelevant to a non fr locale.  Whether 'days' or 'weeks' are useful is locale specific.

rkent, wsm - please provide the definitive feedback here.
Attachment #8782501 - Flags: review?(mkmelin+mozilla)
Attachment #8782501 - Flags: review-
Attachment #8782501 - Flags: feedback?(vseerror)
Attachment #8782501 - Flags: feedback?(rkent)
And regarding weeks: it will continue to be confusing, even if the code is changed to adjust for OS based week start day and so forth, even within the en locale.  That is why Firefox library history uses "Last 7 days".  I really suggest we go with the patch and finish this nuisance bug off.
If StartOfWeek is actually landed, #30 fixes all of this, based off of StartOfWeek.

see #20 "(currentExplodedTime.tm_wday + 0));" --> "(currentExplodedTime.tm_wday + StartOfWeek)); // StartOfWeek is zero relative"

Hell I'll redo the work, just to have it actually reflect the labels, if asked!


Regards
(In reply to Neal horman from comment #52)
> If StartOfWeek is actually landed, #30 fixes all of this, based off of
> StartOfWeek.
> 
> see #20 "(currentExplodedTime.tm_wday + 0));" -->
> "(currentExplodedTime.tm_wday + StartOfWeek)); // StartOfWeek is zero
> relative"
> 
> Hell I'll redo the work, just to have it actually reflect the labels, if
> asked!
> 
> 
> Regards

Ooops, sorry, not #30, but #25, plus some work I never submitted.
Attached image 505981.png
(In reply to alta88 from comment #50)
> The code calculates the "Last week" bucket to mean "Within the last 7 days,
> starting with today (day 1)". "Two weeks ago" means "Within the last 14
> days, starting with today (day 1)".
Yes, you are right, I didn't count it properly.

Today is the 9th of September, so yesterday was the 8th. In "Last Week" we have 7th down to 3rd, that is "Latest 7 days *excluding* today and yesterday. In "Two Weeks Ago" we have 2nd down to 27th of August, so indeed up to 14 days old.

I still think that "Last 7 days" and "Last 14 days" is confusing since the yesterday falls into the both of those.

I you want to show what is really displayed, IMHO you'd have to go:
Today
Yesterday
Up to 7 days old (2 to 7 days old)
Up to 14 days old (8 to 14 days old).
Older.

I'm not really sure that Wayne or Kent do UI decisions.

This is of course just all idle discussion since Neal is offering to fix it all.
Comment on attachment 8782501 [details] [diff] [review]
groupDates.patch

Richard can do a review of UI string changes.

Richard my preferred solution would be:
Today
Yesterday
Up to 7 days old
Up to 14 days old
Older.

In the progression old, old, older, the user can see that he/she should not go looking for something 5 days old in the "Up to 14 days old" group.

I find the proposed "Last 7 days" and "Last 14 days" confusing and don't agree with
> It is obvious an earlier date will go in an earlier bucket, if it fits.
Attachment #8782501 - Flags: review?(richard.marti)
@alta88,

No offense. I don't plan to start a fight.

I'm a native Cantonese and Mandarin speaker. I can also read English and write broken English. I'm not a church people even living in US bible belt. But no matter what language you speaks/which religion you belong to, if you use inappropriate unit to express a measure, it is a disaster. I don't care what firefox uses. It is counter intuitive to express time distance in days larger than magical number 3. 

I'm good at math. But I have trouble to figure it out what does it mean by 7 days ago or 14 days ago. If you use day, my stupid brain can only process tomorrow, today and yesterday (not even the day before yesterday nor the day after tomorrow, sorry for my stupidity)

If anyone wants to fix it with days, I'm 100% sure few people are going to enable grouping and actually use it. So please leave it alone.

If anyone wants to fix it with weeks, please add last weeks and two weeks ago with helper function to determine the start of week. Again, my stupid brain can NOT process time distance larger than 3 weeks ago. Please use months, instead.

Sorry for my long comment. I just feel upset for a problem that relate to my user experiences. If it is in github, I will just fork it and send pr to fix it. I don't mean to waste anybody time.
Comment on attachment 8782501 [details] [diff] [review]
groupDates.patch

(In reply to Jorg K (GMT+2, PTO during summer) from comment #55)
> Richard my preferred solution would be:
> Today
> Yesterday
> Up to 7 days old
> Up to 14 days old
> Older.
> 
> In the progression old, old, older, the user can see that he/she should not
> go looking for something 5 days old in the "Up to 14 days old" group.
> 
> I find the proposed "Last 7 days" and "Last 14 days" confusing and don't
> agree with
> > It is obvious an earlier date will go in an earlier bucket, if it fits.

I'm okay with the "Last 7 days" and "Last 14 days" because the wording is simpler for the user and I see no real difference to your proposal as a yesterday's message would also fit into "Up to 14 days old". ;)

I give a tentative r+ because it would be better if we, like Magnus wrote in comment 44, implement the weekly think and this patch here should only land when the desired patch comes too late for TB 42.
Attachment #8782501 - Flags: review?(richard.marti) → review+
Comment on attachment 8782501 [details] [diff] [review]
groupDates.patch

feedback+ for the patch as is.

my strong preference is for wording to be 
last 7 days
last 14 days
Attachment #8782501 - Flags: feedback?(vseerror) → feedback+
Comment on attachment 8782501 [details] [diff] [review]
groupDates.patch

OK, no harm done in landing this.
Attachment #8782501 - Flags: feedback?(rkent) → review+
Blocks: 1301643
(In reply to Richard Marti (:Paenglab) from comment #57)
> I give a tentative r+ because it would be better if we, like Magnus wrote in
> comment 44, implement the weekly think and this patch here should only land
> when the desired patch comes too late for TB 42.
TB 52 ;-)
We can land this now and then implement the proper solution later in bug 1301643. Sorry about the delay I caused.

(In reply to Ricky Zhang from comment #56)
> No offense. I don't plan to start a fight.
No problem. UI bugs are always difficult. Normally we argue between 100 and 200 comments ;-(

We all prefer a week based solution which we will implement in due course, Neal is already offering this.

How would that look like?
Today
Yesterday.
Earlier this week (can be empty).
Last week.
(then what?, Ricky is suggesting something with months.)
Older.

I filed bug 1301643 for that.
Summary: The "Last Week" sorting group label is misleading / wrong → The "Last Week" sorting group label is misleading / wrong (Fix label only)
(In reply to Neal horman from comment #52)
> Hell I'll redo the work, just to have it actually reflect the labels, if
> asked!
Neal, would you mind attaching your updated patch to bug 1301643. Many thanks in advance.
Keywords: checkin-needed
Attachment #8355581 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/1d1919152899

This fixes the labels. Let's continue with the week grouping in bug 1301643.
Assignee: nobody → alta88
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
@Jorg K

Thanks for your work! I like your suggestions:

> Today
> Yesterday
> Earlier this week (can be empty)
> Last week

I believe anything beyond that I won't remember the exact date therefore we need sophisticated search.
Let's continue the discussion in bug 1301643. This bug here is done.
Attached patch stringsSM.patchSplinter Review
Attachment #8789835 - Flags: review?(ewong)
Comment on attachment 8789835 [details] [diff] [review]
stringsSM.patch

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

lgtm
Attachment #8789835 - Flags: review?(ewong) → review+
Keywords: checkin-needed
Comment on attachment 8789835 [details] [diff] [review]
stringsSM.patch

Let's get those strings in before Monday's merge...
http://hg.mozilla.org/comm-central/rev/4b4f1518b722
Temporarily moving to MailNews Core to set SeaMonkey status flags. Strictly speaking this *is* Core code but doesn't seem to fit any of the given components. :-\
Component: Mail Window Front End → Database
Keywords: checkin-needed
Product: Thunderbird → MailNews Core
Component: Database → Mail Window Front End
Product: MailNews Core → Thunderbird
Ok, bugzilla doesn't like me and reset the SM flag - whatever.
Sorry for the noise.
You need to log in before you can comment on or make changes to this bug.