Make file filters extensible by turning them into an interface, instead of concrete class. - #64
Make file filters extensible by turning them into an interface, instead of concrete class.#64arturaz wants to merge 1 commit into
Conversation
…ad of concrete class.
yasirkula
commented
Apr 13, 2022
Thank you for the PR. How did you benefit from this change in your own projects? What did your custom IFilter implementations do differently than Filter? |
arturaz
commented
Apr 13, 2022
We needed to filter based on regex, something like |
arturaz
commented
Apr 18, 2022
Is there any improvement that should be made to get this merged? |
I couldn't inspect most of the recent PRs in detail due to my busy schedule for a long time. About this feature, I was originally thinking about adding an event to FileBrowser to filter the files with custom C# code (regex can be applied in that event). That solution seems cleaner to me as I'd like to keep file filters as clean and simple as possible. P.S. Actually I forgot that I've already added that event in a past release: |
yasirkula
left a comment
There was a problem hiding this comment.
I've decided to merge this request. Please review my final requests and let me know if you have any questions ^^
| // This is a nonexisting file | ||
| string filename = filenameInput.Substring( startIndex, filenameLength ); | ||
| if( m_pickerMode != PickMode.Folders && filters[filtersDropdown.value].extensions != null ) | ||
| if( m_pickerMode != PickMode.Folders && !filters[filtersDropdown.value].isAllFilesFilter ) |
There was a problem hiding this comment.
We can change it as follows and remove the isAllFilesFilter property: m_pickerMode != PickMode.Folders && filters[filtersDropdown.value] != allFilesFilter
| { | ||
| HashSet<string> extensions = Instance.filters[i].extensionsSet; | ||
| if( extensions != null && extensions.Contains( defaultFilter ) ) | ||
| if( Instance.filters[i].isValidExtension( defaultFilter ) ) |
There was a problem hiding this comment.
We can change it as follows and remove the isValidExtension property: Instance.filters[i].MatchesExtension( defaultFilter, defaultFilter.LastIndexOf( '.' ) != 0 ). Even better, the value of defaultFilter.LastIndexOf( '.' ) != 0 can be cached in a bool defaultFilterHasMultipleSuffixes variable before the for-loop.
| #region Inner Classes | ||
| public class Filter | ||
| public interface IFilter { |
There was a problem hiding this comment.
Please get rid of the free space and put the curly bracket on a new line for consistency (I'm asking this since this is a small commit).
arturaz
commented
Oct 11, 2022
via email
We needed to filter based on regex, something like `new Regex(".data-.+?-ourextension")`. This allowed us to implement the custom filter for this case. …---- On Wed, 13 Apr 2022 19:42:11 +0300 Süleyman Yasir KULA ***@***.***> wrote ----
Thank you for the PR. How did you benefit from this change in your own projects? What did your custom IFilter implementations do differently than Filter?
—
Reply to this email directly, #64 (comment), or https://github.com/notifications/unsubscribe-auth/AAADFA2ELI2LJC3F6ILB3PDVE32OHANCNFSM5TKNPIXA.
You are receiving this because you authored the thread. |
yasirkula
commented
Oct 11, 2022
I'm open to merging this pull request but please see my review and also merge the latest commit on the mainstream to your branch. |
No description provided.