Pinyin library change #183

taooceros · 2020-10-18T09:52:28Z

Moved from #179

Wox has changed the Pinyin Library to ToolGood.Words. #2962 Thank you for the change @bao-qian .
This pull request has done the same thing, but optimize a few.

It keeps the full pinyin query, which seems perform the same as current pinyin library.
It keeps using the Concurrent Dictionary as cache, instead of Wox's Memory Cache. I think changing the cache implementation should be done in separately. However, the cache for Pinyin is changed from store single chinese character to whole word.
IAlphabet interface is kept in this pull request.
I also merge the query of win32 and uwp in Program plugin into one query by adding enabled property to the IProgram interface and cast win32 and uwp to IProgram.
I have checked the previous cache storage and it actually makes the loading time quite long, so I remove the Save and Loading cache in this pull request because it seems that the library is fast enough.

…anlate to avoid cache.

jjw24 · 2020-10-18T10:24:28Z

Thanks @taooceros! If PR is ready, let us know via request review :)

taooceros · 2020-10-18T13:44:28Z

I think this should be ready now. You are welcome!

Flow.Launcher.Infrastructure/StringMatcher.cs

Plugins/Flow.Launcher.Plugin.Program/Main.cs

Flow.Launcher.Infrastructure/Alphabet.cs

jjw24 · 2020-10-19T09:55:42Z

Flow.Launcher.Infrastructure/Alphabet.cs

-                select $"{a1};{a2}"
-            ).ToArray();
-            return combination;
+            return string.Concat(content.Split(';').Select(x => x.First()));


just a question, why is this code only get the first char, do you know what its used for?

This is for getting the acronym for pinyin, just like our discussion in the acronym search. That should be removed once we have done the acronym search in fuzzy search.

If you are talking why this can get the first char of pinyin, it is because when I get the pinyin, I called it with WordsHelper.GetPinyin(content, ";"), which split Chinese character with ";".

Hmm if this is for acronym search, could we remove this and add it in the other acronym search PR, I have a feeling we going to forget to remove this code.

I have done the part of removing this in the acronym branch. I put it here because I think this will be merged earlier so that I won't like to change the behavior since on the past pinyin acronym search is done with this method.

Sure as long as this acronym search code is only specific to Pinyin

taooceros · 2020-10-19T10:15:59Z

Thank you for the reviews! @jjw24

taooceros · 2020-10-21T09:05:02Z

Plugins/Flow.Launcher.Plugin.Program/Main.cs

-            lock (IndexLock)
-            { // just take the reference inside the lock to eliminate query time issues.
-                win32 = _win32s;
-                uwps = _uwps;
-            }


Bao has removed the lock in Wox, so I think removing it won't be an issue in Flow, too.

I dont suppose those two will be updated often unless a reindex is run. Can the user reindex and also query at the same time?

I would consider even if they query and reindex at the same time, it won't affect each other since index only change the memory index after finishing the whole index process (which is time consuming), but assigning values should not be an issue. On the time that Flow is reindexing, the variable won't change until it finish.

I have provide a small test and it seems that won't fine of querying when reindexing.

Plugins/Flow.Launcher.Plugin.Program/Programs/IProgram.cs

Plugins/Flow.Launcher.Plugin.Program/Main.cs

jjw24 · 2020-10-24T23:38:22Z

Pinyin changes are looking good.

I think if we can rename the Alphabet class that would be sweet.

Can you do me a favour, can we pull out the changes related to Programs plugin - locking and program query optimization. Let's stick to one change set per PR so it's easier/quicker to review and test.

jjw24 · 2020-10-24T23:55:46Z

also please bump the version in Plugin.Program's plugin.json file

taooceros · 2020-10-25T01:36:49Z

Sure, I will rollback the changes for programs plug-ins and create a new one for it. I think if we don't include the optimization of the query, this PR won't include change for program plugin. Should we still bump the version?

Plugins/Flow.Launcher.Plugin.Program/Main.cs

jjw24 · 2020-11-01T09:06:58Z

Just done some testing around the usage, I think the acronym search caused the Chinese to Pinyin search to fail:

lets so i have two entries as below.

searching pinyin is good

using acronym is good

