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

Tue Nov 17 22:58:58 UTC 2015

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


src/plugins/osx/macpoller.h (line 24)
<https://git.reviewboard.kde.org/r/126078/#comment60663>

    Nitpick: this should go after #include <QAbstractNativeEventFilter>


src/plugins/osx/macpoller.h (line 75)
<https://git.reviewboard.kde.org/r/126078/#comment60678>

    One line per variable.


src/plugins/osx/macpoller.h (line 76)
<https://git.reviewboard.kde.org/r/126078/#comment60679>

    Here too.


src/plugins/osx/macpoller.cpp (line 2)
<https://git.reviewboard.kde.org/r/126078/#comment60664>

    This should be appended to the list, not prepended. I know, the next line is wrong too.


src/plugins/osx/macpoller.cpp (line 29)
<https://git.reviewboard.kde.org/r/126078/#comment60665>

    Remove trailing space character.


src/plugins/osx/macpoller.cpp (line 49)
<https://git.reviewboard.kde.org/r/126078/#comment60677>

    Initialize m_nativeGrabber here too.


src/plugins/osx/macpoller.cpp (line 100)
<https://git.reviewboard.kde.org/r/126078/#comment60681>

    Use qCWarning instead.


src/plugins/osx/macpoller.cpp (line 135)
<https://git.reviewboard.kde.org/r/126078/#comment60666>

    Shouldn't you check the return value of this method?


src/plugins/osx/macpoller.cpp (line 137)
<https://git.reviewboard.kde.org/r/126078/#comment60667>

    This should be changed to "return true", right?


src/plugins/osx/macpoller.cpp (line 212)
<https://git.reviewboard.kde.org/r/126078/#comment60669>

    We usually join the last } with the next "else {", like this:
    
    } else {


src/plugins/osx/macpoller.cpp (line 221)
<https://git.reviewboard.kde.org/r/126078/#comment60668>

    Remove commented code.


src/plugins/osx/macpoller.cpp (line 242)
<https://git.reviewboard.kde.org/r/126078/#comment60670>

    We usually do not use extra space after ( and before )


src/plugins/osx/macpoller.cpp (line 274)
<https://git.reviewboard.kde.org/r/126078/#comment60671>

    Remove commented code.


src/plugins/osx/macpoller.cpp (line 325)
<https://git.reviewboard.kde.org/r/126078/#comment60672>

    I do not get the "Unsetting m_catch is enough" comment. What is it supposed to mean since unsetting m_catch is the original code.


src/plugins/osx/macpoller.cpp (line 343)
<https://git.reviewboard.kde.org/r/126078/#comment60673>

    Remove trailing space and shouldn't the commented code in this method be removed?


src/plugins/osx/macpoller_helper.mm (line 2)
<https://git.reviewboard.kde.org/r/126078/#comment60674>

    Append new entries in copyright section, do not prepend.


src/plugins/osx/macpoller_helper.mm (line 43)
<https://git.reviewboard.kde.org/r/126078/#comment60680>

    Use qCDebug instead.


src/plugins/osx/macpoller_helper.mm (line 54)
<https://git.reviewboard.kde.org/r/126078/#comment60676>

    Usually when a new operation returns 0 it is because system is on short on RAM memory (or memory is too fragmented). I would add assert here instead of silently ignoring the failure to allocate memory.


src/plugins/osx/macpoller_helper.mm (line 78)
<https://git.reviewboard.kde.org/r/126078/#comment60682>

    There is no point in doing this if the next line will delete nativeGrabber.


src/plugins/osx/macpoller_helper.mm (line 82)
<https://git.reviewboard.kde.org/r/126078/#comment60675>

    This line can go to inside the if clause above.


- Lamarque Souza


On Nov. 17, 2015, 9:13 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. 17, 2015, 9:13 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 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> 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/20151117/6dcd84e5/attachment-0001.html>