[KDE/Mac] Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Boudhayan Gupta]

> On Nov. 18, 2015, 12:52 p.m., Martin Gräßlin wrote:
> > src/plugins/osx/macpoller.cpp, lines 222-227
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line222>
> >
> >     seriously? You care about idle timeouts below 5 msec? This is a framework to tell the application whether the user doesn't use input devices. I don't know how fast you type, but I'm relatively certain that it takes more than 5 msec to move my finger from one key to another. What exactly is that you want to detect here? An event after each key press, maybe even between a key press and a key release? Because that's probably already more about 5 msec.
> 
> René J.V. Bertin wrote:
>     The timer is used only for the detection of idle timeouts (so *absence* of user input), and the precision with which it fires determines the accuracy with which those events can be timed. This is currently inaccessible anyway, but I don't believe in doing things half-bakedly. One might question whether it makes sense to allow the framework to work at high precision (I more or less agree that remains to be seen). But I don't think it makes sense to do that and then leave the QTimer in a mode where the requested precision cannot be met.
> 
> Martin Gräßlin wrote:
>     To get this quite clear: you care about a precision of below 5 msec, this is a third of the time it takes at least till the next frame is rendered. If you are lucky you get the result of a key press or mouse move represented on screen after 16 msec, more likely it's more. And you care about a wrong timer precision below 5 msec. Sorry that's ridiculous. Please don't include such non-sense code. You make that code more difficult to all other developers to maintain. This code has cost if it's included.
> 
> Aleix Pol Gonzalez wrote:
>     It's not a matter of doing things "right" or "wrong". It's a matter of priorities.
>     
>     KIdleTime is a framework for figuring out whether the system is idle. I don't consider 5ms not using a system as it's being idling.
> 
> René J.V. Bertin wrote:
>     Again, we're talking about detecting how long a system has been idle, not whether it is (the framework is called KIdle*Time*, right? :)). The precision in question here applies just as well to (very) short periods (timeouts) as well as to long ones.
>     Also, don't confound the 5*ms* lower limit under which I switch to a high precision timer with the 5`%` error of a Qt::CoarseTimer. The precision of a high precision timer may be overkill for longer timeout durations in a framework that has a 1ms granularity, but the effect of 5% error scales with duration.
>     
>     I consider that it's up to the user of a feature to know what s/he expects of it, not up to a framework. (Turning that around would probably remove the raison d'être of a lot of software.)
>     
>     Finally, I have not been able to measure any increase in overhead when using a (single) high precision timer.
>     
>     Granted, this is a bit a matter of principle for me, but as I've said elsewere, I could easily see me having used this in a previous job.
> 
> Martin Gräßlin wrote:
>     Rene, please stop it. Don't argue! You won't get this code in with polling and setting the qtimer precision based on clearly irrelevant value. It is our responsibility as fellow KDE developers to prevent such code to get it. I absolutely don't care how you intend to use this code, but this is a user idle framework. A system which had an input event in the last five milli seconds is not idle per definition. Thus the code is idiotic and wrong. Please stop arguing and change the code given the feedback you get here. Same above for the nullptr check. That one has to go, it's a wrong check at that place, remove it.
> 
> Nicolás Alvarez wrote:
>     FYI, while you fight and argue about the best way to do high-precision frequent polling on something that doesn't need any precision, I'm waist-deep in a disassembler making good progress figuring out how to do this without any polling at all.

Talk is cheap, show me the code!

**wink**


- Boudhayan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126078/#review88509
-----------------------------------------------------------


On Nov. 18, 2015, 10:05 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2015, 10:05 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> -------
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that the plugin for OS X still uses the deprecated Carbon-based algorithm that I already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, IORegistry and CoreServices to do idle-time calculation as it should be done, and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -----
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> The example now works: when I set a QTimer with interval==0, the expected wait for user input (`resumingFromIdle` signal) works. However, I am getting a `stopCatchingIdleEvents` signal which means the application waits forever, without ever getting to compare idle time to the list of timeouts.
> I haven't been able to figure out where that signal comes from, nor why this doesn't happen on Linux.
> 
> Surely I'm missing something, but what?
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20151119/8b139b1a/attachment.html>