but Chinese to Pinyin, hmmm:

The acronym search has changed the pinyin translation

taooceros · 2020-11-01T09:49:52Z

Hmmmm you are right, I think the original implementation of the acronym search in the pinyin search is done by adding the acronym character to the front of the translated query, which will lead to the weird query translation.

   var combination = PinyinCombination(source);
            
   var pinyinArray=combination.Select(x => string.Join("", x));
   var acronymArray = combination.Select(Acronym).Distinct();

   var joinedSingleStringCombination = new StringBuilder();
   var all = acronymArray.Concat(pinyinArray);
   all.ToList().ForEach(x => joinedSingleStringCombination.Append(x));

   return joinedSingleStringCombination.ToString();

You can see that the current implementation is joining all the combination together including acronym into a large string. I think it doesn't allow we use Chinese query to search for pinyin result as well because of the query translation is also the one that include every combination of polyphone and acronym.

The acronym part will be removed after the native acronym search has been finished, which means that the translation of query will be clean.

jjw24 · 2020-11-01T10:07:29Z

The acronym part will be removed after the native acronym search has been finished, which means that the translation of query will be clean.

yeah that's what I was thinking, once the acronym search is implemented in string matcher, this shouldnt be needed here. Is that PR ready to go?

taooceros · 2020-11-01T10:10:53Z

The acronym part will be removed after the native acronym search has been finished, which means that the translation of query will be clean.

yeah that's what I was thinking, once the acronym search is implemented in string matcher, this shouldnt be needed here. Is that PR ready to go?

It should be ready to go except the highlighted data is hard to control due to the translation. Do you think it is ok when sometimes the highlighted data is not on the right place? Or maybe if it is translated, we don't include the highlight data.

jjw24 · 2020-11-01T10:25:02Z

The acronym part will be removed after the native acronym search has been finished, which means that the translation of query will be clean.

yeah that's what I was thinking, once the acronym search is implemented in string matcher, this shouldnt be needed here. Is that PR ready to go?

It should be ready to go except the highlighted data is hard to control due to the translation. Do you think it is ok when sometimes the highlighted data is not on the right place? Or maybe if it is translated, we don't include the highlight data.

is this when translating Pinyin and matching a Chinese character program? I think the original code doesnt highlight Chinese

taooceros · 2020-11-01T10:38:08Z

The acronym part will be removed after the native acronym search has been finished, which means that the translation of query will be clean.

yeah that's what I was thinking, once the acronym search is implemented in string matcher, this shouldnt be needed here. Is that PR ready to go?

It should be ready to go except the highlighted data is hard to control due to the translation. Do you think it is ok when sometimes the highlighted data is not on the right place? Or maybe if it is translated, we don't include the highlight data.

is this when translating Pinyin and matching a Chinese character program? I think the original code doesnt highlight Chinese

Yes, it only highlights if search with acronym since they are the character that lies one the first few characters and match the chinese character. We can discuss this on #185 .

jjw24 · 2020-11-01T10:40:52Z

The acronym part will be removed after the native acronym search has been finished, which means that the translation of query will be clean.

yeah that's what I was thinking, once the acronym search is implemented in string matcher, this shouldnt be needed here. Is that PR ready to go?

It should be ready to go except the highlighted data is hard to control due to the translation. Do you think it is ok when sometimes the highlighted data is not on the right place? Or maybe if it is translated, we don't include the highlight data.

is this when translating Pinyin and matching a Chinese character program? I think the original code doesnt highlight Chinese

Yes, it only highlights if search with acronym since they are the character that lies one the first few characters and match the chinese character. We can discuss this on #185 .

Ok sounds good.

I havent had a chance to look at 185 yet, will do so soon. Lets not forget to remove this acronym search once 185 is finished.

taooceros · 2020-11-01T10:44:12Z

Of course, the integration has been finished, except for the highlight, and once those two pr has been merged, I will create a new pr to do the job.

jjw24 · 2020-11-01T10:46:22Z

Lets get rid of that white space and then we can merge this in and get a release going :)

bao-qian and others added 7 commits October 18, 2020 17:50

Change Reference

d9085ca

Seems that we don't need storage cache

a1327c1

Change Pinyin Library

