Fixed overlapping ranges when all history is selected

[JMannervik]

Fixes #3932 Fixes #2961

Fix Calendar diagram: prevents dates from other years from appearing when "All history" is selected

Reviews graph: corrects "days ago" range labels (e.g., "0-19 days" and "1-20 days ago" no longer overlap)

[user1823]

I'm on 0abac06c and overlapping ranges are still there:

[user1823]

I do not have that much history so it was a bit hard to test, would love some help testing and getting feedback if you have time

I guess that you can use the collection shared here for testing purposes.

Pinging @Expertium to ask his permission for the same.

[JMannervik]

Thanks for the feedback and the help, i am starting to work on a new solution that I can test more before updating this PR:)

[JMannervik]

Thanks for the feedback and the help, i am starting to work on a new solution that i will be able to test

[JMannervik]

Thanks! I have been able to test the new fix now as well, if you could try it aswell it would be very helpful @user1823. And thank you @Luc-Mcgrady for the great tips!

[user1823]

I am testing 14b88a6f2.

The ranges don't overlap now. But, I noticed some issues:

• The running total of the newest bin doesn't match the total below the graph, which means that the graph misses some reviews.

• The oldest bin is very thin. This is mathematically correct because it represents a smaller number of days, but is not a good UX, IMO.

• The changes in time.ts and calendar.ts are probably not needed.

[JMannervik]

good point, i can remove the changes made in time and calendar, any suggestions for the ux for oldest bin or should i let that be?

[user1823]

any suggestions for the ux for oldest bin

I think that you can make it as wide as the rest of the bins.

[JMannervik]

I can't seem to recreate the error where the running total does not match the overall total. For me it matches for this version of the PR

[Luc-Mcgrady]

The running total of the newest bin doesn't match the total below the graph, which means that the graph misses some reviews.

I can't seem to recreate the error where the running total does not match the overall total. For me it matches for this version of the PR

1-20 days ago doesn't include today. Maybe this is the reason.

[JMannervik]

The running total of the newest bin doesn't match the total below the graph, which means that the graph misses some reviews.

I can't seem to recreate the error where the running total does not match the overall total. For me it matches for this version of the PR

1-20 days ago doesn't include today. Maybe this is the reason.

good point, I think it is logic to include today either case, i will make that adjustment!

[user1823]

I am not sure which one is correct, but this shows a different number of reviews as compared to Anki 25.09.

This PR (e3546ea49):

Anki 25.09:

[JMannervik]

I am not sure which one is correct, but this shows a different number of reviews as compared to Anki 25.09.

I cant really recreate this error, even if I switch beween main and this PR I get the same running total for both of them. It seems confusing since I did not change where the data is collected from for the total. In short, the sums are computed from the same source in both branches, so the difference probably means either the input data differs or one of the environments still has stale build artifacts.

[user1823]

Sorry for that. The difference was because of incorrect timezone on my WSL system.

This looks very good but there is a small issue: If I select the 1 year option, the graph shows reviews from "0 days ago" to "365 days ago", i.e., it includes reviews of 366 days instead of the expected 365 days.

The correct bins should be "0 to 4 days ago", "5 to 9 days ago", "10 to 14 days ago" and so on upto "360 to 364 days ago".

[JMannervik]

Okay! No problem at all, great that we could figure it out. I have made the necessary changes for the bins to be correctly calculated and for the checks to pass now! Thanks for all the help @user1823!

[user1823]

Functionally, it looks perfect to me now. Thank you! You can add Fixes #3932 and Fixes https://github.com/ankitects/anki/issues/2961 to your original post so that GitHub will auto-close these issues when this PR is merged.

The changes in config/bool.rs and revlog/mod.rs are unrelated and should be reverted.

[JMannervik]

No problem at all, I have made the revertions and added the info to the original post aswell. Thanks again for the help!

[JMannervik]

I looked over the comments you left @Luc-Mcgrady! I resolved the first one, and for the second one i refactored it and aded some comments so that it might be easier to understand what it is that is actually happening, I hope that is enough :))

I dont know why the buildkite checks fail now though, should I do anything about it? The files it is complaining about are unrelated files that i have not touched in the latest commits.

[JMannervik]

For the not all time graphs today is cut off.

This is fixed now aswell, It was that the xMax values for 1 month and 3 months needed to be 1 and not 0!

[user1823]

Why does the year require xMax to be 0 while the rest require it to be 1?

[JMannervik]

Why does the year require xMax to be 0 while the rest require it to be 1?

Good question, I dont think that is necessary now actually I had not concidered it!

