<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/100316/">http://git.reviewboard.kde.org/r/100316/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 8th, 2011, 10:13 p.m., <b>Benjamin Poulain</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;">You are gonna kill me. The previous patch was more correct actually :)
I did not notice those objects are parented to the instance of Application. One have to wonder why those methods are static at all.</pre>
</blockquote>
<p>On January 8th, 2011, 11:18 p.m., <b>Pierre Rossi</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;">Huh indeed, that might actually crash on exit now!
The static methods still seem more convenient than something like Application::instance()->historyManger() or similar calls...
Hard to decide which approach to keep for cleanup, I'm tempted to drop the instance() as a parent in favor of a scoped pointer because it's more generic, so if some day someone wants to add something that's not a QObject in the same fashion, it can still be done in the same way.</pre>
</blockquote>
<p>On January 9th, 2011, 8:39 a.m., <b>Andrea Diamantini</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;">The static methods + members has been maintained from the "big switch" to be a singleton. I'm not actually sure on what are the implications of moving from the QWeakPointer (deleting them in dtor) to the QScopedPointer class (deleting them... when?).
I just think that, for cleaness of the Application class code, they should be completely removed. This anyway will force the other classes to use something like "Application::instance()->historyManager()" or similar.
That's it. Tell me what you think it's better.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> I'm not actually sure on what are the implications of moving from the QWeakPointer (deleting them in dtor) to the QScopedPointer class (deleting them... when?).
Static variables are destroyed in reverse order of their creation. Which mean the objects would be destroyed after main().
> I just think that, for cleaness of the Application class code, they should be completely removed. This anyway will force the other classes to use something like > "Application::instance()->historyManager()" or similar.
> That's it. Tell me what you think it's better.
Sounds good.
Make them explicitly dependent on the object application. We could add rApp as a shortcut to Application::instance(), similar to qApp.
Or I guess those classes could be singleton themselves.</pre>
<br />
<p>- Benjamin</p>
<br />
<p>On January 8th, 2011, 9:51 p.m., Pierre Rossi wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for rekonq and Benjamin Poulain.</div>
<div>By Pierre Rossi.</div>
<p style="color: grey;"><i>Updated Jan. 8, 2011, 9:51 p.m.</i></p>
<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;">I believe we don't need static members in QWeakPointers for all the *Managers, static getter functions would do the job.</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;">"compiles and runs" ™</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="/show_bug.cgi?id=N/A">N/A</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/application.h <span style="color: grey">(b30e337)</span></li>
<li>src/application.cpp <span style="color: grey">(466a0a4)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100316/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>