Add 'Server day' and 'Server week' to the Time Periods dropdown #87
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The calendar week, starting with Monday/Sunday, and the day, starting with midnight, are largely irrelevant to me when playing WoW. So I think you’ve already done a great job by offering “last day” (last 24 hours) and “last week” (last 7 days) in the Time Period dropdown instead.
However, what is relevant to me in WoW, instead of the calendar day/week, is the server day and server week, i.e. the time since the daily server reset (currently 6:00 CEST) and the time since the weekly server reset (Wednesday, 6:00).
You have implemented these periods already in the minimap button tooltip, for the profit summaries.
What this PR does is add these periods also to the Time Period dropdown menu in the main window:
In particular, I find the server day to be extremely useful. It allows me to see all my sales from what I perceive as “today”, without the overhang of yesterday’s entries that I get when using the “last day” (24h) time period.1
To keep it inexpensive, the last daily/weekly reset time is only calculated once per session. This is sufficient for me, since I rarely have a session that goes over the reset time. And a player who regularly starts a session just before the reset time will probably make better use of a different time period anyway. But YMMV.
I’ve been using this modification in (almost) identical form in my local copy for months, and it works fine and error-free in WoW Retail. But it is completely untested with other flavors like Classic, TBC, etc.!
Since I’m not very familiar with Journalator’s code structure, my implementation is probably suboptimal. So, if you like the idea, please take this PR as a mere suggestion and do it your/Journalator’s way.
Thanks for considering!
Footnotes
About the wording: I originally called it “Since [daily|weekly] server reset”. This is more expressive, but considering the length of the other menu items, it seemed too long. Also, the “day”/”week” wording is more consistent with the rest of the menu. ↩