-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Filter manager #39 #1613
Filter manager #39 #1613
Conversation
be responsible for the Combo of predefined filters also.
some internal classes.
adapts UICaseSearchFilter to extends this class.
individual selected value filtering options)
CombinedFilterer must implement, preventing it to cache result bitsets, as it may change.
operand nodes. Solving some concurrency bugs.
some related code.
Just pushed the optimization, now it seems to work correctly and the filters are very fast. I'll still implement the TODOs for multivalued string fields. |
I think I'm done, I'll just do some code clean up. Tests by others would be very welcome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this now is good to merge. Thank you @patrickdalla and @wladimirleite for your work on this!
Left as future improvements:
Paint Table Tab using red color when a table header filter is applied(done)- Put the sorting and numeric scale sliders already used by the MetadataPanel as top components in the new table header filters pop ups
Another future work:
|
Done, just pushed to master. |
@marcus6n now that you finished reviewing #2135, I would like your help to test the features and changes integrated by this PR, because this was a big work and I did many fixes and changes. @patrickdalla is on vacation so he couldn't review and test my changes, but since they were a lot, I would like testing from someone else. What needs to be tested:
You can use the master branch for all tests, since this PR was already integrated. Thank you for your help! |
@lfcnassif Okay, I'll run the tests and get back to you with my feedback once I've completed them! Thanks. |
One tip about testing old filters, after processing a case using master, you can copy it to another folder and replace the contents of iped/lib and iped/localization folders for those from the 4.1.5 version in the copied case. Then open the 2 cases at the same time and compare the filters results between the 2 cases, they must be the same. I've found a behavior regression and fixed it using that approach. |
Just remembered the new context menu fast filter to test. Select a value on the results table, Ctrl+right click it, the fast filter menu will show up. Please test it with single and multivalued columns with string, date and number values. |
Hi @lfcnassif, I just tested the features and changes integrated by this PR (#1613) and everything worked perfectly! I used two sample files to perform the tests and the filters worked as expected. |
Thank you @marcus6n. By two sample files you mean two sample forensic images, right? |
@lfcnassif Exactly, I used two samples of forensic images. |
Thank you @wladimirleite for reporting, I'll try to take a look in the afternoon. |
@patrickdalla, should combined filter allow more than 1 node corresponding to the same filter? |
When placing a filter in Combined Filter, in reality its clone is placed.
I. e. the second time you move the same filter to CB, in other node, a
second clone is placed.
Em qua., 10 de abr. de 2024, 20:20, Luis Filipe Nassif <
***@***.***> escreveu:
… @patrickdalla <https://github.com/patrickdalla>, should combined filter
allow more than 1 node corresponding to the same filter?
—
Reply to this email directly, view it on GitHub
<#1613 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG247SZM4PQH2S2CESZJZSTY4XJOXAVCNFSM6AAAAAAWO7KTXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBYGYZTQNZTHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi nassif. I am reviewing the code. The clone method was planned (its signature is created), but not really implemented. I am doing it now. |
Hi @patrickdalla, thanks for reviewing and testing. But please do it on #2163, I already pushed some fixes, including clone() related ones. Thanks! |
I also uploaded a video showing another issue there. |
I think it is eligible to PR.
Some filter or filterer type particularity, such as how they are represented in tree, or how they are separated in more than one filter, can be reviewed. For example, in FilterSelectedEdge, only one filter is presented representing all selected edges.