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

Wed Nov 18 12:48:07 UTC 2015

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.h, line 24
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417216#file417216line24>
> >
> >     Nitpick: this should go after #include <QAbstractNativeEventFilter>

Any guidelines that dictate this?

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 127
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line127>
> >
> >     Initialize m_nativeGrabber here too.

Oops, indeed it could be left un-initialised. My bad.

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 178
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line178>
> >
> >     Use qCWarning instead.

I presume that should be using KIDLETIME for the category?

Isn't it possible to get the `KIDLETIME()` symbol through libKF5KIdleTime rather than having to pull in `../../logging.cpp`?

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 213
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line213>
> >
> >     Shouldn't you check the return value of this method?

As far as I can see all it can be used for is to print a warning, right?

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 215
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line215>
> >
> >     This should be changed to "return true", right?

Yeah, that seems logical, but I don't see any documentation on what setupPoller() should return. Then again the upstream code doesn't appear to use the return value anyway so the question is a bit moot ...

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 300
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line300>
> >
> >     Remove commented code.

OK, but among the final things to do before committing :)

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 353
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line353>
> >
> >     Remove commented code.

The bingo isn't mine!

Oh, the code. As above, in final cleanup :)

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 404
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line404>
> >
> >     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.

The original code worked rather differently. It used a mechanism with an internal poller, but you're right, the wording as it is makes sense only to someone who went about replacing that original code and made a number of (trial-and-)errors on the way.

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 422
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line422>
> >
> >     Remove trailing space and shouldn't the commented code in this method be removed?

I put the commented code as a reference for alternative approaches that could be investigated once more if ever `updateSystemActivity` is removed. If there's a policy against such comments I can also store the snippets in a separate file, but that means adding one. What's preferable?

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 43
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line43>
> >
> >     Use qCDebug instead.

That one actually got through and was never meant to!

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 78
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line78>
> >
> >     There is no point in doing this if the next line will delete nativeGrabber.

I've learned to be fool-proof with this kind of thing (I don't trust `delete` to zero memory before releasing it; there's no `~CocoaEventFilter()`) but what does make it redundant here is setting `m_nativeGrabber = 0` just after deleting `nativeGrabber`.

- René J.V.

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

On Nov. 17, 2015, 10: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, 10: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/20151118/f0b9edc5/attachment-0001.html>