> 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. > > David Faure wrote: > 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. > > Dawit Alemayehu wrote: > > 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.
> 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. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103524/#review9228 ----------------------------------------------------------- On Dec. 27, 2011, 2:37 p.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103524/ > ----------------------------------------------------------- > > (Updated Dec. 27, 2011, 2:37 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 > >
