[PATCH] dix/events: Set currentTime to the given time stamp in NoticeTime

[Peter Hutterer]

On Sat, Apr 25, 2015 at 11:45:46PM -0700, Keith Packard wrote:
> Rui Matos <tiagomatos at gmail.com> writes:
> 
> > The refactoring in commit efc1035ca958f2c9d266338a308518a0834b1773
> > removed the actual update of currentTime.
> >
> > Signed-off-by: Rui Matos <tiagomatos at gmail.com>
> > ---
> >
> > I noticed this while investigating why hitting a key on GNOME's lock
> > screen would wake the monitor from DPMS but instantly enter DPMS
> > again. GNOME uses a 15 sec idle alarm to enter DPMS while the screen
> > is locked and it turns out that the X server was triggering this alarm
> > immediately when pressing a key.
> >
> > Drilling down into the server I noticed that IdleTimeQueryValue() in
> > IdleTimeWakeupHandler() was getting values bigger than 15 sec in this
> > case which is clearly wrong since I had just pressed a key.
> >
> > That's how I arrived at this patch, please review, thanks.
> 
> Thanks for pointing out this bug; always nice to have a good explanation
> like this. I think your fix is sufficient, but I also think the bug
> highlights that the current NoticeTime function is ignoring the 'time'
> argument, and that essentially all callers to it (save
> NoticeTimeMillis) were passing it currentTime as that argument.
> 
> Here's an alternate patch, which has NoticeTimeMillis assigning
> currentTime, removes the 'time' argument from NoticeTime, and changes
> the other callers of that function to stop passing currentTime.
> 

> From 5f0c9fe32eebd5b3d570a6651e717101ca6f0870 Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp at keithp.com>
> Date: Sat, 25 Apr 2015 23:41:12 -0700
> Subject: [PATCH] dix: NoticeTime ignores 'time' argument.
> 
> After efc1035ca958f2c9d266338a308518a0834b1773, NoticeTime no longer
> uses the 'time' value passed in to it. This patch removes that
> argument, making it clear that 'currentTime' will be unaffected by
> this function.
> 
> The sole caller which expected NoticeTime to modify currentTime was
> NoticeTimeMillis (all other callers passed currentTime as the value),
> so that function has been changed to assign currentTime directly,
> before calling NoticeTime.

tbh, I'm in favour of Rui's patch. That NoticeTime wasn't assigning
currentTime is a bug and should be fixed. But one of the points of having
NoticeTime is that we don't have to manually set the various global time
fields. Even if (currently) only one uses it, it's a lot more maintainable
if we have one function that handles this.

Cheers,
   Peter

> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
>  Xext/saver.c  | 2 +-
>  dix/events.c  | 7 ++++---
>  dix/window.c  | 2 +-
>  include/dix.h | 4 ++--
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/Xext/saver.c b/Xext/saver.c
> index 2c14ea0..631ba8c 100644
> --- a/Xext/saver.c
> +++ b/Xext/saver.c
> @@ -380,7 +380,7 @@ ScreenSaverFreeSuspend(void *value, XID id)
>              DeviceIntPtr dev;
>              UpdateCurrentTimeIf();
>              nt_list_for_each_entry(dev, inputInfo.devices, next)
> -                NoticeTime(dev, currentTime);
> +                NoticeTime(dev);
>              SetScreenSaverTimer();
>          }
>      }
> diff --git a/dix/events.c b/dix/events.c
> index c232eba..be7610c 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -1055,7 +1055,7 @@ MonthChangedOrBadTime(CARD32 *ms)
>  }
>  
>  void
> -NoticeTime(const DeviceIntPtr dev, TimeStamp time)
> +NoticeTime(const DeviceIntPtr dev)
>  {
>      lastDeviceEventTime[XIAllDevices].time = currentTime;
>      lastDeviceEventTime[dev->id].time = currentTime;
> @@ -1072,7 +1072,8 @@ NoticeTimeMillis(const DeviceIntPtr dev, CARD32 *ms)
>          MonthChangedOrBadTime(ms);
>      time.months = currentTime.months;
>      time.milliseconds = *ms;
> -    NoticeTime(dev, time);
> +    currentTime = time;
> +    NoticeTime(dev);
>  }
>  
>  void
> @@ -5282,7 +5283,7 @@ InitEvents(void)
>          memcpy(&event_filters[i], default_filter, sizeof(default_filter));
>  
>          dummy.id = i;
> -        NoticeTime(&dummy, currentTime);
> +        NoticeTime(&dummy);
>          LastEventTimeToggleResetFlag(i, FALSE);
>      }
>  
> diff --git a/dix/window.c b/dix/window.c
> index d49276c..9a06b99 100644
> --- a/dix/window.c
> +++ b/dix/window.c
> @@ -3085,7 +3085,7 @@ dixSaveScreens(ClientPtr client, int on, int mode)
>              DeviceIntPtr dev;
>              UpdateCurrentTimeIf();
>              nt_list_for_each_entry(dev, inputInfo.devices, next)
> -                NoticeTime(dev, currentTime);
> +                NoticeTime(dev);
>          }
>          SetScreenSaverTimer();
>      }
> diff --git a/include/dix.h b/include/dix.h
> index 921156b..05124c6 100644
> --- a/include/dix.h
> +++ b/include/dix.h
> @@ -320,8 +320,8 @@ extern _X_EXPORT WindowPtr
>  GetSpriteWindow(DeviceIntPtr pDev);
>  
>  extern _X_EXPORT void
> -NoticeTime(const DeviceIntPtr dev,
> -           TimeStamp time);
> +NoticeTime(const DeviceIntPtr dev);
> +
>  extern _X_EXPORT void
>  NoticeEventTime(InternalEvent *ev,
>                  DeviceIntPtr dev);
> -- 
> 2.1.4
> 

> 
> 
> -- 
> -keith




> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel