added the abitliy to opt out of pinyin in control panel and in program plug in - this give a major perf boost (and I'm guessing it is not relevant for most users)

theClueless commented 5 years ago

by default pinyin would be enabled (to keep the current behavior). added setting and when set the pinyin search is not used. the perf cost of it is huge.

jjw24 commented 5 years ago

Will take a look later

jjw24 commented 5 years ago

I might want to do some testing on this before concluding. I havent had a chance to look at this PR yet but will definitely take a look, but probably will be a bit later.

I wonder if you could add some tests or show some tests results on the part where disabling pinyin will give major performance boost (which will also help me determine the PR as well). If it is the case I think we can have it as a world wide setting rather than just control panel=> any string matcher searches. Do give consideration to users who may be using bilingual OS eg. search in English and sometimes Chinese.

theClueless commented 5 years ago

I did it for both control panel and program (program being the more important one). My initial thought was to make it a global setting but since stringMatcher is a static class it doesn't seem quite right. as for testing I just used visual studio profiler and it was extremely obvious. just run any profiler with and without it (using my PR and see the difference).

jjw24 commented 5 years ago

On make sense, let me test it out

jjw24 commented 5 years ago

Please see the comments above and let me know what you think. Overall:

For Programs plugin feel free just use Main._settings since it's internal static member (let me know if there is reasoning not to do it, happy to discuss). This will overall make the code look neater and simpler to read.
Control panel plugin you can also directly use the _settings member without having to pass it around the methods. Feel free to make it also internal static so other files in the assembly can also use it directly.

jjw24 commented 5 years ago

Still pondering about making the Pinyin change in the infrastructure project instead of on plugin level for the reasons below:

just so much easier to manage as UserSettings and StringMatcher both live in that project. You could just put the option in the UserSetting and simply put a wrapper for the method ScoreForPinyin which would return 0 as the score if user elects to bypass pinyin search. Look at how StringMatcher.UserSettingSearchPrecision is done as an example.
Shouldn't make the plugin decide whether a Pinyin search is required. For example if a new plugin is added that can cater for Pinyin search, the developer will have to write the same code again for the new plugin to allow user the option to opt out. If the developer forgets to give that option then you end up with some plugins do pinyin and some don't, right? If it's done at lower at infrastructure level then the developer wouldn't need to worry about adding that option but just call the ScoreForPinyin and be done with it. Whether a pinyin search is needed to be carried out, it is up to String.Matcher and the option selected in UserSettings.

Let me know your thoughts on the above.

theClueless commented 5 years ago

Overall I think that using static makes testing nearly impossible in the long run. how can you mock things when you have static members? how can you run isolated tests? thus I only turn to static when I really have to. I really agree that the pinyin should be on the settings of whole Wox. and found a simpler fix for that will create a PR and abandon this one.

jjw24 commented 4 years ago

https://github.com/jjw24/Wox/pull/77 - Closing this as per above comment

jjw24 / Wox

added the abitliy to opt out of pinyin in control panel and in program plug in - this give a major perf boost (and I'm guessing it is not relevant for most users) #67