A bit of work around org-clock-idle-time

A bit of work around org-clock-idle-time

[Nicolas Calderon]

Hello,

I was trying to get org-clock-idle-time to work on my machine, but it
would never kick in. Looking at the doc
(http://orgmode.org/manual/Resolving-idle-time.html), I was left under
the impression that x11idle was an option for a better experience, but
emacs idle time would be used otherwise. After digging around a bit, I
found out it was not the case. If you are using X, emacs WILL use
x11idle, wether you have it or not, and in the latter case always get
an idle time of 0.

From that, I have two patches to submit (next 2 emails):

I made a few modifications to x11idle itself. It seemed it could crash
in many ways, one that was noted in comments but somehow not averted
by the addition of a if. I added a few more checks, and made it return
more meaningful error codes (more on that later).


Since org-mode doesn't depend on x11idle being installed on the
machine (at least not on debian), I thought it could be interesting to
add a few checks. First of all, I make sure that the command exists (I
used this post to do it the most generic way,
http://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script),
and then, that the command can execute properly (can connect to the
display, there is enough memory for the info struct and the reporting
of idle time is supported). I'm not sure this is the best
implementation (how often does this get called? If it's often, it
might be worth caching the results rather than invoking two shell
commands every time), but that's as good as I could do with my
knowledge of lisp (none, as of before looking into this).

Hopefully, all this will respect what I read here:
http://orgmode.org/worg/org-contribute.html.

Thanks,

--
Nicolas Calderon

Re: A bit of work around org-clock-idle-time

[Nick Dokos]

Nicolas Calderon <nicolas.calderon.asselin@gmail.com> wrote:

> Hello,
> 
> I was trying to get org-clock-idle-time to work on my machine, but it
> would never kick in. Looking at the doc
> (http://orgmode.org/manual/Resolving-idle-time.html), I was left under
> the impression that x11idle was an option for a better experience, but
> emacs idle time would be used otherwise. After digging around a bit, I
> found out it was not the case. If you are using X, emacs WILL use
> x11idle, wether you have it or not, and in the latter case always get
> an idle time of 0.
> 
> From that, I have two patches to submit (next 2 emails):
> 
> I made a few modifications to x11idle itself. It seemed it could crash
> in many ways, one that was noted in comments but somehow not averted
> by the addition of a if. I added a few more checks, and made it return
> more meaningful error codes (more on that later).
> 
> 
> Since org-mode doesn't depend on x11idle being installed on the
> machine (at least not on debian), I thought it could be interesting to
> add a few checks. First of all, I make sure that the command exists (I
> used this post to do it the most generic way,
> http://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script),
> and then, that the command can execute properly (can connect to the
> display, there is enough memory for the info struct and the reporting
> of idle time is supported). I'm not sure this is the best
> implementation (how often does this get called? If it's often, it
> might be worth caching the results rather than invoking two shell
> commands every time), but that's as good as I could do with my
> knowledge of lisp (none, as of before looking into this).
> 
> Hopefully, all this will respect what I read here:
> http://orgmode.org/worg/org-contribute.html.
> 

Fixing these problems is a good idea, but I have some comments on your
fixes:


o The x11idle.c fixes have whitespace problems, there is an
  unnecessary (badly named *and* misspelled) variable introduced[fn:1]
  and, at least on my (64-bit) platform the %u format causes the
  compiler to complain: it needs to be %lu in my case. This last is of
  course an original problem, not a problem that you introduced, but if
  you are going to fix things, maybe it should be fixed as well, but I'm
  not sure whether %lu is OK on a 32-bit system - can somebody check?

o in org-clock.el, instead of checking whether x11idle exists or not, how
  about something like this:

  ...
  (eq window-system 'x)
     (max (org-x11-idle-seconds) (org-emacs-idle-seconds))
  ...
  
  This is no worse than it is today as far as efficiency goes, but in any
  case, it's not so much the inefficiency of calling out to the shell that I
  object to, it's more the unnecessary complications added to the code.
  Also, it is easy to memoize org-x11-idle-seconds so that it doesn't call
  out to the shell every time, if that seems desirable, but that can be deferred
  for later.


Thanks for finding and fixing the problems: I wouldn't have looked if
you hadn't pointed the way! Hope the comments are useful.

Nick


Footnotes:

[fn:1] I'm talking about "querry" - you can leave it out altogether:

    ...
    //X11 is running, try to retrieve info
    if (XScreenSaverQueryInfo(display, DefaultRootWindow(display), info) == 0) {
	return -1;
    }

    //info was retrieved successfully, print idle time
    printf("%lu\n", info->idle);
    ...

    will do just as well.

Re: A bit of work around org-clock-idle-time

[Bastien]

Nick Dokos <nicholas.dokos@hp.com> writes:

> o in org-clock.el, instead of checking whether x11idle exists or not, how
>   about something like this:
>
>   ...
>   (eq window-system 'x)
>      (max (org-x11-idle-seconds) (org-emacs-idle-seconds))
>   ...

Yes, this is cleaner, thanks!

-- 
 Bastien

Re: A bit of work around org-clock-idle-time

[Nick Dokos]

Bastien <bzg@gnu.org> wrote:

> Nick Dokos <nicholas.dokos@hp.com> writes:
> 
> > o in org-clock.el, instead of checking whether x11idle exists or not, how
> >   about something like this:
> >
> >   ...
> >   (eq window-system 'x)
> >      (max (org-x11-idle-seconds) (org-emacs-idle-seconds))
> >   ...
> 
> Yes, this is cleaner, thanks!
> 

Cleaner maybe, but it's not correct, as Nicolas C. pointed out. Nicolas
C. and I had an email exchange and I hope he will follow up with revised
patches.

Nick

Re: A bit of work around org-clock-idle-time

[Nicolas Calderon]

Sorry, I forgot to reply to all when Nicolas D. answered the first
email and took it off the list. Here is what you missed regarding
org-clock.el:

On Thu, Jul 19, 2012 at 2:33 PM, Nick Dokos <nicholas.dokos@hp.com> wrote:

> Nicolas Calderon <nicolas.calderon.asselin@gmail.com> wrote:
>
>>
>> As for the max solution for idle time, I don't think it's a good
>> solution. It solves the case where x11idle always return 0, but if
>> you're active in X (x11idle always low) but not in emacs (high idle
>> time), you'll end up in clock resolving mode.
>>
>
> Fair enough, but it can still be done so as to simulate today's behavior
> while avoiding the 0 case. Something like
>
>   ((eq window-system 'x)
>      (let ((idle (org-x11-idle-seconds)))
>         (if (> idle 0)
>             idle
>           (org-emacs-idle-seconds))))

The problem I see here is that 0 is a valid output for x11idle, so if
the check is done while X11 is not idle, it will fall back to
org-emacs-idle-seconds. From what I could see in the
shell-command-to-string, there is no way to tell between an output
from the program and the 0 the function returns if there is no output
(can't find the command). That's why I added the additional check for
the command existence.

Optimally, that check would be done once at startup or the first time
idle time is checked but I lack the elisp skills to perform such a
"feat".

That being said, the patch was merged in, so I don't know if it's an
indication that I should not worry.

Thanks,

--
Nicolas Calderon


On Fri, Jul 27, 2012 at 11:19 PM, Nick Dokos <nicholas.dokos@hp.com> wrote:
> Bastien <bzg@gnu.org> wrote:
>
>> Nick Dokos <nicholas.dokos@hp.com> writes:
>>
>> > o in org-clock.el, instead of checking whether x11idle exists or not, how
>> >   about something like this:
>> >
>> >   ...
>> >   (eq window-system 'x)
>> >      (max (org-x11-idle-seconds) (org-emacs-idle-seconds))
>> >   ...
>>
>> Yes, this is cleaner, thanks!
>>
>
> Cleaner maybe, but it's not correct, as Nicolas C. pointed out. Nicolas
> C. and I had an email exchange and I hope he will follow up with revised
> patches.
>
> Nick

Re: A bit of work around org-clock-idle-time

[Bastien]

Nicolas Calderon <nicolas.calderon.asselin@gmail.com> writes:

> Sorry, I forgot to reply to all when Nicolas D. answered the first
> email and took it off the list. Here is what you missed regarding
> org-clock.el:

Thanks for resending.

> Optimally, that check would be done once at startup or the first time
> idle time is checked but I lack the elisp skills to perform such a
> "feat".

This is now the case.

> That being said, the patch was merged in, so I don't know if it's an
> indication that I should not worry.

You should _always_ worry.  :)

-- 
 Bastien