IPED icon indicating copy to clipboard operation
IPED copied to clipboard

Filter manager #39

Open patrickdalla opened this issue 1 year ago • 39 comments

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.

patrickdalla avatar Mar 31 '23 18:03 patrickdalla

Thank you @patrickdalla!

lfcnassif avatar Mar 31 '23 20:03 lfcnassif

This is a fantastic work @patrickdalla! I'm really sorry about taking too long to test and integrate this...

I would like some opinion of others about a possible alternative position of the new tab. Current position is next to the Evidences tab:

image

My concern is the new Tab becoming hidden on small screens. A second option would be in the bottom-center panel (but it's not conceptually related to the other tabs):

image

A third option would be to change all left tabs layout to use a vertical TabbedPane (should be better on smaller screens):

image

Any other suggestion?

lfcnassif avatar Feb 27 '24 21:02 lfcnassif

And about the new TableHeader filter, it's very very useful! image

But I didn't see any indication about the new feature on UI and the user should guess right click on the table header pops up the new Dialog. I think a custom TableHeader Renderer should help, but building new Swing components is not my strength... @wladimirleite, do you think you could help with a new table header renderer when you have available time?

lfcnassif avatar Feb 27 '24 21:02 lfcnassif

@wladimirleite, do you think you could help with a new table header renderer when you have available time?

Sure!

wladimirleite avatar Feb 27 '24 21:02 wladimirleite

About the tab placement, I think I prefer the first option. Maybe it is possible to make the tabs a bit more compact (1) and use a shorter tab name ("Filters") to help keeping all tabs visible, even on relatively small screens.

(1) I can try to help with this part, as I already made them a bit more compact in the past.

wladimirleite avatar Feb 27 '24 22:02 wladimirleite

I would like some opinion of others about a possible alternative position of the new tab.

A long ago, when I registered the FilterManager feature request, I thought to display it as a "flying" panel displayed when user clicks on the "Clear Filters" button, it would become a "FilterManager" button. But displaying it as a new Tab seems better.

lfcnassif avatar Feb 28 '24 02:02 lfcnassif

Hi @patrickdalla, sorry for disturbing you again, but I need an explanation about the general behavior of the "Combined Filter" feature. My guess was that each filter node in the Combined Filter tree was a fixed item set (stored using a bitset or a roaring bitmap). The duplicates filter, although dynamic, when used as a Combined Filter node, seems to use that approach, representing a fixed set of items. But, the "Checked Items" filter, when used as a node in the Combined Filter, gives different results if I change the current checked items in the case. Is that expected? If yes, wouldn't it be better to always use fixed sets as nodes of the Combined Filter tree, always using the same behavior for all Combined Filter nodes?

lfcnassif avatar Mar 01 '24 19:03 lfcnassif

I'm pausing for today. Documenting here to fix: image

I think above checkboxes were intended to be disabled, so user can turn off specific filters from a central place, that's a good idea, but it's not working, the checkboxes are not editable.

lfcnassif avatar Mar 01 '24 21:03 lfcnassif

I have implemented a "tag" interface named IMutableFilter. When a filter implements this, it is not "cached", i. e., its bit set is not cached and a request is made to recalculate it every time it is used. The CheckedFilter implements it. It was a design option I made when implementing it, but you can choose to remove it if you prefer. Test and choose whatever behaviour you prefer.

Em sex., 1 de mar. de 2024, 16:42, Luis Filipe Nassif < @.***> escreveu:

Hi @patrickdalla https://github.com/patrickdalla, sorry for disturbing you again, but I need an explanation about the general behavior of the "Combined Filter" feature. My guess was that each filter node in the Combined Filter tree was a fixed item set (stored using a bitset or a roaring bitmap). The duplicates filter, when used as a Combined Filter node, seems to use that approach, representing a fixed set of items when the duplicate filter was first applied. But, the "Checked Items" filter, when used as a node the Combined Filter, seems to give different results if I change the current checked items in the case. Is that expected? If yes, wouldn't it be better to always use fixed sets as nodes of the Combined Filter tree?

— Reply to this email directly, view it on GitHub https://github.com/sepinf-inc/IPED/pull/1613#issuecomment-1973808629, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG247S7H27BZSMV6R6HYMALYWDK33AVCNFSM6AAAAAAWO7KTXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZTHAYDQNRSHE . You are receiving this because you were mentioned.Message ID: @.***>

patrickdalla avatar Mar 02 '24 00:03 patrickdalla

The IMutableFilter interface marks the filter to recalc its bitset whenever applied. If the filter does not implements it, its bitset is calculated at creation time and not recalculated anymore. I don't know if have implemented pesistence of this behaviour. i. e., if I close IPED and open it again its original bitset would be loaded? Check if this behaviour is kept between IPED executions Nassif, and if it should be kept. Anyway, these are design options you can change if you prefer

Em sex., 1 de mar. de 2024, 21:36, Patrick Bernardina < @.***> escreveu:

I have implemented a "tag" interface named IMutableFilter. When a filter implements this, it is not "cached", i. e., its bit set is not cached and a request is made to recalculate it every time it is used. The CheckedFilter implements it. It was a design option I made when implementing it, but you can choose to remove it if you prefer. Test and choose whatever behaviour you prefer.

Em sex., 1 de mar. de 2024, 16:42, Luis Filipe Nassif < @.***> escreveu:

Hi @patrickdalla https://github.com/patrickdalla, sorry for disturbing you again, but I need an explanation about the general behavior of the "Combined Filter" feature. My guess was that each filter node in the Combined Filter tree was a fixed item set (stored using a bitset or a roaring bitmap). The duplicates filter, when used as a Combined Filter node, seems to use that approach, representing a fixed set of items when the duplicate filter was first applied. But, the "Checked Items" filter, when used as a node the Combined Filter, seems to give different results if I change the current checked items in the case. Is that expected? If yes, wouldn't it be better to always use fixed sets as nodes of the Combined Filter tree?

— Reply to this email directly, view it on GitHub https://github.com/sepinf-inc/IPED/pull/1613#issuecomment-1973808629, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG247S7H27BZSMV6R6HYMALYWDK33AVCNFSM6AAAAAAWO7KTXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZTHAYDQNRSHE . You are receiving this because you were mentioned.Message ID: @.***>

patrickdalla avatar Mar 02 '24 00:03 patrickdalla

I have implemented a "tag" interface named IMutableFilter. When a filter implements this, it is not "cached", i. e., its bit set is not cached and a request is made to recalculate it every time it is used.

Thanks for the explanation! Now it makes sense to me. Seems a reasonable behavior for the Checked Items filter.

I don't know if have implemented pesistence of this behaviour. i. e., if I close IPED and open it again its original bitset would be loaded?

I'll check it. But I don't think filters results must be persisted between application executions, it's not a requirement from my side.

lfcnassif avatar Mar 02 '24 01:03 lfcnassif

Maybe it is possible to make the tabs a bit more compact (1) and use a shorter tab name ("Filters") to help keeping all tabs visible, even on relatively small screens. (1) I can try to help with this part, as I already made them a bit more compact in the past.

@lfcnassif, I made a few quick changes to make tabs more compact. I think it would be possible to have "Categories", "Evidences" and "Filters" tabs visible, even on relatively small screens. The changes would also be useful for the central bottom tabs (Hits, Duplicates, Referenced By etc.), as even in larger screens some tabs are currently hidden. As any visual change, it is kind of a personal taste, in this case between more or less compact layout. I will finish some pending details on Monday and will create a branch so you can take a look and see if it is worth changing.

wladimirleite avatar Mar 02 '24 14:03 wladimirleite

Thank you @wladimirleite!

I think above checkboxes were intended to be disabled, so user can turn off specific filters from a central place

@patrickdalla please just confirm if that was the goal. If yes, re-enabling the filters from the checkboxes should also work? I think that may need more changes to send the filters values to their corresponding panels, right?

lfcnassif avatar Mar 02 '24 14:03 lfcnassif

I don't remember well. But I think it would be good to disable a filter temporarely without removing it. Sorry if it is not implemented. Maybe we can remove them and add this feature as an future enhancement issue.

Em sáb., 2 de mar. de 2024, 11:48, Luis Filipe Nassif < @.***> escreveu:

Thank you @wladimirleite https://github.com/wladimirleite!

I think above checkboxes were intended to be disabled, so user can turn off specific filters from a central place

@patrickdalla https://github.com/patrickdalla please just confirm if that was the goal. If yes, re-enabling the filters from the checkboxes should also work? I think that may need more changes to send the filters values to their corresponding panels, right?

— Reply to this email directly, view it on GitHub https://github.com/sepinf-inc/IPED/pull/1613#issuecomment-1974820227, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG247S4PDXJB6QBNBWGGVHDYWHRD5AVCNFSM6AAAAAAWO7KTXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZUHAZDAMRSG4 . You are receiving this because you were mentioned.Message ID: @.***>

patrickdalla avatar Mar 02 '24 15:03 patrickdalla

What about an option to save current results on table as a custom filter, and keeping the last X custom filters? It's possible to achieve a similar result using bookmarks, but it is kind a workaround. I can try to implement that.

lfcnassif avatar Mar 02 '24 22:03 lfcnassif

image

With last commits, checkboxes are now editable. Unfortunately, for now, user has to select the node first to be able to deselect the checkboxes with a second click. I wasn't able to make it work with one click, after a few hours trying some approaches... If the selection model of the Tree is set to null (no selection allowed), it works, but dragging nodes to the bottom panel stops working. Seems the node selection is taking priority over the checkbox click.

lfcnassif avatar Mar 03 '24 15:03 lfcnassif

I wasn't able to make it work with one click

Just fixed.

lfcnassif avatar Mar 03 '24 23:03 lfcnassif

@lfcnassif, I made a few quick changes to make tabs more compact. I think it would be possible to have "Categories", "Evidences" and "Filters" tabs visible, even on relatively small screens. The changes would also be useful for the central bottom tabs (Hits, Duplicates, Referenced By etc.), as even in larger screens some tabs are currently hidden. As any visual change, it is kind of a personal taste, in this case between more or less compact layout. I will finish some pending details on Monday and will create a branch so you can take a look and see if it is worth changing.

I pushed the changes to a branch named "CompactTabs". image

wladimirleite avatar Mar 04 '24 11:03 wladimirleite

I pushed the changes to a branch named "CompactTabs".

Thank you, @wladimirleite! I just took a look, I'm not sure which tab size I prefer. So I'll defer the decision to you, ok?

lfcnassif avatar Mar 04 '24 15:03 lfcnassif

Thank you, @wladimirleite! I just took a look, I'm not sure which tab size I prefer. So I'll defer the decision to you, ok?

I prefer the new option (more compact). It also includes very minor visual glitches fixes (shown below). And the "document" icon is kind of useless, as it is the same for all tabs. image

wladimirleite avatar Mar 04 '24 16:03 wladimirleite

Well, I would need new eyes to see those details :-)

