search

sepinf-inc / IPED

IPED Digital Forensic Tool. It is an open source software that can be used to process and analyze digital evidence, often seized at crime scenes by law enforcement or in a corporate investigation by private examiners.
Other
884 stars 209 forks source link

Individual filterer disable/reenable in filters panel. #2163

Open lfcnassif opened 2 months ago

lfcnassif commented 2 months ago

This will close #39 again when finished. There are more fixes to come, so this is just a draft.

lfcnassif commented 2 months ago

@patrickdalla, I would appreciate if you could take a look if last commit is ok, thanks in advance.

lfcnassif commented 2 months ago

Just found another issue in the combined filter, a video illustrating it is inside zip below, produced before commits above: combined filter issue.zip

I'll wait until @patrickdalla returns from vacation, so he can take a look. I'll focus on other 4.2.0 pending enhancements.

patrickdalla commented 2 months ago

Did you create a new Branch with these modifications?

lfcnassif commented 2 months ago

Yes, its name is at the top of this PR page.

patrickdalla commented 2 months ago

I have noticed you refactored the cachedBitSet map to have FilterNode as its key. Although it is an alternative, it stores in memory redundant copies of same filter bitmaps used in different nodes of the logic.

I had already implemented a solution to the problem with the old MAP specification before noticing this PR (not the node), do you mind if I roll back this? Does this modification address any other issue beyond duplicate filter node removal?

lfcnassif commented 2 months ago

do you mind if I roll back this?

No problem, just revert my last 3 commits and apply yours after reverting them, maybe you can keep 02c580f.

Does this modification address any other issue beyond duplicate filter node removal?

Yes, the first commit fixes an unrelated NPE, please keep it. I also uploaded a zipped video here showing another issue I didn't try to fix.

patrickdalla commented 2 months ago

I think the pointed issues are solved. I have also recovered the functionality that was lost of MOVING and COPYING filterNodes from CombinedFilterer to other Operand Nodes inside the CombinedFilter.

Please, double check.

lfcnassif commented 2 months ago

Hi @patrickdalla, thank you very much for your fixes and improvements here. Since you are working on the FilterManager, I would like to ask you what do you think about making it possible to re-enable last filters from the FilterManager. Do you think it would be straightforward to code? I think it would make sense to let users enable filters from the filter manager, not just disabling them (and combining them of course). I think propagating filters state to their original panel would be more complex and could be left as another future improvement.

patrickdalla commented 2 months ago

Hi @patrickdalla, thank you very much for your fixes and improvements here. Since you are working on the FilterManager, I would like to ask you what do you think about making it possible to re-enable last filters from the FilterManager. Do you think it would be straightforward to code? I think it would make sense to let users enable filters from the filter manager, not just disabling them (and combining them of course). I think propagating filters state to their original panel would be more complex and could be left as another future improvement.

You mean reenable in the check box, keeping the checkbox unchecked so it can be reenabled by checking it again, right?

lfcnassif commented 2 months ago

You mean reenable in the check box, keeping the checkbox unchecked so it can be reenabled by checking it again, right?

Exactly!

patrickdalla commented 2 months ago

I final opinion request on the checkbox filterer enabler, @lfcnassif: When it is disabled, the corresponding tab red color should turn gray, right?

lfcnassif commented 2 months ago

@lfcnassif: When it is disabled, the corresponding tab red color should turn gray, right?

Yes, I think it was already working this way.

patrickdalla commented 2 months ago

Yes, it is working this way. I had a silly momentaneous doubt if it should work also this way if the Filterer is disabled, but it is really better to continue so.

Em sex., 19 de abr. de 2024, 13:51, Luis Filipe Nassif < @.***> escreveu:

@lfcnassif https://github.com/lfcnassif: When it is disabled, the corresponding tab red color should turn gray, right?

Yes, I think it was already working this way.

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

patrickdalla commented 2 months ago

Hi @patrickdalla, thank you very much for your fixes and improvements here. Since you are working on the FilterManager, I would like to ask you what do you think about making it possible to re-enable last filters from the FilterManager. Do you think it would be straightforward to code? I think it would make sense to let users enable filters from the filter manager, not just disabling them (and combining them of course). I think propagating filters state to their original panel would be more complex and could be left as another future improvement.

This feature is also implemented with the last commits. Test and tell if there is anything missing.

lfcnassif commented 2 months ago

