Skip to content

Conversation

@jjw24
Copy link
Member

@jjw24 jjw24 commented May 30, 2021

No description provided.

@jjw24 jjw24 requested a review from taooceros May 30, 2021 05:18
@jjw24 jjw24 self-assigned this May 30, 2021
@taooceros
Copy link
Member

taooceros commented May 30, 2021

Why not use binding instead of a seperated function triggered when checked

@jjw24
Copy link
Member Author

jjw24 commented May 31, 2021

Why not use binding instead of a seperated function triggered when checked

Crossed out?

@taooceros
Copy link
Member

Why not use binding instead of a seperated function triggered when checked

Crossed out?

Crossed out because the function require additional checking (but still can be done in binding, but is similarly complex). Let me check whether we can simplify this process.

1. Moving Property match to Settings.cs
2. Use Binding to avoid explicit Function Assign
3. Simplify ActionKeywordMatch
@taooceros
Copy link
Member

hmmm some logic is weird. Give me some time to fix that.

@taooceros
Copy link
Member

Do we want these setting be the same? Or they are not supposed to be the same (the same just to use SearchActionkeyword)

@jjw24
Copy link
Member Author

jjw24 commented Jun 2, 2021

Do we want these setting be the same? Or they are not supposed to be the same (the same just to use SearchActionkeyword)

what settings are you referring to?

@taooceros
Copy link
Member

Do we want these setting be the same? Or they are not supposed to be the same (the same just to use SearchActionkeyword)

what settings are you referring to?

The four action keywords.

@jjw24
Copy link
Member Author

jjw24 commented Jun 2, 2021

same as in using the global action keyword? then yes except for content search which default is doc:

@taooceros
Copy link
Member

same as in using the global action keyword? then yes except for content search which default is doc:

Well in my last commit, I make it to be exclusive. I would consider user should use the SearchActionkeyword when they want index search and path search with one actionkeyword, use PathSearchActionkeyword or IndexOnlyActionkeyword when they only wants a specific location.
Maybe there's no need for allowing duplicate actionkeyword, which will increase the complexity of updating actionkeyword.

@jjw24
Copy link
Member Author

jjw24 commented Jun 2, 2021

same as in using the global action keyword? then yes except for content search which default is doc:

Well in my last commit, I make it to be exclusive. I would consider user should use the SearchActionkeyword when they want index search and path search with one actionkeyword, use PathSearchActionkeyword or IndexOnlyActionkeyword when they only wants a specific location.
Maybe there's no need for allowing duplicate actionkeyword, which will increase the complexity of updating actionkeyword.

Yeah, the changes in this pr is intending to allow search as how it is functioning before, plus those individual action keywords when enabled.

No need to worry about allowing dup action keyword for this, if we going to tackle it better do it in a separate pr.

@jjw24
Copy link
Member Author

jjw24 commented Jun 2, 2021

Only duplicate action keyword that is allowed is global action keyword

@taooceros
Copy link
Member

Only duplicate action keyword that is allowed is global action keyword

yes you are right, because that's handled by Flow's plugin system.

@taooceros taooceros added enhancement New feature or request Explorer Plugin Issue or Enhancement Link to Explorer Plugin labels Jun 4, 2021
@jjw24
Copy link
Member Author

jjw24 commented Jun 4, 2021

if this is good to go, shall we merge it into your pr?

@taooceros
Copy link
Member

if this is good to go, shall we merge it into your pr?

Sure, I just waiting for your review.

@taooceros taooceros merged commit 9d7ff08 into ExplorerPathActionkeyword Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Explorer Plugin Issue or Enhancement Link to Explorer Plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants