Skip to content

Conversation

@taooceros
Copy link
Member

@taooceros taooceros commented May 22, 2021

resolve #355

  • Seperated directory and index search, original main search is still intact.

  • Allow action keywords (except file content search) to be disabled, reduces query time when not used.

  • When an action keyword is not used, disable it automatically

  • When user changes a disabled action keyword, enable it automatically

@taooceros taooceros requested a review from jjw24 May 22, 2021 09:24
.editorconfig Outdated
[*.cs]

# IDE0011: 添加大括号
csharp_prefer_braces = when_multiline
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@taooceros taooceros May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I forget to response to this message. I add that because vs is consistently asking me to add brace after if. I don't notice that it will be pushed to the repo. Sorry.

I will be afk for these two days, so I will revise that after tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, I wasn't sure of this option. I might remove it next time I continue to review this pr, if you haven't done do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@jjw24 jjw24 added the enhancement New feature or request label May 26, 2021
@jjw24 jjw24 enabled auto-merge May 26, 2021 09:50
@jjw24
Copy link
Member

jjw24 commented May 26, 2021

can we keep the action keyword to trigger both index and path search?

@jjw24
Copy link
Member

jjw24 commented May 26, 2021

i am making some enhancements to allow using the original search action keyword or dedicated path and index action keywords. will push up soon.

@jjw24
Copy link
Member

jjw24 commented May 26, 2021

this issue should resolve 355 not 292 right?

@taooceros
Copy link
Member Author

this issue should resolve 355 not 292 right?

Ah you are right

@jjw24
Copy link
Member

jjw24 commented May 26, 2021

were you ever able to successfully implement allowing duplicate action keywords?

@taooceros
Copy link
Member Author

were you ever able to successfully implement allowing duplicate action keywords?

I only allow duplicate * keyword, but it is possible to implement duplicate action keywords without #227

@taooceros
Copy link
Member Author

Do we want all actionkeyword in Explorer plugin can be the same? Or only Path and Index search, while full-text search will remain distinct?

@jjw24
Copy link
Member

jjw24 commented May 28, 2021

Maybe just hold off if you are going to make changes, I have made some new changes but have not pushed up yet. Will do so tomorrow

@jjw24
Copy link
Member

jjw24 commented May 30, 2021

@taooceros I think it's better to keep the Search Activation action keyword for the scenario where you can still use one action keyword to search index & path. We can add additional action keywords this way as well so we get the best of both scenarios.

I made the changes in #457, please take a look and let me know what you think, merge if all good.

@jjw24 jjw24 added the review in progress Indicates that a review is in progress for this PR label May 31, 2021
taooceros and others added 4 commits June 1, 2021 18:10
1. Moving Property match to Settings.cs
2. Use Binding to avoid explicit Function Assign
3. Simplify ActionKeywordMatch
update to ExplorerPathActionkeyword branch
@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

found an issue with current action keyword binding, to repro- change file content keyword will reveal that the current action keyword is already assigned.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

binding causes cancel to be ineffective too, think we can switch OK to Done and remove Cancel

@taooceros
Copy link
Member Author

binding causes cancel to be ineffective too, think we can switch OK to Done and remove Cancel

Yeah if that's OK for you. I think I forget to implement cancel. 😂😖

@taooceros
Copy link
Member Author

Oh you mean when the action keyword is global, and disabled, user click done it is supposed to enable the action keyword?

yeah something like that. When user want to enable global, it will return false and show users that this is already assigned, because the old one is equal to the new one.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Hold on it shouldnt show the message to tell user there is already one assigned in my latest commit, Are you working off that?

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Basically it should behave as- do not update the search keyword if it (global/non global) is already assigned.

@taooceros
Copy link
Member Author

Hold on it shouldnt show the message to tell user there is already one assigned in my latest commit, Are you working off that?

Not sure whether I have changed something wrong. Let me take a look on the current commit.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Additionally, if action keyword is disabled, and user changes action keyword, then enable it automatically.

If action keyword is enabled and user disable it, automatically revert to global

@taooceros
Copy link
Member Author

Additionally, if action keyword is disabled, and user changes action keyword, then enable it automatically.

If action keyword is enabled and user disable it, automatically revert to global

Yes, I see these logic and keep them.

There's a logic issue on the last commit. If the IsActionKeywordAssigned return false, it will still close the window, but not to tell user that they are supposed to change some setting that is not allowed. I believe that why you don't discover the issue.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Additionally, if action keyword is disabled, and user changes action keyword, then enable it automatically.
If action keyword is enabled and user disable it, automatically revert to global

Yes, I see these logic and keep them.

There's a logic issue on the last commit. If the IsActionKeywordAssigned return false, it will still close the window, but not to tell user that they are supposed to change some setting that is not allowed. I believe that why you don't discover the issue.

Yes good pick up lol, I forgot abt that use case.

…etting.xaml

1. It will automatically focus on the textbox
2. Enter will trigger the done button
@taooceros
Copy link
Member Author

