<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/106687/">http://git.reviewboard.kde.org/r/106687/</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;">Thanks for this contribution! I have a few remarks however:
1. Make libkactivity a mandatory dependency. It is now widespread enough IMO and I prefer to avoid adding too many optional dependencies: it helps reducing the number of possible compilation setups and avoid breaking build in corner-cases.
2. Instead of finding out the QGraphicsView from the scene, I would prefer you to add a "QWidget* parent" argument to DocumentView constructor and use this to initialize the ResourceInstance. You can then get rid of most of the "if (ptr)" blocks (To save you some time, DocumentView are instantiated by the DocumentViewContainer class)
3. Can you rename mResourceInstance to mActivityResource? Outside of the domain of KActivities, "ResourceInstance" sounds too generic.
4. Final nitpick, Gwenview coding style for pointers is "Foo* var;", ie: no space before '*', space after '*'. Can you fix the few "Foo * var;"?</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On October 2nd, 2012, 11:21 a.m., Ivan Čukić 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 Gwenview and Plasma.</div>
<div>By Ivan Čukić.</div>
<p style="color: grey;"><i>Updated Oct. 2, 2012, 11:21 a.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;">Gwenview reports the open/close document events to activity manager daemon.
Side-effect - support for Share-Like-Connect applet.
</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;">Yes</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>CMakeLists.txt <span style="color: grey">(a73e4d3)</span></li>
<li>config-gwenview.h.cmake <span style="color: grey">(634c677)</span></li>
<li>lib/CMakeLists.txt <span style="color: grey">(2474ec1)</span></li>
<li>lib/documentview/documentview.cpp <span style="color: grey">(5451062)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106687/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>