lfcnassif avatar Mar 04 '24 16:03 lfcnassif

https://github.com/sepinf-

inc/IPED/commit/1655eed7940b1c8681d123beecd42c01cdf43c71

Hi nassif,  i could not test, but, AFAIK, isSelected in renderer method means highlighted, not checked.

I think this simplification might affect multiple value check. Maybe you have tested, if so, ignore this comment.

you can navigate in items with keyboard, highlighting them (isSelected is true) but not checking them till you find the one you really want to check.

patrickdalla avatar Mar 06 '24 12:03 patrickdalla

Hi nassif,  i could not test, but, AFAIK, isSelected in renderer method means highlighted, not checked.

Hi @patrickdalla. Yes, I know, thanks. I kept original behavior, where the checkboxes were already being set or unset simply highlighting and unhighlighting rows. I found it pretty uncommon and I may change. So, what is the expected behavior? Thanks for reviewing my changes!

lfcnassif avatar Mar 06 '24 12:03 lfcnassif

I think that with the implementation i've made, the user could even search for a term, check one of the options presented with this term, and later search again another option but with a complete different term, and check it also.

So the original highlight is only to make easier the keyboard navigation and selection of the items that would be checked (with space keyboard).

I think this behaviour is what I intended to achieve.

patrickdalla avatar Mar 06 '24 12:03 patrickdalla