I changed it when I tried to fix the range of the year. (i.e. graph shows reviews from "0 days ago" to "365 days ago", i.e., it includes reviews of 366 days instead of the expected 365 days.) and then i made changes so that the range 0-4 days, 5-9 days and so on worked. Together it worked, but now when i test xmax = 1 for the year it makes no difference.

[user1823]

I haven't yet tried to build it again. But, I reviewed the code and some parts look over-engineered. It is always the best practice to keep the code as simple as possible.

For example, this code looks functionally equivalent to what is proposed in this PR, but is much easier to read.

export function dayLabel(daysStart: number, daysEnd: number): string {
    // Convert bin boundaries [start, end) to display range
    // For past days: bin [-10, -5) should display "5-9 days ago"
    // For future days: bin [5, 10) should display "In 5-9 days"
    
    const larger = Math.max(Math.abs(daysStart), Math.abs(daysEnd));
    const smaller = Math.min(Math.abs(daysStart), Math.abs(daysEnd));
    
    if (larger - smaller <= 1) {
        // singular
        if (daysStart >= 0) {
            return tr.statisticsInDaysSingle({ days: daysStart });
        } else {
            return tr.statisticsDaysAgoSingle({ days: -daysStart });
        }
    } else {
        // range
        if (daysStart >= 0) {
            // Future: [5, 10) -> "In 5-9 days"
            return tr.statisticsInDaysRange({
                daysStart,
                daysEnd: daysEnd - 1,
            });
        } else if (daysEnd <= 0) {
            // Past: [-10, -5) -> "5-9 days ago"
            return tr.statisticsDaysAgoRange({
                daysStart: Math.abs(daysEnd),
                daysEnd: Math.abs(daysStart) - 1,
            });
        } else {
            // Crosses zero: [-5, 1) -> "0-4 days ago"
            return tr.statisticsDaysAgoRange({
                daysStart: 0,
                daysEnd: Math.abs(daysStart) - 1,
            });
        }
    }
}

But more importantly, do we really need to modify the dayLabel function? Why not fix the values before inputting them into dayLabel?

[JMannervik]

I haven't yet tried to build it again. But, I reviewed the code and some parts look over-engineered. It is always the best practice to keep the code as simple as possible.

But more importantly, do we really need to modify the dayLabel function? Why not fix the values before inputting them into dayLabel?

I see, yes ofcourse I can spend some time trying to simple it down, and minimize the code as much as possible. I will try this solution of modfying it in review ts aswell, and see if i can make that work!

[JMannervik]

I was able to simplyfy it quite a bit thanks to you suuggestion @user1823! I dont know how I could simplify it more, but i am open to suggestions! Thanks again for all the help and feedback :)))

[JMannervik]

Okay, now I have managed to fix both of the issues #3932 and #2961 while only making changes in review. The solution is simpler than before and I dont touch the actual daylabel function now.

I think this is enough for the issues, there is no overlap for any of the ranges and the data is correct in the ranges aswell!

As usual, all and any feedback are appreciated!

[JMannervik]

I added some comments and could remove some of the logic that adressed issues that were not related to the code but the enviroment! If anyone else could test this aswell to make sure it works well for everyone that would be greatly appreciated!

[JMannervik]

@Luc-Mcgrady Luc-Mcgrady 3 hours ago // For Year, shift thresholds forward by one day to make first bin 0-4 instead of 0-5

Are you removing the "today" bar for this year on purpose?

No i dont think so, Day 0 (today) is still included: the last bin will be [last_negative, 1) which includes day 0

[Luc-Mcgrady]

I mean that for the all bar, unlike the year bar, today has its own bar: On second look it is weird to me that it has 0-5 days, with the today bar also there. I assume this is a quirk of the labeling? (You can see the pixel wide bin for today on the right) (The year bar does not have the today bar as shown here:)

[JMannervik]

Regarding the today bar @Luc-Mcgrady I think the problem is fixed now, when i made the changes that were suggested, could you check if you can still se the bar with these changes?

[JMannervik]

user1823 left a comment Left two suggestions.

There's one more problem:

The graph shows up to "1839 days ago" (i.e., a total of 1840 days) but the days studied part says "of 1841 days". One of them has to be wrong.

Regarding this, when i try to recreate it the oldest bin includes the same day span as all other bins (20 or 50 in my case depending on if i select all history or not) For both of them my total amount of days is included in the span. for example: my total of is 3613 days, had the oldest bin show 3600-3649, which is because the bin should be the same sizes.

Is this a neccessary fix still to make sure the oldest bin instead (in my case) displays 3600-3612 days?

And is do you still have the same issue now? is it still one day to "little" included in the graph"?