3454dc6

change new string to string.concat()

d17621c

change query translation to Program.Main instead of using Alphabet.Tr…

c4b81a9

…anlate to avoid cache.

using ContainKey instead of getvalueofdefault

4ebfba7

fixup the reduced code due to testing

dd2d3c6

taooceros mentioned this pull request Oct 18, 2020

Pinyin improvement (Merge from Wox) #179

Closed

taooceros mentioned this pull request Oct 19, 2020

Add Acronym Support for Fuzzy Search #185

Merged

jjw24 assigned taooceros Oct 19, 2020

jjw24 added enhancement New feature or request review in progress Indicates that a review is in progress for this PR labels Oct 19, 2020

jjw24 reviewed Oct 19, 2020

View reviewed changes

taooceros added 5 commits October 19, 2020 20:37

Fix reverse condition

6210454

Add curly bracket

4bf5b2e

Add curly bracket

9b779d9

Add curly bracket

fb25d54

Put tranlate query back to fuzzy search until better solution

80a2f0d

taooceros commented Oct 21, 2020

View reviewed changes

Plugins/Flow.Launcher.Plugin.Program/Programs/IProgram.cs Outdated Show resolved Hide resolved

taooceros commented Oct 21, 2020

View reviewed changes

Plugins/Flow.Launcher.Plugin.Program/Main.cs Show resolved Hide resolved

taooceros force-pushed the PinyinLibraryChange branch from 8a212a6 to 80a2f0d Compare October 25, 2020 02:22

Initialize uninitialized cache

77aff43

jjw24 requested changes Nov 1, 2020

View reviewed changes

Plugins/Flow.Launcher.Plugin.Program/Main.cs Outdated Show resolved Hide resolved

Plugins/Flow.Launcher.Plugin.Program/Main.cs Outdated Show resolved Hide resolved

Removed unused using due to query translation

f146dd6

Change the Alphabet Name to allow future expansion

eb73656

remove extra whitespace

da801f8

jjw24 approved these changes Nov 1, 2020

View reviewed changes

jjw24 merged commit aa3177d into Flow-Launcher:dev Nov 1, 2020

jjw24 removed the review in progress Indicates that a review is in progress for this PR label Nov 1, 2020

jjw24 mentioned this pull request Nov 1, 2020

Release 1.4.0 | Plugin 1.2.2 #191

Merged

taooceros deleted the PinyinLibraryChange branch November 2, 2020 03:08

taooceros restored the PinyinLibraryChange branch November 2, 2020 03:30

taooceros deleted the PinyinLibraryChange branch November 2, 2020 03:39

taooceros linked an issue Dec 9, 2020 that may be closed by this pull request

Merge performance improvements from Wox #174

Closed

Uh oh!

Pinyin library change #183

Pinyin library change #183

Uh oh!

Conversation

taooceros commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjw24 commented Oct 18, 2020

Uh oh!

taooceros commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjw24 Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

taooceros Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjw24 Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

taooceros Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

jjw24 Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taooceros commented Oct 19, 2020

Uh oh!

taooceros Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

jjw24 Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

taooceros Oct 25, 2020

Choose a reason for hiding this comment

Uh oh!

taooceros Oct 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jjw24 commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjw24 commented Oct 24, 2020

Uh oh!

taooceros commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjw24 commented Nov 1, 2020

Uh oh!

taooceros commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjw24 commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taooceros commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjw24 commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taooceros commented Nov 1, 2020

Uh oh!

jjw24 commented Nov 1, 2020

Uh oh!

taooceros commented Nov 1, 2020

taooceros commented Oct 18, 2020 •

edited

Loading

taooceros commented Oct 18, 2020 •

edited

Loading

taooceros Oct 19, 2020 •

edited

Loading

jjw24 Oct 24, 2020 •

edited

Loading

jjw24 commented Oct 24, 2020 •

edited

Loading

taooceros commented Oct 25, 2020 •

edited

Loading

taooceros commented Nov 1, 2020 •

edited

Loading

jjw24 commented Nov 1, 2020 •

edited

Loading

taooceros commented Nov 1, 2020 •

edited

Loading

jjw24 commented Nov 1, 2020 •

edited

Loading