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

RESOLVED FIXED in 1.4

Status

Instantbird
Other
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Mic, Assigned: aleth)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
*** 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.
(Assignee)

Comment 1

4 years ago
*** 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).
(Assignee)

Comment 3

4 years ago
*** 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.
(Assignee)

Comment 5

4 years ago
*** 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
Last Resolved: 4 years ago
Resolution: --- → INVALID
(Reporter)

Comment 6

4 years ago
*** 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 → ---
(Assignee)

Comment 7

4 years ago
Created attachment 8354118 [details] [diff] [review]
Patch

*** Original post on bio 1926 as attmnt 2351 at 2013-04-13 15:24:00 UTC ***

Alright then :)
Attachment #8354118 - Flags: review?(benediktp)
(Assignee)

Updated

4 years ago
Assignee: nobody → aleth
Status: REOPENED → ASSIGNED
(Assignee)

Comment 8

4 years ago
Created attachment 8354119 [details] [diff] [review]
Patch

*** Original post on bio 1926 as attmnt 2352 at 2013-04-13 15:27:00 UTC ***

Nit: avoid unnecessary {}
Attachment #8354119 - Flags: review?(benediktp)
(Assignee)

Comment 9

4 years ago
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)
(Assignee)

Comment 10

4 years ago
*** 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.
(Reporter)

Comment 11

4 years ago
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-
(Assignee)

Comment 12

4 years ago
Created attachment 8354147 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 13

4 years ago
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
(Reporter)

Comment 14

4 years ago
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+
(Reporter)

Updated

4 years ago
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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed], see comment 12
Target Milestone: --- → 1.4
(Assignee)

Comment 16

4 years ago
*** 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.