feat: Implement searching for the Table page #158
Conversation
There was a problem hiding this comment.
There is a bug now when switching between Extract Files and Table View tabs after starting a search but before canceling it. The current search filter is preserved so the new table can end up filtered in inadvertently. It just looks like the table is empty or smaller than it should be, but there are actually missing rows.
I left some other suggestions and items. Let me know if you have questions or concerns. Thanks again!
| { | ||
| foreach(object obj in objects) | ||
| { | ||
| if (obj.ToString().Contains(needle, comparisonType)) |
There was a problem hiding this comment.
This needs to be if (obj != null && obj.ToString().Contains(needle, comparisonType)). Without it I was encountering exceptions.
There was a problem hiding this comment.
@activescott do you happen to have an example MSI to test this on?
| (o, args) => Presenter.BeginSearching(args.SearchString), | ||
| (o, args) => { Presenter.BeginSearching(""); } | ||
| Presenter.ExecuteFileSearch, | ||
| () => { Presenter.ExecuteFileSearch(this.fileGrid, string.Empty); } |
There was a problem hiding this comment.
Seems like we had to make the SearchPanel smart enough to know that his grid could change. This function is already checking for an empty searchPanel and creating it once if needed. I wonder if it wouldn't be cleaner to just do this in the cancel action/callback here:
searchPanel.Dispose();
searchPanel = null;
Then searchPanel is always refreshed for every search and you don't have to worry about it's state getting out of sync and event handle unsubscribing, etc.
There was a problem hiding this comment.
Would you like me to keep it as events in this case, as to minimize the amount of changes needed?
There was a problem hiding this comment.
And some events still need to be de-registered, for example:
the search controls registers itself to the resize event of a tab page, so at least this one needs to be unregistered on dispose.
There was a problem hiding this comment.
No concern about these callback events.
Regarding need to unsubscribe: I remember when I wrote this thinking that calling Dispose unsubscribes, but I think you're right and it won't do that (it was late :)). Given the need to remove Parent_Resize maybe your approach is cleaner after all. I'm fine either way. Please just do some testing and you decide. Thanks again for walking me through this! This UI code all makes me nervous because it doesn't have good automated tests so please bare with me on my questions!
There was a problem hiding this comment.
Yeah, sadly it is very hard to reliably test UI.
The dispose would only add value in an instance like this if we add an OnDispose event handler, and actively unsubscribe in that.
In this particular instance, it does make the code slightly more obvious when doing that, and re-adding an assert back in the search function that the last search grid is actually the current one.
Another thing that could be an option is to create one search dialog per tab.
Then a search could even be remembered when switching back to a tab.
There was a problem hiding this comment.
I think "remembering" the search between tabs is going to be more confusing to the user. Better to clear it probably (the app might remember, but the user won't :)). Again, as for dispose or not, you decide. Just please test and make sure it works good and I'll trust your judgement.
| } | ||
|
|
||
| internal void ExecuteTableSearch(DataGridView _, string searchTerm) |
There was a problem hiding this comment.
Lets remove the DataGridView arg here. There is no reason for it. in ExecuteFileSearch it would be ideal to refactor that so that it was using the IMainFormView too so it wrote View.SetFileGridDataSource in the same way this one does.
|
@learn-more Please tag me or assign to me to review or something whenever you're ready. I don't want to hold you up! |
|
@learn-more I'm going to go ahead and close this since it's quite stale, but feel free to re-open it if you're interested. I'd be happy to pitch in and help you get it over the line! |
#134
This renames the 'Search File' to a more generic 'Search'.
Searching is now also case insensitive.