Thanks @patrickdalla. I won't have time to test this before Thursday. Could you please mark this as ready for review when finished?

lfcnassif commented 2 months ago

@patrickdalla, I think current behavior is not good. Executing these steps:

Then:

  1. the duplicate filter checkbox on top bar remains checked and its label red, but results are not being filtered on main table
  2. if you disable and enable the top bar duplicate filter multiple times, nothing happens
  3. this happens to all filters

Original filters should be disabled if they are disabled in the new Filters tab (that is working on master). But, if they are re-enabled in the Filters tab, the original filters could remain disabled for now (re-enabling the original filters into their components could be a future improvement), but the Filters tab should be painted in red to flag there is an active filter inside it.

lfcnassif commented 2 months ago

The "Clear Filters" button is also not disappearing anymore after being clicked.

lfcnassif commented 2 months ago

This is also being printed in the log after some filter is applied and before the search bar is used:

iped.exception.QueryNodeException: INVALID_SYNTAX_CANNOT_PARSE: Syntax Error, cannot parse [Type or choose the search expression. Use [TAB] to autocomplete properties.]:  
    at iped.engine.search.QueryBuilder.getQuery(QueryBuilder.java:387)
    at iped.engine.search.QueryBuilder.getQuery(QueryBuilder.java:344)
    at iped.app.ui.App$SearchFilterer.getQuery(App.java:1990)
    at iped.app.ui.CaseSearcherFilter.getQueryWithUIFilter(CaseSearcherFilter.java:120)
    at iped.app.ui.CaseSearcherFilter.applyUIQueryFilters(CaseSearcherFilter.java:91)
    at iped.app.ui.CaseSearcherFilter.applyUIQueryFilters(CaseSearcherFilter.java:86)
    at iped.app.ui.AppListener.updateFileListing(AppListener.java:94)
    at iped.app.ui.AppListener.updateFileListing(AppListener.java:44)
    at iped.app.ui.AppListener.actionPerformed(AppListener.java:153)
    at java.desktop/javax.swing.AbstractButton.fireActionPerformed(Unknown Source)
    at java.desktop/javax.swing.AbstractButton$Handler.actionPerformed(Unknown Source)
    at java.desktop/javax.swing.DefaultButtonModel.fireActionPerformed(Unknown Source)
    at java.desktop/javax.swing.JToggleButton$ToggleButtonModel.setPressed(Unknown Source)
    at java.desktop/javax.swing.plaf.basic.BasicButtonListener.mouseReleased(Unknown Source)
    at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
    at java.desktop/java.awt.Component.processMouseEvent(Unknown Source)
    at java.desktop/javax.swing.JComponent.processMouseEvent(Unknown Source)
    at java.desktop/java.awt.Component.processEvent(Unknown Source)
    at java.desktop/java.awt.Container.processEvent(Unknown Source)
    at java.desktop/java.awt.Component.dispatchEventImpl(Unknown Source)
    at java.desktop/java.awt.Container.dispatchEventImpl(Unknown Source)
    at java.desktop/java.awt.Component.dispatchEvent(Unknown Source)
    at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Unknown Source)
    at java.desktop/java.awt.LightweightDispatcher.processMouseEvent(Unknown Source)
    at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Unknown Source)
    at java.desktop/java.awt.Container.dispatchEventImpl(Unknown Source)
    at java.desktop/java.awt.Window.dispatchEventImpl(Unknown Source)
    at java.desktop/java.awt.Component.dispatchEvent(Unknown Source)
    at java.desktop/java.awt.EventQueue.dispatchEventImpl(Unknown Source)
    at java.desktop/java.awt.EventQueue$4.run(Unknown Source)
    at java.desktop/java.awt.EventQueue$4.run(Unknown Source)
    at java.base/java.security.AccessController.doPrivileged(Native Method)
    at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
    at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
    at java.desktop/java.awt.EventQueue$5.run(Unknown Source)
    at java.desktop/java.awt.EventQueue$5.run(Unknown Source)
    at java.base/java.security.AccessController.doPrivileged(Native Method)
    at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
    at java.desktop/java.awt.EventQueue.dispatchEvent(Unknown Source)
    at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
    at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
    at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
    at java.desktop/java.awt.EventDispatchThread.pumpEvents(Unknown Source)
    at java.desktop/java.awt.EventDispatchThread.pumpEvents(Unknown Source)
    at java.desktop/java.awt.EventDispatchThread.run(Unknown Source)
