Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Individual filterer disable/reenable in filters panel. #2163

Draft
wants to merge 60 commits into
base: master
Choose a base branch
from
Draft

Individual filterer disable/reenable in filters panel. #2163

wants to merge 60 commits into from

Conversation

lfcnassif
Copy link
Member

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

@lfcnassif
Copy link
Member Author

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

@lfcnassif
Copy link
Member Author

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.

@lfcnassif lfcnassif mentioned this pull request Apr 18, 2024
Merged
@patrickdalla
Copy link
Collaborator

patrickdalla commented Apr 18, 2024 •
edited
Loading

Did you create a new Branch with these modifications?

@lfcnassif
Copy link
Member Author

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

@patrickdalla
Copy link
Collaborator

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
Copy link
Member Author

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
'#39 updates lastClickedPath also when clicking for start dragging from
d4a1d7b 
Combined Filterer Tree.
@patrickdalla
'#39 refactors FilterNode classes in a way to keep track of its
945ad13 
correspondent CombinedFilterTreeModel where it is created, so it can
update the map of Filters with corresponding nodes set when removing or
adding one of its children.
@patrickdalla
'#39 returns cachedBitSet map from CombinedFilterer to its previous
897e291 
structure Map<IFilter, FutureBitSetResult>, as the remove of an item in
this cache now only occurs when no other FilterNode refers to the same
filter.
@patrickdalla
'#39 Properly implements clone methods of DecisionNode subclasses, so
e0ee88a 
they can be cloned when dragging with CTRL button pressed (COPY
operation).
@patrickdalla
'#39 Alters field names to be similar with correspondent
4da3cdf 
CombinedFilterer class name.
@patrickdalla
'#39 Allow only on filter node selection at a time in combined filterer
14e403c 
tree.
@patrickdalla
'#39 Implement code to avoid NPE
7e31973
@patrickdalla
'#39 Adds a centered tip message informing about drag and drop filter
df8043d 
operations to combine them.
@patrickdalla
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Member Author

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

Exactly!

@patrickdalla
'#39 Add support for disable Filterer on Filterers tree via its checkbox
d6cd081 
without removing its defined filters, which are restored when checkbox
is checked again.
@patrickdalla
Copy link
Collaborator

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

@lfcnassif
Copy link
Member Author

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

Yes, I think it was already working this way.

@patrickdalla
Copy link
Collaborator

patrickdalla commented Apr 20, 2024 via email •
edited
Loading

@patrickdalla
'#2163 Implements restore filters of MetadataPanel filterer.
c2eea39
@patrickdalla
'#2163 Implements restoreDefinedFilters for evidence tree filterer.
10dba2d
@patrickdalla
'#2163 Corrects NPE on evidence filterer
df5d086
@patrickdalla
'#2163 Removes unnecessary field and corrects getQuery method to return
77d3e11 
a value only if a filter is defined.
@patrickdalla
'#2163 updates internal control field 'selection' on
2686d8a 
restoreDefinedFilters.
@patrickdalla
'#2163 Implements restoreFilters for Bookmarks filterer.
b46dff9
@patrickdalla
'#2163 corrects hasFilters from DuplicatesFilterer
32c22bd
@patrickdalla
'#2163 Code to avoid enabling filterer from filterpanel without any
c324e7e 
defined filter defined by the filterer before.
@patrickdalla
'#2163 corrects similar documents and similar image filter restoration.
5fa2d11
@patrickdalla
'#2163 corrects similar image filterer restoration.
c606d3e
@patrickdalla
'#2163 Implementation of restore filters for Table Header Filterers.
e8b19e3
@patrickdalla
'#2163 forces updateFileListing at the end of setSelectedIndices method.
039a51f
@patrickdalla
'#2163 repositions update file listing outside setSelectedIndices, right
5a6309c 
after its call.
@patrickdalla
'#2163 Force call to setDockableColores after filterer checkbox click.
b77a2f4
@patrickdalla
'#2163 Creates a map to store the correspondence between a query escaped
3434d2d 
name of the field and its original name, that needs to be recovered when
restoring defined filters.
@patrickdalla
'#2163 Implementation of restoreDefinedFilters of Timeline filterer.
c2793dc
@patrickdalla
Copy link
Collaborator

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

@lfcnassif
Copy link
Member Author

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

@marcus6n
Copy link
Contributor

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

Ok, I'll test it here.

@marcus6n
Copy link
Contributor

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

@lfcnassif lfcnassif mentioned this pull request Jun 28, 2024
Closed
@lfcnassif
Copy link
Member Author

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
Copy link
Collaborator

Sure @lfcnassif.

@patrickdalla patrickdalla mentioned this pull request Jul 1, 2024
Open
@patrickdalla patrickdalla changed the title Minor #39 fixes Individual filterer disable/reenable in filters panel. Jul 1, 2024
@patrickdalla
Copy link
Collaborator

patrickdalla commented Jul 1, 2024 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@patrickdalla patrickdalla

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filter Manager
3 participants
@lfcnassif @patrickdalla @marcus6n