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

Thu Nov 19 16:45:24 UTC 2015

> On Nov. 17, 2015, 10: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>
> 
> René J.V. Bertin wrote:
>     Any guidelines that dictate this?

https://wiki.qt.io/Coding_Conventions there will be more guidelines in https://techbase.kde.org/Policies/Frameworks_Coding_Style, until there please use Qt's code conventions.

> On Nov. 17, 2015, 10: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.
> 
> René J.V. Bertin wrote:
>     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`?

Read https://community.kde.org/Frameworks/Porting_To_qCDebug for more information on how to define KIDLETIME.

> On Nov. 17, 2015, 10: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?
> 
> René J.V. Bertin wrote:
>     As far as I can see all it can be used for is to print a warning, right?

I guess so, since the "additional" prefix implies that it is not required.

> On Nov. 17, 2015, 10: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?
> 
> René J.V. Bertin wrote:
>     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 ...

The point is that you changed the semantics of this method. The original code returns true on all successful setUpPoller() calls. Now it returns false on the first successfull setUpPoller() call and returns true only on the second call and on. Besides, this is a framework, do not assume that there will always be just one "upstream" for a framework.

> On Nov. 17, 2015, 10: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?
> 
> René J.V. Bertin wrote:
>     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?

Well, it is a general consensus that adding commented code is not a good programming practice. I am not aware of any written recomendation in Frameworks or Qt code conventions about that but I have never seen a patch with commented code to pass any code review either.

> On Nov. 17, 2015, 10: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.
> 
> René J.V. Bertin wrote:
>     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`.

Delete never zeros memory before releasing it for performance reasons. The overhead of zeroing m_monitorId here is minimum though. Anyway nobody will be able to access m_monitorId after the next line or the program will crash.

> On Nov. 17, 2015, 10:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 54
> > <https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line54>
> >
> >     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.
> 
> René J.V. Bertin wrote:
>     And I would argue that it is up to the calling software to decide how to react to a failure to set up idle detection: in this case it's only the "resume from idle" functionality that can no longer work. The idle timeout detection feature should still be able to function (possibly by adding some kind of reset when the calculated idle time returns to 0).
>     Asserting would mean that the application aborts, which I find a bit overkill for a situation that could be handled a lot more elegantly. That would require handling the return value from `setupPoller()` in `KIdleTimePrivate::loadSystem()` and up, of course.

Ok then.

- Lamarque

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

On Nov. 18, 2015, 4:35 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, 4:35 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-frameworks-devel/attachments/20151119/81b13e2a/attachment-0001.html>