Review Request: Make kfmclient honor the user configured browser settings for local resources

[Dawit Alemayehu]

> On Dec. 24, 2011, 8:38 a.m., David Faure wrote:
> > FTP requires a file manager, not a web browser. You get a much better experience with a FTP url in e.g. dolphin than in firefox, IMHO. Using web browsers for FTP urls is a historic relic from before proper network transparency in file managers.

Ok. I can remove that change since there was no complaint in the bug report about that.


> On Dec. 24, 2011, 8:38 a.m., David Faure wrote:
> > konqueror/client/kfmclient.cpp, line 358
> > <http://git.reviewboard.kde.org/r/103524/diff/1/?file=44675#file44675line358>
> >
> >     The naming is confusing. From the code, this method really checks if konqueror is the preferred service, right?
> >     
> >     In fact this sounds very much like KonqMainWindow::isMimeTypeAssociatedWithSelf(mimetype). Not that you can call that method, but you could compare the implementations, and call this
> >     isMimeTypeAssociatedWithKonqueror here, or better, isKonquerorPreferred.

Right. I was not really thrilled with that name myself. Its functionality is a little bit different from isMimeTypeAssociatedWithKonqueror since we do not yet have the mimetype so I have renamed it to "isKonquerorPreferredServiceFor". I think that makes is clear what that function is for.


> On Dec. 24, 2011, 8:38 a.m., David Faure wrote:
> > konqueror/client/kfmclient.cpp, line 363
> > <http://git.reviewboard.kde.org/r/103524/diff/1/?file=44675#file44675line363>
> >
> >     The check for == 100 seems too strict to me, you get 80 in many cases, iirc. In fact I'm thinking of removing the accuracy stuff in qt5, because there's no good way of doing something with that number...

Removed it completely. 


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

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.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103524/#review9228
-----------------------------------------------------------


On Dec. 24, 2011, 4:14 a.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, 4:14 a.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20111224/963368b2/attachment.htm>