I finish the editing. Now, ActionKeywordView is linked to setting.actionkeyword, and it will only be trigger when hitting the done button. We can add the Cancel Button back if you want because I use two temp field to store ActionKeyword and Enabled for binding (because I directly linked ActionKeywordView to setting, so using another instance of that won't work for this design).

@jjw24
Copy link
Member

jjw24 commented Jun 6, 2021

let me take it for a spin

We can add the Cancel Button back if you want because I use two temp field to store ActionKeyword and Enabled for binding (because I directly linked ActionKeywordView to setting, so using another instance of that won't work for this design).

I dont really mind not having the cancel button, i think this is more modern design right? the only thing i think worth thinking about is it's not consistent with the rest of the plugins, but i would prefer modern design and put in effort to move the default plugins as well as Flow's interface towards that.

@jjw24
Copy link
Member

jjw24 commented Jun 6, 2021

Getting this error when i change search action keyword to e (which works and triggers) then switch back to global (then nothing works, no results from any plugin displays):

Exception thrown: 'System.Collections.Generic.KeyNotFoundException' in System.Private.CoreLib.dll
Exception thrown: 'System.Reflection.TargetInvocationException' in System.Private.CoreLib.dll
System.Windows.Data Error: 8 : Cannot save value from target back to source. BindingExpression:Path=QueryText; DataItem='MainViewModel' (HashCode=8662426); target element is 'TextBox' (Name='QueryTextBox'); target property is 'Text' (type 'String') KeyNotFoundException:'System.Collections.Generic.KeyNotFoundException: The given key 'e' was not present in the dictionary.
at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
at Flow.Launcher.ViewModel.MainViewModel.RemoveOldQueryResults(Query query) in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 564
at Flow.Launcher.ViewModel.MainViewModel.QueryResults() in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 471
at Flow.Launcher.ViewModel.MainViewModel.Query() in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 352
at Flow.Launcher.ViewModel.MainViewModel.set_QueryText(String value) in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 270'
The thread 0x2acc has exited with code 0 (0x0).
The thread 0x54b0 has exited with code 0 (0x0).
The thread 0x35c0 has exited with code 0 (0x0).
The thread 0x4770 has exited with code 0 (0x0).

@taooceros
Copy link
Member Author

We can add the Cancel Button back if you want because I use two temp field to store ActionKeyword and Enabled for binding (because I directly linked ActionKeywordView to setting, so using another instance of that won't work for this design).

I dont really mind not having the cancel button, i think this is more modern design right? the only thing i think worth thinking about is it's not consistent with the rest of the plugins, but i would prefer modern design and put in effort to move the default plugins as well as Flow's interface towards that.

Well, I am not sure whether not having a cancel button is a more modern design (in fact, what's a more modern design is hard to be argued).

@taooceros
Copy link
Member Author

Getting this error when i change search action keyword to e (which works and triggers) then switch back to global (then nothing works, no results from any plugin displays):

Exception thrown: 'System.Collections.Generic.KeyNotFoundException' in System.Private.CoreLib.dll
Exception thrown: 'System.Reflection.TargetInvocationException' in System.Private.CoreLib.dll
System.Windows.Data Error: 8 : Cannot save value from target back to source. BindingExpression:Path=QueryText; DataItem='MainViewModel' (HashCode=8662426); target element is 'TextBox' (Name='QueryTextBox'); target property is 'Text' (type 'String') KeyNotFoundException:'System.Collections.Generic.KeyNotFoundException: The given key 'e' was not present in the dictionary.
at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
at Flow.Launcher.ViewModel.MainViewModel.RemoveOldQueryResults(Query query) in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 564
at Flow.Launcher.ViewModel.MainViewModel.QueryResults() in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 471
at Flow.Launcher.ViewModel.MainViewModel.Query() in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 352
at Flow.Launcher.ViewModel.MainViewModel.set_QueryText(String value) in C:\Workbench\Git\Flow.Launcher\Flow.Launcher\ViewModel\MainViewModel.cs:line 270'
The thread 0x2acc has exited with code 0 (0x0).
The thread 0x54b0 has exited with code 0 (0x0).
The thread 0x35c0 has exited with code 0 (0x0).
The thread 0x4770 has exited with code 0 (0x0).

That seems some concurrency issue? Let me take a check on that.

@taooceros
Copy link
Member Author

image
Do we need this logic? I don't believe we have an increment strategy when updating plugins.

@taooceros
Copy link
Member Author

hmmm Let me check for the update logic

@taooceros
Copy link
Member Author

I found the issue. When we have a temp query, and change the actionkeyword for a plugin, it will result the dictionary being changed, but this method will access last time actionkeyword to check for removing results. Therefore, it will result the issue. I will fix it soon.

@taooceros
Copy link
Member Author

I rethink logic about that. We don't need to make it so complicate. Only if current actionkeyword and lastactionkeyword is null, we need to refresh the view. No need for only removing results for certain plugins because every query results will refresh their plugin results else where (which is not incremental). Removing all will be fine and clear.

@jjw24
Copy link
Member

jjw24 commented Jun 13, 2021

From testing, fixed two minor logic issue with file content search and revert to global when disabled, otherwise 💯 , lets get this merged.

@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Jun 13, 2021
@jjw24 jjw24 merged commit ad36f89 into dev Jun 13, 2021
@jjw24 jjw24 deleted the ExplorerPathActionkeyword branch June 13, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate actionkeyword of directory and index search

3 participants