I think that with the implementation i've made, the user could even search for a term, check one of the options presented with this term, and later search again another option but with a complete different term, and check it also.

This behavior changed, I'll rollback.

So the original highlight is only to make easier the keyboard navigation and selection of the items that would be checked (with space keyboard).

This wasn't working, highlighting was causing checking. I'll try to fix.

lfcnassif avatar Mar 06 '24 12:03 lfcnassif

anyway, if you prefer to keep it simple, you can change

patrickdalla avatar Mar 06 '24 12:03 patrickdalla

Just refactored the code to use JTable, since JCheckBoxes into a JList are not easily editable. Now behavior is much better.

lfcnassif avatar Mar 06 '24 23:03 lfcnassif

Hi @patrickdalla, in last commit I disabled equals and startsWith context menu filters for String fields, they are not working correctly. They can't be implemented properly with Queries (IQueryFilter) for tokenized String fields, like name. I will leave the fix to you when you return. I also suggest you to put some tip about this new Ctrl+RightClick context menu on result table values, most users won't notice the new feature without a tip.

lfcnassif avatar Mar 08 '24 00:03 lfcnassif

@wladimirleite, I tried to implement a custom table column header with a "button" to open the new table header filter, as I anticipated, it was very ugly :-) If you could help with that when you have available time, that would be great. If yes, please let me know if you think it's better to implement it into this PR or in a separate one (so I could merge this after I finish my review).

PS: The new control action should call the MouseListener code at the end of MetadataValueSearchList class.

lfcnassif avatar Mar 08 '24 03:03 lfcnassif

@wladimirleite, I tried to implement a custom table column header with a "button" to open the new table header filter, as I anticipated, it was very ugly :-) If you could help with that when you have available time, that would be great. If yes, please let me know if you think it's better to implement it into this PR or in a separate one (so I could merge this after I finish my review).

PS: The new control action should call the MouseListener code at the end of MetadataValueSearchList class.

I thought a bit about this. Not sure if there is a "beautiful" solution. I will try to implement what I have in mind, in a separated branch, so you can take a look, and eventually improve it.

wladimirleite avatar Mar 08 '24 09:03 wladimirleite