Fix inaccurate weekly episode count

[ryban]

After the new year I noticed the weekly episode count was off. Just made the Date class track the seconds to easily count the number of days.

[erengy]

Could you explain what you mean by "weekly episode count"? Is it the estimated last-aired episode number? How does this change fix the issue?

For starters, let's get a couple of relatively minor issues out of the way:

• Since you're using seconds only for internal calculations, it should be a private member rather than a public one.
• Our implementation allows unknown values, but it seems you didn't take this into account while converting them into a tm struct. For instance, if the month value is 0, then tm.tm_mon becomes -1. Is this allowed? Did you check how mktime behaves in this case?
• You've changed a base object that is used throughout the application for many purposes, not just in relation to what you intended to fix. Did you check if this change affects anything else? If so, you should mention that in your pull request. (e.g. "ToDayCount now returns days since Unix epoch, but this doesn't break anything because the existing codebase didn't rely on a different assumption.")

The main issue is that the Date class is supposed to be YYYY-MM-DD object with the resolution of one day. Even if you represent this information in a time_t variable, the resolution won't change. In other words, you'd always have some information such as 2017-01-02 00:00:00, 2017-01-03 00:00:00 and so on. What makes you think that this change would have any practical impact on anything?

If we need more resolution, then we shouldn't be using Date in our calculations. But in case of airing dates, we don't have the information that would make this change useful.

[ryban]

The issue I was trying to fix is the "You've watched X episodes in the last week" count in the Now Playing tab. Basically on December 30th I had ~70 episodes watched in the last week (didn't check the 31st), but come January 1st I had only 8 episodes watched, which isn't right since I continually watched episodes throughout the previous week. I manually did the calculation for the 9th to last item in my history which did come out to 8 days ago but was actually on the 28th. I suspect a similar issue with a smaller gap can happen when the month changes. Probably more obvious during a leap year on March 1st.

Quick example: If the date is 2017-01-01, everything from 2016-12-28 and older is out of the 1 week range. ((2017 * 365) + (1 * 30) + 1) - ((2016 * 365) + (12 * 30) + 28)) = 8 when the result should have been 4.

The issue comes from the way we convert Dates to days. (year * 365) + (month * 30) + day isn't correct. It doesn't properly account for all of the weird issues with dates. The differing lengths of months in this case. When I converted the YMD to unix time it handles the dates properly and I go from "8 episodes in the last week" to "50 episodes in the last week".

Since you're using seconds only for internal calculations, it should be a private member rather than a public one.

Thinking about this, it might actually be a better idea to not save the seconds at all since someone can just come along and change the year, month, or day value which won't update the stored seconds. So everytime we do the subtraction we have to recalculate the seconds value. I don't think that is a huge performance issue.

I don't handle having a month of 0 and if it is, mktime will return -1 while which will likely break the whole calculation. Not sure what to do about that. If its already 0 leave it as 0? mktime will also give -1 on systems that don't handle negative time if we give it a date before the unix epoch.

ToDayCount is only used here so I think it should be fine with this change. It should actually be more correct now. As for the other changes, they should only making things more accurate and since the - operator is really only relative I don't think it should matter too much in the end and it should make other parts more accurate.

[erengy]

I see, thanks for the explanation.

There are two main approaches to implementing a date class:

1. Storing separate year, month and day values (this is currently the case for our Date class)
2. Using a single time-since-epoch value (e.g. std::chrono::system_clock::time_point or std::time_t)

Mixing them both is not necessary, unless you have to maintain a cache to improve performance. For our purposes, performance is not the primary concern. If I remember correctly, the reason I went for the field-based approach and used that rather crude algorithm was to accommodate unknown values. While I was aware that it could cause problems, I haven't encountered this particular issue until now.

Handling dates and time is a lot more complicated than it appears to be, so it's generally not a good idea to roll your own code. As I mentioned in #316, I intend to use a proper date library in the future. That being said, a proper date class won't allow partial dates such as 2017-01-00 or 2017-00-00. It's probably a better idea to handle them elsewhere (e.g. using a separate PartialDate class or some helper functions), but I'm yet to decide on how to do that.

For now, changing the conversion algorithm might suffice at fixing this issue. Could you provide a solution that would work for both full and partial dates, without introducing the seconds member variable?

---

Edit: I'm playing around with the date library now, and it seems it's going to work.

[ryban]

Since you seem to have this working using the date library I'll go ahead and close this.