-
-
Notifications
You must be signed in to change notification settings - Fork 304
feat: Implement nextDayStartHour setting #3640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -331,6 +331,28 @@ export class SettingsTab extends PluginSettingTab { | |
| }); | ||
| }); | ||
|
|
||
| new Setting(containerEl) | ||
| .setName(i18n.t('settings.dates.nextDayStartHour.name')) | ||
| .setDesc( | ||
| SettingsTab.createFragmentWithHTML( | ||
| i18n.t('settings.dates.nextDayStartHour.description') + | ||
| '</br>' + | ||
| this.seeTheDocumentation( | ||
| 'https://publish.obsidian.md/tasks/Getting+Started/Dates#Next+day+start+at', | ||
| ), | ||
| ), | ||
| ) | ||
| .addSlider((slider) => { | ||
| const settings = getSettings(); | ||
| slider.setLimits(0, 23, 1); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this setting relate to the user's time zone, if at all?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't. It should only look at local time value. |
||
| slider.setValue(settings.nextDayStartHour); | ||
| slider.setDynamicTooltip(); | ||
| slider.onChange(async (value) => { | ||
| updateSettings({ nextDayStartHour: value }); | ||
| await this.plugin.saveSettings(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is the only problem I foresaw. I guess there are two options.
|
||
| }); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| new Setting(containerEl).setName(i18n.t('settings.datesFromFileNames.heading')).setHeading(); | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import type { Moment } from "moment"; | ||
| import { getSettings } from "../Config/Settings"; | ||
|
|
||
| /** | ||
| * Returns the current moment, adjusted for the "Next day start hour" setting. | ||
| * | ||
| * If the current time is before the "next day start hour" (e.g., 2 AM), | ||
| * 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 { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| const { nextDayStartHour } = getSettings(); | ||
| const now = window.moment(); | ||
| if (now.hour() < nextDayStartHour) { | ||
|
||
| return now.subtract(1, 'day').endOf('day'); | ||
| } | ||
| return now; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import type { Moment } from 'moment'; | ||
| import { momentAdjusted } from './DateAdjusted'; | ||
|
|
||
| /** | ||
| * Represent an inclusive span of time between two days at 00:00 local time. | ||
|
|
@@ -41,8 +42,8 @@ export class DateRange { | |
| const unitOfTime = range === 'week' ? 'isoWeek' : range; | ||
|
|
||
| return new DateRange( | ||
| window.moment().startOf(unitOfTime).startOf('day'), | ||
| window.moment().endOf(unitOfTime).startOf('day'), | ||
| momentAdjusted().startOf(unitOfTime).startOf('day'), | ||
| momentAdjusted().endOf(unitOfTime).startOf('day'), | ||
|
Comment on lines
+45
to
+46
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maintenance-wise, I think this PR is going to be a bit fragile over time.
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 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think whatever method we use there'd be potential to not respect user's setting of I agree that centralized |
||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,10 @@ | |
| }, | ||
| "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.", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please could it expand on what it means by "interacting"?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| "name": "Next day starts at" | ||
| }, | ||
| "cancelledDate": { | ||
| "description": "Enabling this will add a timestamp ❌ YYYY-MM-DD at the end when a task is toggled to cancelled.", | ||
| "name": "Set cancelled date on every cancelled task" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 REQUIREDlabel 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.