----------------------------------------------------------- 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
