> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel