<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/103524/">http://git.reviewboard.kde.org/r/103524/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 24th, 2011, 8:38 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/103524/diff/1/?file=44675#file44675line386" style="color: black; font-weight: bold; text-decoration: underline;">konqueror/client/kfmclient.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; ">void ClientApp::sendASNChange()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">349</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KConfig</span> <span class="n">config</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span class="s">"kfmclientrc"</span><span class="p">));</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">386</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">checkForUserDefinedBrowser</span><span class="p">(</span><span class="o">&</span><span class="n">browserApp</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<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 code later on uses "new KRun" for the case of a user-defined browser, given that KRun can handle that. Why not factorize this and call that code for local files too? It would make the code shorter and easier to maintain.</pre>
</blockquote>
<p>On December 24th, 2011, 10:13 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;">Well that would be nice and that was my first choice, but unfortunately that is not possible because KRun::init would need to be modified to use the user defined browser settings for local URLs too. Right now it does not. However, doing that would mean hard coding the check for whether or not "konqueror/kfmclient" is the service configured to handle the given request (mimetype). I doubt we would want to do that. Without that change using the "new KRun" call for the local file case would cause an infinite recurssion where KRun will end up calling kfmclient to handle the local html page.
However, the other way around is viable. That is we can use "KRun::run" for the "http" case too. It would absolve kfmclient from using "new KRun" which itself might end up calling it back under certain circumstances. Otherwise, unless I am missing something those two scenarios have to be handled differently.</pre>
</blockquote>
<p>On December 25th, 2011, 8:59 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;">The infinite loop issue can't happen if we only call KRun (from kfmclient) when a BrowserApplication is set (so it won't call back kfmclient).
Something is clear to me: if BrowserApplication should apply to local html files too, then KRun should use it for local html files. Otherwise other users of KRun won't do the right thing (well, ok, or via kfmclient, but that's a runtime dependency that they might not want -- we've had cases of people without konqueror installed at all).
IMHO the best fix is to let KRun handle http and local-html files the same way, and just call new KRun from kfmclient, for these two cases.
For krun this would mean something like this... hmm ok requires factorizing some code to avoid a goto...
http://www.davidfaure.fr/2011/krun.diff
Feel free to apply and test :-)
(I only tested that it doesn't break default (no-browser-app) usage)</pre>
</blockquote>
<p>On December 25th, 2011, 5:04 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;">Right. The infinite looping back will not occur if the local resource condition is handled properly and we most definitely do not want to hard code the check for konqueror/kfmclient for the reasons you stated. I also think that the KRun patch is probably the best approach to solve this problem.
However, I have one issue with your patch. The hard coding of "text/html" as the test for when to use an external browser in the local file case. What about the cases where the user has configured other mime-types to be handled by a browser ? Instead of hard coding that case, would it not be better to simply compare the preferred service configured to handle the mime-type of the local file against the preferred service for "text/html" ? That way you are not hard coding to a specific mime-type, but the service that handles that mime-type without actually hard coding a specific service name. A win win, no ?</pre>
</blockquote>
<p>On December 26th, 2011, 4:21 p.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;">I don't follow. If the user configures "PDF files should be opened in firefox", then KRun will already honour that, this has nothing to do with the special-case "BrowserApplication" setting, which was meant to mean "the handler for all http urls". I can see some point in making that setting work automatically for local html files too (so that the user doesn't have to configure that separately in kcmfiletypes). But if I configure "inode/directory goes to konqueror" then that should keep being the case, even if BrowserApplication=firefox (your suggested logic would break that, and send *everything* to firefox that used to go to konqueror).</pre>
</blockquote>
<p>On December 26th, 2011, 4:42 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;">I was talking about the check in your patch that does the following:
if (!d->m_externalBrowser.isEmpty() && mime->is(QLatin1String("text/html")))
That check will only allow local HTML files to be opened with the external browser. What if the mime-type of the file was "application/xhtml+xml" ? Or any other mime-type that is configured to be handled by the browser ? What I was actually saying was instead comparing against the "text/html" mime-type you should compare the preferred service configured for the given mime type. That way you would not care if the local file is "application/xhtml+xml" or "text/html" . As such to me it makes more sense if "mime->is" check in the above if statement was replaced with something like:
KMimeTrader::self()->preferredService(mime->name()) == KMimeTrader::self()->preferredService(QLatin1String("text/html"))
That way a local file of type "application/xhtml+xml" is properly sent to the external browser as well. Does that make more sense ?</pre>
</blockquote>
<p>On December 26th, 2011, 4:45 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;">ooops. I meant to compare the "desktopEntryName()" of the preferred services in the example I provided.</pre>
</blockquote>
<p>On December 26th, 2011, 7:24 p.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;">Yes, on top of text/html, application/xhtml+xml could be added, and possibly application/xml. These are the three mimetypes handled by khtml.desktop, so that should cover all the cases.
Your suggested code would break the inode/directory case that I mentionned: preferredService(inode/directory)==preferredService(text/html)==konqueror and yet we shouldn't open directories in firefox.
I don't think BrowserApplication should be implemented as "replace konqueror everywhere". If one wants that, we should just implement the long-standing request for kcmfiletypes, a feature that would allow to replace/remove an app globally across all mimetypes.</pre>
</blockquote>
<p>On December 27th, 2011, 2:35 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;">> I don't think BrowserApplication should be implemented as "replace konqueror everywhere". If one wants that, we should just implement the long-standing request for
> kcmfiletypes, a feature that would allow to replace/remove an app globally across all mimetypes.
Yes, BrowserApplication should not be implemented as "replace konqueror everywhere" for the obvious reason that Konqueror is not only a web browser application. However, simply hard coding a couple of common values seems problematic to me when Konqueror is the preferred service for handling several more types of XML files than the ones in your list. Would it not be much easier to exclude all "inode/*" mime-types than to check for all the mime-types that should be allowed to be opened by an external browser ? Perhaps something like this ?
static bool isBrowserPreferredServiceFor(KMimeType::Ptr mime)
{
if (mime->isDefault() || mime->name().startsWith("inode/"))
return false;
KService::Ptr service = KMimeTypeTrader::self()->preferredService(mime->name());
if (!service)
return false;
KService::Ptr preferredService = KMimeTypeTrader::self()->preferredService(QLatin1String("text/html"));
if (!preferredService)
return false;
return (service->desktopEntryName() == preferredService->desktopEntryName());
}
Anyhow, other than being concerned about someone complaining about this down the road, I really have no real objection as to how this should be handled. And your KRun patch seem to work fine for opening local html files ; so it is safe to apply it. I will modify this patch to simply do the same thing for the "file" protocol as is being done for the "http" protocol.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> Konqueror is the preferred service for handling several more types of XML files than the ones in your list
I don't see that.
kfmclient_html.desktop:MimeType=text/html;application/xhtml+xml;application/xml;
(where the middle one is even useless, since it's a subclass of application/xml)
I'll add application/xml to my krun patch. Shall I commit it then?
> Would it not be much easier to exclude all "inode/*" mime-types?
I disagree. Basically, if a user associates konqueror with anything else himself, your patch would disregard that and just fire up firefox.
I can see the sense in replacing it for its default associated mimetypes, to simplify setup, but not in overriding any other mimetype associations.
</pre>
<br />
<p>- David</p>
<br />
<p>On December 27th, 2011, 2:37 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 Dec. 27, 2011, 2:37 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 changes kfmclient so that it
* honors the user configured browser setting when the specified URL is a local page that is to be handled by a browser.
* honors the user configured browser setting when the user specified url is ftp.</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;">Change the browser in the default application list to "firefox" and make sure all of the following commands open the URL in firefox:
* kfmclient openURL http://www.kde.org
* kfmclient openURL /usr/share/doc/qt/html/index.html
* kfmclient openURL ftp://ftp.kde.org</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=182591">182591</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/client/kfmclient.cpp <span style="color: grey">(339ba31)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103524/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>