<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/106908/">http://git.reviewboard.kde.org/r/106908/</a>
</td>
</tr>
</table>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/106908/diff/1/?file=90755#file90755line48" style="color: black; font-weight: bold; text-decoration: underline;">konqueror/src/konqview.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<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">48</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#ifdef KActivities_FOUND</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You should switch to #cmakedefine01 in the config-apps.h file, and use #if here.
This has the additional benefit that if you forget to include the header file, or make a typo in the #if line, the compiler tells you nicely.
We are switching all of KDE Frameworks to that, please do the same in new code.
(Doesn't have to be done in this review request of course, since apparently it will affect existing code in the rest of the module, too...)</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/106908/diff/1/?file=90756#file90756line1240" style="color: black; font-weight: bold; text-decoration: underline;">konqueror/src/konqview.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool KonqView::eventFilter( QObject *obj, QEvent *e )</pre></td>
</tr>
</tbody>
<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">1240</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_focusedActivityResourceInstance</span> <span class="o">=</span> <span class="n">m_activityResourceInstance</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is there a risk that this static pointer becomes dangling when this view is destroyed? I guess you're relying on FocusOut being received, but I wonder if this is 100% safe. Worst case an event filter could eat the FocusOut (but ok it would be a bug to have such an over-hungry eventfilter), or a Qt bug could make it fail to deliver the FocusOut before the view gets deleted. I'm not completely sure there's a bug here, but it just sounds risky to rely on an event to be received in order to avoid a crash.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On October 17th, 2012, 3:27 p.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 KDE Base Apps and David Faure.</div>
<div>By Ivan Čukić.</div>
<p style="color: grey;"><i>Updated Oct. 17, 2012, 3:27 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;">Konqueror reports the open/close document events to activity manager daemon.
By knowing which window contains which documents and which one is in focus, we can do the following:
- collect the statistics about visited pages. Further, this provides a score for each document visited, that depends on the number of times it was open, the time the user spent on that location, and the time passed since the last visit.
- availability of a global/workspace applet that allows sharing the current document via e-mail, social networks; bookmarking and rating the link, or connecting it to the current activity. (advantage of this is a unified UI for sharing/rating/linking that works with any application)
- jump-lists (not impl. yet in plasma) to list top rated documents on a launcher icon or in the task manager applet
- krunner can sort the documents based on the score
- more things that I haven't thought of yet
There is no need to *use* ativities to have these benefits. Activities just serve as manual data clustering to provide more useful scores compared to the one-activity approach.
</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;">The event reporting was watched in two ways:
- by using the SLC applet (planned to be included in 4.10, currently plasma-mobile) - when konqueror opens a url, the icons light-up and when clicking on an icon, it show whether it reports the document properly
- by using sqlite3 to browse the .kde/share/apps/activitymanager/resources/database - 'select * from nuao_DesktopEvent;'
Konqueror was used to open webpages and local directories; urls were opened in multiple windows, multiple tabs, or split-views, and both open/close and focusin/focusout events were registered correctly.</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>konqueror/src/CMakeLists.txt <span style="color: grey">(529909d)</span></li>
<li>konqueror/src/konqview.h <span style="color: grey">(45c5bde)</span></li>
<li>konqueror/src/konqview.cpp <span style="color: grey">(2ee9896)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106908/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>