Closed Bug 955363 Opened 10 years ago Closed 10 years ago

"Last week"-group in log viewer containing one day too many

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: aleth)

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1926 at 2013-04-12 21:17:00 UTC ***

The "Last week"-group in the log viewer should contain the days of the last week minus today and yesterday which have own items. That would make it at most five days in there.

There's currently six: the oldest item in the list is the same day of the week as today. I don't think that's right.
*** Original post on bio 1926 at 2013-04-13 11:00:33 UTC ***

I'm not sure about this.

Also, should "Last week" (the string) really be "This week"? Is that clearer?
*** Original post on bio 1926 at 2013-04-13 14:41:02 UTC ***

(In reply to comment #1)
> Also, should "Last week" (the string) really be "This week"? Is that clearer?

This is confusing in English...generally if I'm using the term "Last week" it refers to the 7 days from the Saturday before "now" to the Sunday before that, but sometimes the Sunday before "now" to the Monday before that...

We're using it here as the "past 7 days", so I think "Last week" is OK (I would probably use "the past week" if I was talking about this, but that probably would look awkward in the UI).
*** Original post on bio 1926 at 2013-04-13 14:46:29 UTC ***

So if I say "last Thursday" on a Thursday, is that previous Thursday part of "last week"? That would be the question. I can see an argument both ways.
*** Original post on bio 1926 at 2013-04-13 14:51:49 UTC ***

(In reply to comment #3)
> So if I say "last Thursday" on a Thursday, is that previous Thursday part of
> "last week"? That would be the question. I can see an argument both ways.

Yes.
*** Original post on bio 1926 at 2013-04-13 14:52:57 UTC ***

(In reply to comment #4)
> (In reply to comment #3)
> > So if I say "last Thursday" on a Thursday, is that previous Thursday part of
> > "last week"? That would be the question. I can see an argument both ways.
> 
> Yes.

Everything OK then :)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
*** Original post on bio 1926 at 2013-04-13 15:18:20 UTC ***

Reopening the bug as clokep also thinks that there should be either five or seven items in this group.

http://log.bezut.info/instantbird/130413#m247 :

15:03:01 <Mic> I understand what you mean in this bug but I don't agree completely.
15:03:15 * clokep didn't really read what the bug was about. ;)
15:05:08 <Mic> This: http://imgur.com/szBnAhs
15:05:29 <Mic> Having six items in the "last week"-group seem wrong in my opinion.
15:05:48 <Mic> *seems
15:06:03 <clokep> Mic: "Yesterday" is Wednesday?
15:06:21 <Mic> Today = Friday, Yesterday = Thursday
15:07:27 <clokep> I agree with Mic.
15:07:32 <clokep> 6 looks wrong, no matter what.
15:07:52 <clokep> It should be 5 or 7, because it should be fairly clear that "5" is excluding todya and yesterday.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1926 as attmnt 2351 at 2013-04-13 15:24:00 UTC ***

Alright then :)
Attachment #8354118 - Flags: review?(benediktp)
Assignee: nobody → aleth
Status: REOPENED → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1926 as attmnt 2352 at 2013-04-13 15:27:00 UTC ***

Nit: avoid unnecessary {}
Attachment #8354119 - Flags: review?(benediktp)
Comment on attachment 8354118 [details] [diff] [review]
Patch

*** Original change on bio 1926 attmnt 2351 at 2013-04-13 15:27:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354118 - Attachment is obsolete: true
Attachment #8354118 - Flags: review?(benediktp)
*** Original post on bio 1926 at 2013-04-16 11:35:30 UTC ***

To answer your question from IRC, what you were missing is that the log time (not date) for concatenated logs is always 00:00, and so <= vs < does matter.
Comment on attachment 8354119 [details] [diff] [review]
Patch

*** Original change on bio 1926 attmnt 2352 at 2013-04-17 10:29:01 UTC ***

I understand what's happening here now (but I don't like it) and think we need an explaining comment.

In my opinion |timeFromToday <= kDayInMsecs| made much more sense than extending the upper limit by another day just to check if the timeFromToday was exactly kDayInMsecs. I assume you did that to match the operator with the ones you used for the weeks?

I'd therefore suggest using |timeFromToday <= ... - kDayInMsecs| for the week-checks with a comment mentioning that the last n days including today started (n-1)*kDayInMsecs before because of our choice of the reference point. 

(Maybe it would have made more sense to use the end of today as reference point in general? All checks would follow the pattern "Today <= endOfToday - kOneDayInMsecs", "Yesterday <= endOfToday - 2* kOneDayinMsecs", ... then)
Attachment #8354119 - Flags: review?(benediktp) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1926 as attmnt 2380 at 2013-04-17 13:54:00 UTC ***

I agree it's wrong to effectively add a whole day minus one second to the range, both on principle and in case there ever are any logs with dates with timestamps in these ranges. Thanks for pointing that out. Added some comments too.
Attachment #8354147 - Flags: review?(benediktp)
Comment on attachment 8354119 [details] [diff] [review]
Patch

*** Original change on bio 1926 attmnt 2352 at 2013-04-17 13:54:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354119 - Attachment is obsolete: true
Comment on attachment 8354147 [details] [diff] [review]
Patch

*** Original change on bio 1926 attmnt 2380 at 2013-04-17 19:01:56 UTC ***

This looks good and works fine. Thanks a lot!

This patch needs to be checked in after the patch for bug 955371 (bio 1933).
Attachment #8354147 - Flags: review?(benediktp) → review+
Whiteboard: [checkin-needed], see comment 12
*** Original post on bio 1926 at 2013-04-17 23:01:43 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/90d78da5cb25
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed], see comment 12
Target Milestone: --- → 1.4
*** Original post on bio 1926 at 2013-04-18 09:30:31 UTC ***

Thanks for spotting this and sorry about all the confusion!
You need to log in before you can comment on or make changes to this bug.