Review Request: Modify the KUrilFilter API once more to accomodate unforseen use case...

[Dawit Alemayehu]

> On 2010-08-30 12:44:47, David Faure wrote:
> > isAlwaysReturnPreferredSearchProvidersOn is quite a mouthful. And yet, I would use "Enabled" instead of "On", to be more Qt-like.
> > 
> > Thinking about all this, there are really two use cases for KUriFilter: "the user's text -might- be a candidate for uri filtering" (e.g. konqueror or krunner), and "the user wants this to be uri-filtered, including web-search fallback" (e.g. konversation's popupmenu; pasting onto khtml; any use case not based on a location bar, really, where typos can't happen (*)).
> > 
> > Your setter is really about choosing between these two modes. Maybe it could have a better name, like setForceFiltering()? or more precise: setWebSearchFallbackEnabled()?
> > 
> > (*) iirc one of the reasons why the fallback-to-web-search was disabled in konqueror's location bar is that typing /ect instead of /etc would trigger a google search for /ect, unexpectedly.
> > 
> > Even the need for setWebSearchFallbackEnabled() would have been unexpected to Eike, but well, we have to cover both use cases and the only way to do that is to have a setter; and for compatibility it has to be off by default.
> > 
> > => Ship It, but with a better method name if possible.

You are mostly correct about the two use cases KUriFilter currently faces. The exception being that pasting into khtml/kwebkitpart actually requires a check to see if what is being pasted is a url or regular text and hence is not only about search.

As far as the "fallback-to-web-search" feature goes, it was disabled by default not because it gets triggered as a result of a mistype of a local resource, specially not one that starts with a "/". The only mistype that could potentially trigger this feature incorrectly would be a relative path that contains spaces, but there is no support in any of the filter plugins for relative urls. The actual reason behind the disabling of this feature is my desire to stay neutral as to which engine would be used as the default. At the time the people that came up with the concept of "keywords", the now defunct "RealNames", were hoping that they would be hardcoded as the default. My objection to doing that resulted in a configuration option that was defaulted to "None", but can be configured to whatever the user chose. At the time the list of engines to choose from was rather small compared to what is there today. Anyhow, that is rather old history now...

The choice of  isAlwaysReturnPreferredSearchProvidersOn as a function name is indeed quite a mouthful but I did not know what else to name it to adequately describe its purpose.  I feel that the word "PreferredSearchProviders" must be in the function name because this change is completely about what is returned as a list of preferred search providers ; so how about

void setForcePreferredSearchProvidersEnabled(bool);
bool isForcePreferredSearchProvidersEnabled() const;

I think that would be a better fit in this case.

Aaron also said there are issues he wants to discuss about KUriFilter ; so let me see if more changes need to be made and if there are I will post the updated patch here.


- Dawit


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


On 2010-08-27 23:09:20, Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/5172/
> -----------------------------------------------------------
> 
> (Updated 2010-08-27 23:09:20)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The attached patch does the following:
> 
> #1. Adds setAlwaysReturnPreferredSearchProvidersOn/isAlwaysReturnPreferredSearchProvidersOn to ensure that the kuriikwsfilter plugin returns the preferred provider search engine list when no default search engine is specified. Currently it fails to do so and that behavior will not change unless you set the flag using the aforementioned function. 
> With this change instead of having to do the following:
> 
> KUriFilterData filterData(foo);
> filterData.setAlternateDefaultSearchProvider("google"); // hmm... would google be there ?
> if (KUriFilter::self()->filterUri(filterData, QStringList() << "kuriikwsfilter")
>   // do something here...
> 
> With
> 
> KUriFilterData filterData(foo);
> filterData.setAlwaysReturnPreferredSearchProvidersOn(true);
> if (KUriFilter::self()->filterUri(filterData, QStringList() << "kuriikwsfilter")
>   // do something here...
> 
> #2. Deprecates the newly added function filterSearchUri(KUriFilterData&) in favor of one that accepts an enum filterSearchUri(KUriFilterData&, SearchFilterTypes types). That way instead of the caller having to know what "kuriikwsfilter' or 'kurisearchfilter' plugins do, they can specify "NormalTextFilter" or "WebShortcutFilter" or an OR combination of both. This change makes it possible to do search filtering without having to worry about which filters to use. For example, the last if statement from the above example would become:
> 
> if (KUriFilter::self()->filterSearchUri(filterData, KUriFilter::NormalTextFilter))
>    // do something here...
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/runtime/kurifilter-plugins/ikws/kuriikwsfilter.h 1168549 
>   /trunk/KDE/kdebase/runtime/kurifilter-plugins/ikws/kuriikwsfilter.cpp 1168549 
>   /trunk/KDE/kdelibs/kio/kio/kurifilter.h 1168902 
>   /trunk/KDE/kdelibs/kio/kio/kurifilter.cpp 1168902 
> 
> Diff: http://reviewboard.kde.org/r/5172/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100830/77d2afe4/attachment.htm>