Review Request: The WebshortcutRunner did not check whether the search engines it provided were even enabled.
Aaron Seigo
aseigo at kde.org
Wed Mar 3 23:58:08 CET 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3097/#review4356
-----------------------------------------------------------
looks pretty good ... i can think of a few more possible reasonable improvements to this code as well, including:
* registering a separate QuerySyntax entry per enabled shortcut (right now it create one big QuerySyntax string, and that's not exactly accurate)
* caching the matching Service objects in WebshortcutRunner::loadSyntaxes for use later in match to avoid going through them all again and again. doing this would make caching (and re-checking) m_enabledSearchEngines over and over unecessary, and that could just be kept local to loadSyntaxes.
/branches/KDE/4.4/kdebase/workspace/plasma/generic/runners/webshortcuts/webshortcutrunner.h
<http://reviewboard.kde.org/r/3097/#comment3844>
a QSet would probably be faster since it's used for lookups only and order doesn't matter. given that it's likely to be a small set, however, it probably doesn't matter to much.
/branches/KDE/4.4/kdebase/workspace/plasma/generic/runners/webshortcuts/webshortcutrunner.cpp
<http://reviewboard.kde.org/r/3097/#comment3847>
a weakness of this patch, as it stands, is that this file may get changed without sycoca being changed. e.g. if the user enables a web shortcut in the relevant control panel, it won't be picked up immediately by this runner.
loadDelimiter is already called when kuriikwsfilterrc changes; perhaps loadDelimiter could be renamed to something like readFiltersConfig and it could read both the delimiter there as well as call loadSyntaxes.
- Aaron
On 2010-03-03 22:42:44, Nikolaus Waxweiler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3097/
> -----------------------------------------------------------
>
> (Updated 2010-03-03 22:42:44)
>
>
> Review request for Plasma.
>
>
> Summary
> -------
>
> Solution: find out which ones are enabled and compare before presenting a match/advertising a search engine.
>
> I added a QStringList m_enabledSearchEngines which is updated after every relevant change to sycoca and changed 2 places in the runner to query it: when building the runner-help-string to be displayed via krunner's [?]-button and when looking if the first word of the user's string could possibly be a shorthand for/keyword of a search engine.
>
>
> Diffs
> -----
>
> /branches/KDE/4.4/kdebase/workspace/plasma/generic/runners/webshortcuts/webshortcutrunner.h 1098255
> /branches/KDE/4.4/kdebase/workspace/plasma/generic/runners/webshortcuts/webshortcutrunner.cpp 1098255
>
> Diff: http://reviewboard.kde.org/r/3097/diff
>
>
> Testing
> -------
>
> Used rev. 1098255 as the starting point. Compiled it and overwrote the 4.4.1 .so I had installed. Because I'm lazy. I experienced some krunner-crashes, maybe because of this :p Didn't check though. Because I'm lazy.
>
> I then typed the shorthand for some search engine I had disabled ("amg") to check whether the runner would respond. Nope. I then enabled it and ran kbuildsycoca4 manually (the web shortcut dialog in konq. is unreliable with this.. I think there's a few bug reports for this), and the runner responded appropriately. Other shortcuts worked fine.
>
>
> Thanks,
>
> Nikolaus
>
>
More information about the Plasma-devel
mailing list