<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/107048/">http://git.reviewboard.kde.org/r/107048/</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 22nd, 2012, 11:04 a.m., <b>David Faure</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;">Well, if setFocus() shouldn't trigger the PartManager, then the fix would be in PartManager itself. Otherwise the part manager and konqueror will have a different vision of the currently active part.
In order to avoid breaking other apps that use PartManager (if any), I would suggest to add a setter in PartManager, something like setIgnoreExplicitSetFocus(true).
In addition, this patch (whether it's done in konq or kparts) requires changing some code in Konqueror, where setFocus() was assumed to end up calling setActivePart.
1) there's a comment in konqviewmanager.cpp:1062 about that, but the comment can simply be adjusted (this was an unnecessary recursive call, so removing that is no problem)
2) there's a setFocus in konqmainwindow.cpp:4778 whose purpose was precisely to set the current part back to what it was before the RMB popup was opened. With this change, setFocus doesn't trigger a setActivePart anymore, so an explicit call to setActivePart should be added.
Looking at the other setFocus calls:
* KonqView::setLoading calls setFocus, but it's ok if this doesn't change the active part (fixes a race condition with the user changing parts quickly after starting to load a typed url, so that's fine)
</pre>
</blockquote>
<p>On December 22nd, 2012, 2:41 p.m., <b>Dawit Alemayehu</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;">Ok. I am working on correcting the original fix for this. Should I aim the fix for 4.10 or 4.9.5 since it is actually a bug fix (technically a fix for a bug fix) ? I would guess 4.10 makes more sense at this point since the fix requires the addition of a new member function to KParts::PartManager. Also I am unsure what you meant for the comment in konqviewmanager.cpp. Do you want to change the comment or remove the setFocus call itself ?</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;">This seems a bit too invasive for a 4.9.5 fix, to me. Go for 4.10.
The comment in konqviewmanager.cpp says basically "setFocus will call setActivePart". Your patch will change that -> change the comment. Don't remove the call, we need it :)</pre>
<br />
<p>- David</p>
<br />
<p>On October 30th, 2012, 7:49 p.m., Dawit Alemayehu 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 Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated Oct. 30, 2012, 7:49 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;">The attached patch is an attempt to fix the bug where opening certain types of documents in a background tab results in the new tab being activated inadvertently.
The issue can be reproduced by simply attempting to open a PDF document using either the MMB or the CTRL+LMB button in Konqueror. If you have a local PDF file simply navigate to the location of the file and click on it using the MMB. Otherwise, see the aforementioned bug report for a site with PDF links in it. If the "Open new tabs in background" option is checked when you clicked on the PDF document, then the PDF document is opened embedded in Konqueror tab in the background. However, this background tab should not be activated when the aforementioned option is enabled.
Unfortunately, in the currently implementation the background tab will get activated if the part used to handle the resource (PDF file in this case) issues a FocusIn event, for example by calling setFocus. That is because KPart::PartManager installs an event filter on all the KParts it manages, listens for FocusIn events, and invokes KParts::PartManager::setActivatePart.</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;">- Tested opening a PDF file when the "Open new tabs in background" option is checked.
- Tested opening a PDF file when the "Open new tabs in background" option is UNchecked.
- Tested opening a PDF link from a webpage using both the MMB and CTRL+LMB.</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="http://bugs.kde.org/show_bug.cgi?id=306417">306417</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>konqueror/src/konqviewmanager.cpp <span style="color: grey">(c8e3cb0)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107048/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>