<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/101886/">http://git.reviewboard.kde.org/r/101886/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Just to explain the d-pointer thing:
This is an excellent guide on why it's done
http://zchydem.enume.net/2010/01/19/qt-howto-private-classes-and-d-pointers/
We do it in libraries to maintain binary comparability, so if someone else makes an application which uses our lib (for example KBattleship includes the ChatWidget) it won't break when we want to change the lib.
Without d-pointers if we added a private member it would change the size of our class. So any application compiled against an old version of the lib expects it to be one size, the library would have something else. It all crashes and the world explodes. We don't want that. With d pointers, only the size of a private class changes, which we can do.
For the KCM, even though it does install like a library, no-one links against it, so this whole situation doesn't matter.
summary:
is some-one else linking to our code?
yes : use d-pointers.
no : personal preference, you don't need to.</pre>
<br />
<p>- David</p>
<br />
<p>On July 8th, 2011, 7:10 p.m., Lasath Fernando wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/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 Telepathy.</div>
<div>By Lasath Fernando.</div>
<p style="color: grey;"><i>Updated July 8, 2011, 7:10 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;">This was rather a big refractor (for me at least), and this is the first time I created any new classes. So I'm not sure I did quite a few things right.
I created a new tab in the KCM, called Behaviour. I think we'll probably have to change the labels on the radiobuttons to something that makes more sense.
I also included "../app/telepathy-chat-ui.h" because I needed an enum from it. I don't know if there's anything against doing that or not.
I also am not sure I used a QButtonGroup correctly.
Also, I noticed the other class didn't use a d-pointer, (even though a KCM is supposed to be like a library file) so I didn't.
There are many more things I was doubtful at the time, but I can't possibly remember now (I shouldn't leave writing review requests till 2AM). Hopefully, everything makes sense in the diff.
And as always, my code is in my scratch repo:
http://quickgit.kde.org/?p=clones%2Ftelepathy-chat-handler%2Ffernando%2FdetachableTabs.git&a=shortlog&h=refs/heads/refractored_tabs</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;">Changed a (well, the only) setting, and the program changed behaviour as expected. </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>app/telepathy-chat-ui.h <span style="color: grey">(ad6809f)</span></li>
<li>app/telepathy-chat-ui.cpp <span style="color: grey">(71ac4ee)</span></li>
<li>config/CMakeLists.txt <span style="color: grey">(48252a7)</span></li>
<li>config/appearance-config.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>config/appearance-config.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>config/behavior-config.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>config/behavior-config.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>config/behaviorconfig.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>config/main-window.h <span style="color: grey">(e95bf93)</span></li>
<li>config/main-window.cpp <span style="color: grey">(3a79da4)</span></li>
<li>config/main.cpp <span style="color: grey">(c47f493)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101886/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>