[PATCH v2 0/1] MR1660: mstask: Don't assume a ONCE trigger is on the current day.

[Sven Baars (＠sbaars)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52994

This fixes a regression.

--
  v2: mstask: Don't assume a ONCE trigger is on the current day.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1660

[PATCH v2 1/1] mstask: Don't assume a ONCE trigger is on the current day.

[Sven Baars]

From: Sven Baars <sbaars(a)codeweavers.com>

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52994
---
 dlls/mstask/task.c               |  5 ++++-
 dlls/mstask/tests/task_trigger.c | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/dlls/mstask/task.c b/dlls/mstask/task.c
index d2a03aa8b56..c3113ebbb72 100644
--- a/dlls/mstask/task.c
+++ b/dlls/mstask/task.c
@@ -516,12 +516,15 @@ static HRESULT WINAPI MSTASK_ITask_GetNextRunTime(ITask *iface, SYSTEMTIME *rt)
 
             case TASK_TIME_TRIGGER_ONCE:
                 st = current_st;
+                st.wYear = This->trigger[i].wBeginYear;
+                st.wMonth = This->trigger[i].wBeginMonth;
+                st.wDay = This->trigger[i].wBeginDay;
                 st.wHour = This->trigger[i].wStartHour;
                 st.wMinute = This->trigger[i].wStartMinute;
                 st.wSecond = 0;
                 st.wMilliseconds = 0;
                 SystemTimeToFileTime(&st, &trigger_ft);
-                if (CompareFileTime(&begin_ft, &trigger_ft) <= 0 && CompareFileTime(&trigger_ft, &end_ft) < 0)
+                if (CompareFileTime(&trigger_ft, ¤t_ft) >= 0 && CompareFileTime(&trigger_ft, &end_ft) < 0)
                 {
                     if (!have_best_time || CompareFileTime(&trigger_ft, &best_ft) < 0)
                     {
diff --git a/dlls/mstask/tests/task_trigger.c b/dlls/mstask/tests/task_trigger.c
index 24b873770d2..236540cf0ee 100644
--- a/dlls/mstask/tests/task_trigger.c
+++ b/dlls/mstask/tests/task_trigger.c
@@ -525,6 +525,22 @@ static void test_GetNextRunTime(void)
     hr = ITaskTrigger_SetTrigger(trigger, &data);
     ok(hr == S_OK, "got %#lx\n", hr);
 
+    memset(&st, 0xff, sizeof(st));
+    hr = ITask_GetNextRunTime(task, &st);
+    ok(hr == S_OK, "got %#lx\n", hr);
+    ok(!memcmp(&st, &cmp, sizeof(st)), "got %u/%u/%u wday %u %u:%02u:%02u\n",
+       st.wDay, st.wMonth, st.wYear, st.wDayOfWeek,
+       st.wHour, st.wMinute, st.wSecond);
+
+    hr = ITaskTrigger_GetTrigger(trigger, &data);
+    ok(hr == S_OK, "got %#lx\n", hr);
+    data.rgFlags &= ~TASK_TRIGGER_FLAG_DISABLED;
+    data.TriggerType = TASK_TIME_TRIGGER_ONCE;
+    /* add 1 day to test triggers in the far future */
+    trigger_add_ms(&data, 24 * 60 * 60 * 1000, &cmp);
+    hr = ITaskTrigger_SetTrigger(trigger, &data);
+    ok(hr == S_OK, "got %#lx\n", hr);
+
     memset(&st, 0xff, sizeof(st));
     hr = ITask_GetNextRunTime(task, &st);
     ok(hr == S_OK, "got %#lx\n", hr);
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/1660

[Sven Baars (＠sbaars)]

On Fri Dec  2 14:47:58 2022 +0000, **** wrote:
> Dmitry Timoshkov replied on the mailing list:
> ```
> "Sven Baars (@sbaars)" <wine(a)gitlab.winehq.org> wrote:
> > On Fri Dec  2 14:16:59 2022 +0000, **** wrote:
> > > Dmitry Timoshkov replied on the mailing list:
> > > \`\`\`
> > > Sven Baars <wine(a)gitlab.winehq.org> wrote:
> > > > -                if (CompareFileTime(&begin_ft, &trigger_ft) <= 0 &&
> > > > CompareFileTime(&trigger_ft, &end_ft) < 0)
> > > > +                if (CompareFileTime(&trigger_ft, ¤t_ft) >= 0)
> > > >                  {
> > > >                      if (!have_best_time ||
> > > > CompareFileTime(&trigger_ft, &best_ft) < 0)
> > > >                      {
> > > > Removing the trigger end time check doesn't look right.
> > > > -- 
> > > > Dmitry.
> > > > \`\`\`
> > > > Hi Dmitry,
> >
> > Thanks for the reply. I don't see how begin_ft and end_ft are relevant
> > for this trigger. It happens exactly once, at the "Start" time and
> > "Begin" day.
> >
> > Having an "End" day before the "Begin" day is invalid for all
> > triggers. I could add a patch that checks for that.
> > Obviosuly the trigger shouldn't fire outside of begin/end time, so changing
> > the trigger time bounds check doesn't look right. At best, that should be
> > a separate patch with a bunch of tests.
> > -- 
> > Dmitry.
> I submitted a v2 that addresses your concern.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1660#note_18443

[Dmitry Timoshkov]

"Sven Baars (@sbaars)" <wine(a)gitlab.winehq.org> wrote:
> On Fri Dec  2 14:47:58 2022 +0000, **** wrote:
> > Dmitry Timoshkov replied on the mailing list:
> > ```
> > "Sven Baars (@sbaars)" <wine(a)gitlab.winehq.org> wrote:
> > > On Fri Dec  2 14:16:59 2022 +0000, **** wrote:
> > > > Dmitry Timoshkov replied on the mailing list:
> > > > \`\`\`
> > > > Sven Baars <wine(a)gitlab.winehq.org> wrote:
> > > > > -                if (CompareFileTime(&begin_ft, &trigger_ft) <= 0 &&
> > > > > CompareFileTime(&trigger_ft, &end_ft) < 0)
> > > > > +                if (CompareFileTime(&trigger_ft, ¤t_ft) >= 0)
> > > > >                  {
> > > > >                      if (!have_best_time ||
> > > > > CompareFileTime(&trigger_ft, &best_ft) < 0)
> > > > >                      {
> > > > > Removing the trigger end time check doesn't look right.
> > > > > -- 
> > > > > Dmitry.
> > > > > \`\`\`
> > > > > Hi Dmitry,
> > >
> > > Thanks for the reply. I don't see how begin_ft and end_ft are relevant
> > > for this trigger. It happens exactly once, at the "Start" time and
> > > "Begin" day.
> > >
> > > Having an "End" day before the "Begin" day is invalid for all
> > > triggers. I could add a patch that checks for that.
> > > Obviosuly the trigger shouldn't fire outside of begin/end time, so changing
> > > the trigger time bounds check doesn't look right. At best, that should be
> > > a separate patch with a bunch of tests.
> > > -- 
> > > Dmitry.
> > I submitted a v2 that addresses your concern.

Looking at it again I don't understand the bounds check change. beging_ft already
accounts for current_ft earlier in the code. Can you leave it as it is now?

-- 
Dmitry.