feat: Implement nextDayStartHour setting #3640
Conversation
There was a problem hiding this comment.
Many thanks for doing this @allexendar - it's a great start.
I've added a few observations from an initial review of the code, and trying out the UI - hoping to start a discussion about what next....
Unfortunately I can't stay up beyond midnight tonight to test it out today.
| momentAdjusted().startOf(unitOfTime).startOf('day'), | ||
| momentAdjusted().endOf(unitOfTime).startOf('day'), |
There was a problem hiding this comment.
Maintenance-wise, I think this PR is going to be a bit fragile over time.
- It is repetitious
- Every person who works on this code in future is going to have to know that they mustn't use
window.moment()directly. - And/or every reviewer is going to have to remember to look out for
window.moment()uses
I started rolling out a TasksDate but unfortunately not managed to make time on fully adopting it to replace all storage and direct access to moment objects.
Too much is in my head only, currently, about what needs to be done to finish adopting it...
I am not totally opposed to the approach in this PR, but personally, I would have imagined that standardising time-and-date storage and access would be a preferable first step, before implementing this feature.
There was a problem hiding this comment.
I think whatever method we use there'd be potential to not respect user's setting of nextDayStartHour (unless we centralize time source). Personally, I think that just adding a note in "Using Moment as a type in src/" should be enough. That's one of the first entries I read before typing moment anywhere so I think it's prominent enough.
I agree that centralized TasksDate would be ideal but perhaps this is already a turn in that direction? We'd have centralized momentAdjusted (or some other name) procedure which could later just be replaced with TasksDate version.
| "changeRequiresRestart": "REQUIRES RESTART.", | ||
| "dates": { | ||
| "nextDayStartHour": { | ||
| "description": "Sets the hour at which the new day begins. Interacting with tasks after midnight but before this hour is treated as if interacting the day before.", |
There was a problem hiding this comment.
Please could it expand on what it means by "interacting"?
There was a problem hiding this comment.
I wanted to emphasize "all interactions with obsidian-tasks plugin", i.e. "all responses-to-user-action ...". Meaning that, yes, completing tasks "is treated as if interacting the day before", but also so is using the "td"/"tm" shortcuts, playing with datetime picker, etc.
| .addSlider((slider) => { | ||
| const settings = getSettings(); | ||
| slider.setLimits(0, 23, 1); |
There was a problem hiding this comment.
I didn't find the slider UI very intuitive, although that is mainly an Obsidian thing, rather than specific to Tasks.
Please have a think about whether the (recently-added) RESTART REQUIRED label will be required on this, for now. Like, will search queries need to be re-triggered if the setting is changed?
There was a problem hiding this comment.
Yes. Adding "RESTART REQUIRED" would be necessary. Queries would only need to retrigger if current local time is between nextDayStartHour_OLD:00 and nextDayStartHour_NEW:00 (or reverse) though.
I don't think anyone will change this setting often, this is a 'set to 4AM and forget' feature. Anki uses 4AM as default and I don't think many users ever change it. Of course, we'd want to keep 0 as default to preserve backwards compatibility.
| slider.setDynamicTooltip(); | ||
| slider.onChange(async (value) => { | ||
| updateSettings({ nextDayStartHour: value }); | ||
| await this.plugin.saveSettings(); |
There was a problem hiding this comment.
What impact will this feature have on the "Reload the vault at midnight" facility in Tasks?
The idea of that feature is that all queries are reloaded a few seconds after midnight, so that things like "due today" are re-interpreted with the new date.
Except that with this feature set to +2, the reload at midnight will have no effect...
And instead presumably the queries should all be reloaded at a few seconds after 2am instead - but for now, users will need to know to do that themselves manually.
There was a problem hiding this comment.
Yes, this is the only problem I foresaw. I guess there are two options.
-
Change
now.subtract(1, 'day').endOf('day');tonow.subtract(nextDayStartHour, 'hours');
This would mean thatreloadQueryAtMidnightwould run at true midnight (uselessly) and then againnextDayStartHourhours later. Perhaps the best option would be to just donow.subtract(nextDayStartHour, 'hours');for all calls to this function. IfnextDayStartHour = 0, nothing happens. Otherwise, we preserve the continuity of time if any caller is actually interested in that. -
Keep
now.subtract(1, 'day').endOf('day');but makereloadQueryAtMidnightaware ofnextDayStartHoursetting.
I'd really wanted to evade this and make as few external procedures aware of the setting.
| * this function will return a moment object for the end of the *previous* day. | ||
| * Otherwise, it returns the current, unaltered moment. | ||
| */ | ||
| export function momentAdjusted(): Moment { |
There was a problem hiding this comment.
There really needs to be tests for this, and updated versions of the existing tests for features that have been modified in this PR - to demonstrate how the behaviour has changed.
For example, demonstration of how this feature interacts in time-zones other than GMT... (I haven't yet been able to get my head around that)
| ) | ||
| .addSlider((slider) => { | ||
| const settings = getSettings(); | ||
| slider.setLimits(0, 23, 1); |
There was a problem hiding this comment.
How does this setting relate to the user's time zone, if at all?
There was a problem hiding this comment.
It shouldn't. It should only look at local time value.
src/DateTime/DateAdjusted.ts
Outdated
| export function momentAdjusted(): Moment { | ||
| const { nextDayStartHour } = getSettings(); | ||
| const now = window.moment(); | ||
| if (now.hour() < nextDayStartHour) { |
There was a problem hiding this comment.
What happens when clocks change? There needs to be tests for this - including in timezones close to the international date line...
I have written up the results of my previous experiments in timezones in tests:
https://publish.obsidian.md/tasks-contributing/Testing/Testing+and+Time+Zones
Added "RESTART REQUIRED" to settings UI. Ran `yarn run lint` on code.
Changed modified moment from `.subtract(1, 'day').endOf('day')` to
`.subtract(nextDayStartHour, 'hours')` to preserve time continuity.
|
|
I have thought about the changes in this PR a lot more, and having edited in the link to the original issue above, have come to the realisation that this is doing more than the agreed changes in that request... #3395 is specifically and intentionally only Allow Users to Define Start of Day for Task Completion and Recurrence. That is a behaviour that is very specific and clear for users to reason about and to document. This PR is doing many more things that that, making it:
|
|
I agree that this commit is doing more than #3395 asks for. I disagree that it is less clear. This design already exists in other software, e.g., Anki:
Why would testing be hard?
etc?
By default we'd have nextDayStartHour=0 — so nothing changes. If users deliberately go into settings, read the docs and change the feature, is it not reasonable to expect them to also edit their time manipulation code? They know what the code does after all so the fact that nothing changed shouldn't be surprising to them? Thanks for the very rapid and considerate responses. As a maintainer, you're probably more attuned to long-term impacts than me. I'm fine with us thinking about this for the next few days and then closing if we conclude this approach is unsound. I'll be testing the changes on my local vault. |
|
I am not saying that we would never change the search facility. I am trying to say that based on years of experience, I strongly prefer small, focussed PRs that do one thing that is releasable. Once the facility described in the feature request is done, there can be further experimentation and discussion about how to change search.
In case you haven’t seen it, I’ve written up a fair amount about writing of automated tests in this repo. https://publish.obsidian.md/tasks-contributing/Testing/About+Testing Every changed line of code here would need at least one new test that would fail if that changed line were reverted. |
|
One other observation - based on my knowledge of the code. More big advantages of just doing what is requested in the Feature Request:
|
2 participants
Types of changes
Changes visible to users:
feat- non-breaking change which adds functionality)feat!!orfix!!- fix or feature that would cause existing functionality to not work as expected)fix- non-breaking change which fixes an issue)i18n- additions or improvements to the translations - see Support a new language)docs- improvements to any documentation content for users)vault- improvements to the Tasks-Demo sample vault)contrib- any improvements to documentation content for contributors - see Contributing to Tasks)Internal changes:
refactor- non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)test- additions and improvements to unit tests and the smoke tests)chore- examples include GitHub Actions, issue templates)Description
First draft of "Allow Users to Define Start of Day for Task Completion and Recurrence".
More detail in my comment: #3395 (comment).
Motivation and Context
How has this been tested?
No change-specific tests yet. Only existing
yarn testcall.Screenshots (if appropriate)
Checklist
yarn run lint.Terms