> On Dec. 24, 2011, 8:38 a.m., David Faure wrote: > > konqueror/client/kfmclient.cpp, line 386 > > <http://git.reviewboard.kde.org/r/103524/diff/1/?file=44675#file44675line386> > > > > 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. > > Dawit Alemayehu wrote: > 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. > > David Faure wrote: > 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) > > Dawit Alemayehu wrote: > 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 ? > > David Faure wrote: > 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). > > Dawit Alemayehu wrote: > 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 ? > > Dawit Alemayehu wrote: > ooops. I meant to compare the "desktopEntryName()" of the preferred > services in the example I provided.
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. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103524/#review9228 ----------------------------------------------------------- On Dec. 24, 2011, 10:16 p.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103524/ > ----------------------------------------------------------- > > (Updated Dec. 24, 2011, 10:16 p.m.) > > > Review request for KDE Base Apps and David Faure. > > > Description > ------- > > 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. > > > This addresses bug 182591. > http://bugs.kde.org/show_bug.cgi?id=182591 > > > Diffs > ----- > > konqueror/client/kfmclient.cpp 339ba31 > > Diff: http://git.reviewboard.kde.org/r/103524/diff/diff > > > Testing > ------- > > 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 > > > Thanks, > > Dawit Alemayehu > >
