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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-frameworks-devel
Subject:    Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)
From:       "Lamarque Souza" <lamarque () kde ! org>
Date:       2015-11-19 16:45:24
Message-ID: 20151119164524.14400.14930 () mimi ! kde ! org
[Download RAW message or body]

--===============3734917059520257887==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

> 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
> 
> 

--===============3734917059520257887==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit

<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">  \
<tr>  <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="https://git.reviewboard.kde.org/r/126078/">https://git.reviewboard.kde.org/r/126078/</a>
  </td>
    </tr>
   </table>
   <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417216#file417216line24" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller.h</a>  <span style="font-weight: normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">24</font></th>  <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#include \
&lt;Carbon/Carbon.h&gt;</span></pre></td>  <th bgcolor="#e9eaa8" style="border-left: \
1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">24</font></th>  <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "><span class="cp">#include \
&lt;QTimer&gt;</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Nitpick: this should go after #include \
&lt;QAbstractNativeEventFilter&gt;</p></pre>  </blockquote>

 <p>On November 18th, 2015, 12:48 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Any \
guidelines that dictate this?</p></pre>  </blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre> <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line178" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller.cpp</a>  <span style="font-weight: normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">100</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">            \
<span class="n">qWarning</span><span class="p">()</span> <span \
class="o">&lt;&lt;</span> <span class="s">&quot;failed to load UpdateSystemActivity \
from CoreServices.framework&quot;</span><span class="p">;</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Use \
qCWarning instead.</p></pre>  </blockquote>

 <p>On November 18th, 2015, 12:48 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
presume that should be using KIDLETIME for the category?</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Isn't \
it possible to get the <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">KIDLETIME()</code> symbol \
through libKF5KIdleTime rather than having to pull in <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">../../logging.cpp</code>?</p></pre>  </blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Read https://community.kde.org/Frameworks/Porting_To_qCDebug for more \
information on how to define KIDLETIME.</p></pre> <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line213" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller.cpp</a>  <span style="font-weight: normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">135</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">additionalSetUp</span><span class="p">();</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Shouldn't you check the return value of this method?</p></pre>  \
</blockquote>

 <p>On November 18th, 2015, 12:48 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As \
far as I can see all it can be used for is to print a warning, right?</p></pre>  \
</blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">I guess so, since the "additional" prefix implies that it is not \
required.</p></pre> <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line215" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller.cpp</a>  <span style="font-weight: normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">137</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="k">return</span> <span class="nb">false</span><span \
class="p">;</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This \
should be changed to "return true", right?</p></pre>  </blockquote>

 <p>On November 18th, 2015, 12:48 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 ...</p></pre>  </blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre> <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417217#file417217line422" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller.cpp</a>  <span style="font-weight: normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">226</font></th>  <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span class="c1">// \
</span><span class="cs">TODO</span><span class="c1"></span></pre></td>  <th \
bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">343</font></th>  <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="c1">// The alternative is to disable sleep using<span class="ew"> \
</span></span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Remove trailing space and shouldn't the commented code in this method be \
removed?</p></pre>  </blockquote>

 <p>On November 18th, 2015, 12:48 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I put \
the commented code as a reference for alternative approaches that could be \
investigated once more if ever <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">updateSystemActivity</code> 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?</p></pre>  </blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre> <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line54" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller_helper.mm</a>  <span style="font-weight: \
normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">54</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="k">if</span> <span class="p">(</span><span class="n">nativeGrabber</span><span \
class="p">)</span> <span class="p">{</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre>  </blockquote>

 <p>On November 18th, 2015, 2:38 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">setupPoller()</code> in <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KIdleTimePrivate::loadSystem()</code> and up, of course.</p></pre>  \
</blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Ok then.</p></pre> <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 17th, 2015, 10:58 p.m. UTC, \
<b>Lamarque Souza</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/126078/diff/6/?file=417218#file417218line78" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/plugins/osx/macpoller_helper.mm</a>  <span style="font-weight: \
normal;">

     (Diff revision 6)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">78</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">            \
<span class="n">nativeGrabber</span><span class="o">-&gt;</span><span \
class="n">m_monitorId</span> <span class="o">=</span> <span class="mi">0</span><span \
class="p">;</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There \
is no point in doing this if the next line will delete nativeGrabber.</p></pre>  \
</blockquote>

 <p>On November 18th, 2015, 12:48 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've \
learned to be fool-proof with this kind of thing (I don't trust <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">delete</code> to zero memory before releasing it; there's no \
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">~CocoaEventFilter()</code>) but what does \
make it redundant here is setting <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">m_nativeGrabber = 0</code> just after deleting <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">nativeGrabber</code>.</p></pre>  </blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre> <br />

<p>- Lamarque</p>

<br />
<p>On November 18th, 2015, 4:35 p.m. UTC, René J.V. Bertin wrote:</p>

<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;">  <tr>
  <td>

<div>Review request for KDE Software on Mac OS X, KDE Frameworks and Dario \
Freddi.</div> <div>By René J.V. Bertin.</div>

<p style="color: grey;"><i>Updated Nov. 18, 2015, 4:35 p.m.</i></p>

<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
kidletime
</div>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The example now works: when I set a QTimer with \
interval==0, the expected wait for user input (<code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">resumingFromIdle</code> signal) works. However, I am getting a <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">stopCatchingIdleEvents</code> 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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Surely I'm missing \
something, but what?</p></pre>  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>src/plugins/osx/CMakeLists.txt <span style="color: grey">(e1b50b8)</span></li>

</ul>

<p><a href="https://git.reviewboard.kde.org/r/126078/diff/" style="margin-left: \
3em;">View Diff</a></p>

  </td>
 </tr>
</table>

  </div>
 </body>
</html>

--===============3734917059520257887==--

[Attachment #3 (text/plain)]

_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

[prev in list] [next in list] [prev in thread] [next in thread] 