Caused by: INVALID_SYNTAX_CANNOT_PARSE: Syntax Error, cannot parse [Type or choose the search expression. Use [TAB] to autocomplete properties.]:  
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.generateParseException(StandardSyntaxParser.java:2093)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.jj_consume_token(StandardSyntaxParser.java:1961)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.TermRangeExpr(StandardSyntaxParser.java:1111)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.Term(StandardSyntaxParser.java:1029)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.Clause(StandardSyntaxParser.java:290)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.ModClause(StandardSyntaxParser.java:249)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.ConjQuery(StandardSyntaxParser.java:186)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.DisjQuery(StandardSyntaxParser.java:163)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.Query(StandardSyntaxParser.java:124)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.TopLevelQuery(StandardSyntaxParser.java:114)
    at org.apache.lucene.queryparser.flexible.standard.parser.StandardSyntaxParser.parse(StandardSyntaxParser.java:92)
    at org.apache.lucene.queryparser.flexible.core.QueryParserHelper.parse(QueryParserHelper.java:214)
    at org.apache.lucene.queryparser.flexible.standard.StandardQueryParser.parse(StandardQueryParser.java:280)
    at iped.engine.search.QueryBuilder.getQuery(QueryBuilder.java:380)
    ... 44 more

PS: It also happens before merging master, but with the previous search bar message tip.

lfcnassif commented 2 months ago

The "Clear Filters" button is also not disabling the "Combined Filter".

I'll wait for your fixes before continuing the tests.

patrickdalla commented 2 months ago

@patrickdalla, I think current behavior is not good. Executing these steps:

  • enable the duplicate filter on top bar
  • disable the duplicate filter checkbox in the new Filters tab

Then:

  1. the duplicate filter checkbox on top bar remains checked and its label red, but results are not being filtered on main table
  2. if you disable and enable the top bar duplicate filter multiple times, nothing happens
  3. this happens to all filters

Original filters should be disabled if they are disabled in the new Filters tab (that is working on master). But, if they are re-enabled in the Filters tab, the original filters could remain disabled for now (re-enabling the original filters into their components could be a future improvement), but the Filters tab should be painted in red to flag there is an active filter inside it.

Hi @lfcnassif, back to it: maybe I misunderstood the need.

I thought that the checkbox on the filterer tree was to disable the filters inside the filter, but not remove them, so you could reenable them. So, the checkbox of the duplicate filterer on its original top position won't change, as it was its "state" although disabled by the tree filterer checkbox.

The same with the remaining filterers: i. e. if you have 2 tableheader filters defined and you disable the filterer in the tree, they will remain "defined" in the tableheader filterer, and could be reenabled.

For the case of the duplicate filterer, it really appear a little confusing.

So, just to double check if I understand what do you want now:

patrickdalla commented 2 months ago

With the clear filters, the same: They do not disappear because the at least one filterer has a defined filter, even though the filterer is disabled by the checkbox in the filterers tree.

lfcnassif commented 2 months ago

Hi @patrickdalla, this is my opinion:

  • If the check box in the filterer tree is clicked, all the filterer defined filters should be cleared, not keeping its state?

I think Filters state inside the Filterer tree should be kept (as an internal bitmap/bitset to allow re-enabling them) but original filters in their original panels should be cleared, like is being done on master branch.

  • When keeping it disabled, the user could not defined new filters?

Not sure if I undertook... I think users should be able to define new filters.

  • Only when reenabling it, the user could use the filterer UI again to define new filters?

I think user should be able to define new filters even if some filters checkboxes are disabled

With the clear filters, the same: They do not disappear because the at least one filterer has a defined filter, even though the filterer is disabled by the checkbox in the filterers tree.

I think "Clear Filters" button should just take in account enabled/applied Filters, like last release.

patrickdalla commented 2 months ago

Filters state, i. e., the defined filters inside a filterer is controlled by the filterer itself, not by the filter manager. I. e. when the filterer is disabled the defined filters still remain in the filterer control, although they are not used to filter the result set.

lfcnassif commented 2 months ago

Not entering into the implementation details, I think the current behavior of this PR on UI is not good from an user perspective, I found it very confusing as an user... Old Filters should be disabled in their original UI component if their checkbox is disabled in the new Filters panel (master is working this way) and the "Clear Filters" button should disable all filters and disappear after clicked (like master again) regardless if previous filters states are kept in the new Filter panel to be re-enabled later.

