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

Lamarque Souza lamarque at kde.org
Tue Nov 17 22:58:58 UTC 2015

This is an automatically generated e-mail. To reply, visit:

src/plugins/osx/macpoller.h (line 24)

    Nitpick: this should go after #include <QAbstractNativeEventFilter>

src/plugins/osx/macpoller.h (line 75)

    One line per variable.

src/plugins/osx/macpoller.h (line 76)

    Here too.

src/plugins/osx/macpoller.cpp (line 2)

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

src/plugins/osx/macpoller.cpp (line 29)

    Remove trailing space character.

src/plugins/osx/macpoller.cpp (line 49)

    Initialize m_nativeGrabber here too.

src/plugins/osx/macpoller.cpp (line 100)

    Use qCWarning instead.

src/plugins/osx/macpoller.cpp (line 135)

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

src/plugins/osx/macpoller.cpp (line 137)

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

src/plugins/osx/macpoller.cpp (line 212)

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

src/plugins/osx/macpoller.cpp (line 221)

    Remove commented code.

src/plugins/osx/macpoller.cpp (line 242)

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

src/plugins/osx/macpoller.cpp (line 274)

    Remove commented code.

src/plugins/osx/macpoller.cpp (line 325)

    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)

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

src/plugins/osx/macpoller_helper.mm (line 2)

    Append new entries in copyright section, do not prepend.

src/plugins/osx/macpoller_helper.mm (line 43)

    Use qCDebug instead.

src/plugins/osx/macpoller_helper.mm (line 54)

    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)

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

src/plugins/osx/macpoller_helper.mm (line 82)

    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>

More information about the kde-mac mailing list