comments and minor cleanup

Core/src/org/sleuthkit/autopsy/timeline/filters/CompoundFilter.java

@@ -20,7 +20,6 @@ package org.sleuthkit.autopsy.timeline.filters;
 
 import java.util.List;
 import java.util.Objects;
-import javafx.beans.Observable;
 import javafx.collections.FXCollections;
 import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
@@ -31,12 +30,8 @@ import javafx.collections.ObservableList;
  * implementations can decide how to combine the sub-filters.
  *
  * a {@link CompoundFilter} uses listeners to enforce the following
- * relationships between it and its sub-filters:
+ * relationships between it and its sub-filters: if all of a compound filter's
- * <ol>
+ * sub-filters become un-selected, un-select the compound filter.
- * <le>if a compound filter becomes inactive disable all of its sub-filters</le>
- * <le>if all of a compound filter's sub-filters become un-selected, un-select
- * the compound filter.</le>
- * </ol>
  */
 public abstract class CompoundFilter<SubFilterType extends Filter> extends AbstractFilter {
 
@@ -57,27 +52,23 @@ public abstract class CompoundFilter<SubFilterType extends Filter> extends Abstr
     public CompoundFilter(List<SubFilterType> subFilters) {
         super();
 
-        //listen to changes in list of subfilters and 
+        //listen to changes in list of subfilters 
-        this.subFilters.addListener((ListChangeListener.Change<? extends SubFilterType> c) -> {
+        this.subFilters.addListener((ListChangeListener.Change<? extends SubFilterType> change) -> {
-            while (c.next()) { //add active state listener to newly added filters
+            while (change.next()) {
-                addSubFilterListeners(c.getAddedSubList());
+                //add a listener to the selected property of each added subfilter
+                change.getAddedSubList().forEach(addedSubFilter -> {
+                    //if a subfilter's selected property changes...
+                    addedSubFilter.selectedProperty().addListener(selectedProperty -> {
+                        //set this compound filter selected af any of the subfilters are selected.
+                        setSelected(getSubFilters().parallelStream().anyMatch(Filter::isSelected));
+                    });
+                });
             }
-            setSelected(getSubFilters().parallelStream().anyMatch(Filter::isSelected));
         });
 
         this.subFilters.setAll(subFilters);
     }
 
-    private void addSubFilterListeners(List<? extends SubFilterType> newSubfilters) {
-        for (SubFilterType sf : newSubfilters) {
-            //if a subfilter changes selected state
-            sf.selectedProperty().addListener((Observable observable) -> {
-                //set this filter selected af any of the subfilters are selected.
-                setSelected(getSubFilters().parallelStream().anyMatch(Filter::isSelected));
-            });
-        }
-    }
-
     static <SubFilterType extends Filter> boolean areSubFiltersEqual(final CompoundFilter<SubFilterType> oneFilter, final CompoundFilter<SubFilterType> otherFilter) {
         if (oneFilter.getSubFilters().size() != otherFilter.getSubFilters().size()) {
             return false;

Core/src/org/sleuthkit/autopsy/timeline/filters/DataSourcesFilter.java

@@ -30,21 +30,21 @@ import org.openide.util.NbBundle;
  */
 public class DataSourcesFilter extends UnionFilter<DataSourceFilter> {
 
+    //keep references to the overridden properties so they don't get GC'd
     private final BooleanBinding activePropertyOverride;
-    private BooleanBinding disabledPropertyOverride;
+    private final BooleanBinding disabledPropertyOverride;
 
     public DataSourcesFilter() {
         disabledPropertyOverride = Bindings.or(super.disabledProperty(), Bindings.size(getSubFilters()).lessThanOrEqualTo(1));
-        activePropertyOverride = super.activeProperty().and(Bindings.not(disabledProperty()));
+        activePropertyOverride = super.activeProperty().and(Bindings.not(disabledPropertyOverride));
     }
 
     @Override
     public DataSourcesFilter copyOf() {
         final DataSourcesFilter filterCopy = new DataSourcesFilter();
         //add a copy of each subfilter
-        getSubFilters().forEach(dataSourceFilter ->
+        getSubFilters().forEach(dataSourceFilter -> filterCopy.addSubFilter(dataSourceFilter.copyOf()));
-                filterCopy.addSubFilter(dataSourceFilter.copyOf())
+        //these need to happen after the listeners fired by adding the subfilters 
-        );
         filterCopy.setSelected(isSelected());
         filterCopy.setDisabled(isDisabled());
 

Core/src/org/sleuthkit/autopsy/timeline/filters/HashHitsFilter.java

@@ -43,9 +43,8 @@ public class HashHitsFilter extends UnionFilter<HashSetFilter> {
     public HashHitsFilter copyOf() {
         HashHitsFilter filterCopy = new HashHitsFilter();
         //add a copy of each subfilter
-        this.getSubFilters().forEach(hashSetFilter ->
+        this.getSubFilters().forEach(hashSetFilter -> filterCopy.addSubFilter(hashSetFilter.copyOf()));
-                filterCopy.addSubFilter(hashSetFilter.copyOf())
+        //these need to happen after the listeners fired by adding the subfilters 
-        );
         filterCopy.setSelected(isSelected());
         filterCopy.setDisabled(isDisabled());
 

Core/src/org/sleuthkit/autopsy/timeline/filters/TagsFilter.java

@@ -45,9 +45,8 @@ public class TagsFilter extends UnionFilter<TagNameFilter> {
     public TagsFilter copyOf() {
         TagsFilter filterCopy = new TagsFilter();
         //add a copy of each subfilter
-        getSubFilters().forEach(tagNameFilter ->
+        getSubFilters().forEach(tagNameFilter -> filterCopy.addSubFilter(tagNameFilter.copyOf()));
-                filterCopy.addSubFilter(tagNameFilter.copyOf())
+        //these need to happen after the listeners fired by adding the subfilters 
-        );
         filterCopy.setSelected(isSelected());
         filterCopy.setDisabled(isDisabled());
 

Core/src/org/sleuthkit/autopsy/timeline/filters/TypeFilter.java

@@ -102,10 +102,8 @@ public class TypeFilter extends UnionFilter<TypeFilter> {
         //make a nonrecursive copy of this filter
         final TypeFilter filterCopy = new TypeFilter(eventType, false);
         //add a copy of each subfilter
-        getSubFilters().forEach(typeFilter ->
+        getSubFilters().forEach(typeFilter -> filterCopy.addSubFilter(typeFilter.copyOf(), comparator));
-                filterCopy.addSubFilter(typeFilter.copyOf(), comparator)
+        //these need to happen after the listeners fired by adding the subfilters 
-        );
-
         filterCopy.setSelected(isSelected());
         filterCopy.setDisabled(isDisabled());
         return filterCopy;

Core/src/org/sleuthkit/autopsy/timeline/ui/filtering/FilterSetPanel.java

@@ -84,6 +84,10 @@ final public class FilterSetPanel extends BorderPane {
     private final FilteredEventsModel filteredEvents;
     private final TimeLineController controller;
 
+    /**
+     * map from filter to its expansion state in the ui, used to restore the
+     * expansion state as we navigate back and forward in the history
+     */
     private final ObservableMap<Filter, Boolean> expansionMap = FXCollections.observableHashMap();
     private double dividerPosition;
 
@@ -112,6 +116,7 @@ final public class FilterSetPanel extends BorderPane {
         legendColumn.setCellValueFactory(cellDataFeatures -> cellDataFeatures.getValue().valueProperty());
         legendColumn.setCellFactory(col -> new LegendCell(this.controller));
 
+        //type is the only filter expanded initialy
         expansionMap.put(controller.getEventsModel().getFilter().getTypeFilter(), true);
 
         this.filteredEvents.eventTypeZoomProperty().addListener((Observable observable) -> applyFilters());

Core/src/org/sleuthkit/autopsy/timeline/ui/filtering/FilterTreeItem.java

@@ -31,50 +31,56 @@ import org.sleuthkit.autopsy.timeline.filters.Filter;
 final public class FilterTreeItem extends TreeItem<Filter> {
 
     /**
-     * recursively construct a tree of treeitems to parallel the filter tree of
+     * recursively construct a tree of TreeItems to parallel the filter tree of
      * the given filter
      *
      *
-     * @param f the filter for this item. if f has sub-filters, tree items will
+     * @param filter the filter for this item. if f has sub-filters, tree items
-     *          be made for them added added to the children of this
+     *               will be made for them added added to the children of this
-     *          FilterTreeItem
+     *               FilterTreeItem
      */
-    public FilterTreeItem(Filter f, ObservableMap<Filter, Boolean> expansionMap) {
+    public FilterTreeItem(Filter filter, ObservableMap<Filter, Boolean> expansionMap) {
-        super(f);
+        super(filter);
 
+        //listen to changes in the expansion map, and update expansion state of filter object
         expansionMap.addListener((MapChangeListener.Change<? extends Filter, ? extends Boolean> change) -> {
-            if (change.getKey().equals(f)) {
+            if (change.getKey().equals(filter)) {
                 setExpanded(expansionMap.get(change.getKey()));
             }
         });
 
-        if (expansionMap.containsKey(f)) {
+        if (expansionMap.containsKey(filter)) {
-            setExpanded(expansionMap.get(f));
+            setExpanded(expansionMap.get(filter));
         }
 
-        expandedProperty().addListener(expandedProperty -> expansionMap.put(f, isExpanded()));
+        //keep expanion map upto date if user expands/collapses filter
+        expandedProperty().addListener(expandedProperty -> expansionMap.put(filter, isExpanded()));
 
-        if (f instanceof CompoundFilter<?>) {
+        //if the filter is a compound filter, add its subfilters to the tree
-            final CompoundFilter<?> compoundFilter = (CompoundFilter<?>) f;
+        if (filter instanceof CompoundFilter<?>) {
+            final CompoundFilter<?> compoundFilter = (CompoundFilter<?>) filter;
 
-            for (Filter subFilter : compoundFilter.getSubFilters()) {
+            //add all sub filters
-                getChildren().add(new FilterTreeItem(subFilter, expansionMap));
+            compoundFilter.getSubFilters().forEach(subFilter -> getChildren().add(new FilterTreeItem(subFilter, expansionMap)));
-            }
 
+            //listen to changes in sub filters and keep tree in sync
             compoundFilter.getSubFilters().addListener((ListChangeListener.Change<? extends Filter> c) -> {
                 while (c.next()) {
                     for (Filter subfFilter : c.getAddedSubList()) {
-                        setExpanded(true);
+                        setExpanded(true); //emphasize new filters by expanding parent to make sure they are visible
                         getChildren().add(new FilterTreeItem(subfFilter, expansionMap));
                     }
                 }
             });
 
+            /*
+             * enforce the following relationship between a compound filter and
+             * its subfilters: if a compound filter's active property changes,
+             * disable the subfilters if the compound filter is not active.
+             */
             compoundFilter.activeProperty().addListener(activeProperty -> {
                 compoundFilter.getSubFilters().forEach(subFilter -> subFilter.setDisabled(compoundFilter.isActive() == false));
             });
-
         }
-
     }
 }