> 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 ?
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). - 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 > >