lfcnassif commented 2 months ago

If current implementation should be totally refactored to implement that, maybe we should leave re-enabling filters from the new Filters panel as a future enhancement and just fix bugs here, but enabling Filters from a Filter Manager looks an important feature to have in a filter manager IMHO...

patrickdalla commented 2 months ago

Hi @lfcnassif . I've made some modifications. Check if is this behaviour you mean.

patrickdalla commented 2 months ago

Now, when disabling a filterer through filters panel, it keeps its internal filter, although they aren't used to filter result set. Also the corresponding TAB of the filterer turns gray.

lfcnassif commented 2 months ago

Thanks @patrickdalla! I won’t have time to test it again today. If you finished here, I would appreciate very much your help to test #2187.

lfcnassif commented 1 month ago

This is still not working properly. Testing commit c277b39:

Please test every UI filter action, every filter combination and every code change carefully, please.

patrickdalla commented 1 month ago

Sorry @lfcnassif , I've made a more detailed review. Please check.

lfcnassif commented 1 month ago

Thanks @patrickdalla!

  1. Select some category, the panel is painted with red color correctly, thanks!
  2. Go to the new Filters tab and disable the category filter there.
  3. The category is left selected in the category tab and if you try to filter by any other category, it doesn't work.

I'm marking this as Draft until it is stable...

patrickdalla commented 1 month ago

I have sent you an audio. Maybe i could not get what you completely want with this feature. The way it is implemented it disables the filterer without clearing its defined filters. So, if you return to the filterer UI and change its defined filters (In case of category filter you select another category) the disabled state is not altered, keeping the filtered disabled as you have disabled it in fliters panel.

That is the way it is implemented. I think that you would like that when returning to filter UI and changing its defined filters, it automatically reenables the filterer, right?

I can do it, just confirm if it is your desired behaviour

Em ter., 28 de mai. de 2024, 14:52, Luis Filipe Nassif < @.***> escreveu:

Thanks @patrickdalla https://github.com/patrickdalla!

  1. Select some category, the panel is painted with red color correctly, thanks!
  2. Go to the new Filters tab and disable the category filter there.
  3. The category is left selected in the category tab and if you try to filter by any other category, it doesn't work.

I'm marking this as Draft until it is stable...

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

lfcnassif commented 1 month ago

Now I got what you implemented, thanks for the explanation.

I think that you would like that when returning to filter UI and changing its defined filters, it automatically reenables the filterer, right?

Yes, please, please. I also think if the filter is disabled in the new Filters tab, the filter state in its original panel should be cleared (and the state copied to the new Filters tab internal data structures to allow reapplying them later). I thought it confusing keeping the category selected but not being applied, honestly. Today filters are applied with one user click and I think that behavior should be kept. Maybe the UI behavior could be changed for 5.0, but for 4.x I think it should be as backwards compatible as possible.

patrickdalla commented 1 month ago

@lfcnassif I've implemented what you meant for category tree filterer (please check if I missunderstood it again). Meanwhile I am implementing similar behaviour for the other filterers.

lfcnassif commented 1 month ago

@lfcnassif I've implemented what you meant for category tree filterer (please check if I missunderstood it again). Meanwhile I am implementing similar behaviour for the other filterers.

Thank you very much @patrickdalla, I just tested the category filter, it's the perfect behavior!

patrickdalla commented 2 weeks ago

Hi @lfcnassif, I've implemented the method to restore old filters from unselected filterers from Filters Panel for all filterers but Graph.

lfcnassif commented 2 weeks ago

Thanks @patrickdalla! @marcus6n, I also would like your help to test this after testing #1341, ok?

marcus6n commented 2 weeks ago

Thanks @patrickdalla! @marcus6n, I also would like your help to test this after testing #1341, ok?

Ok, I'll test it here.

marcus6n commented 1 week ago

@lfcnassif I've run the tests and everything works correctly with the filters, with apparently no errors or bugs.

lfcnassif commented 4 days ago

Hi @patrickdalla. On Monday, could you copy just the fixing commits from this PR to a new one, excluding the re-enable filters new feature?

patrickdalla commented 1 day ago

Sure @lfcnassif.

patrickdalla commented 1 day ago

As asked by @lfcnassif, I copied the fixes to PR #2256 and renamed this to "Individual filterer disable/reenable in filters panel". It includes the latter changes done to support filter state restoration.