From e0e08534fbf6aa6b7d4088bc8bb1f9958460bcac Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 13 May 2024 18:36:43 +0200 Subject: [PATCH 1/5] Fix memory leak when index filter is removed We never unsubscribed the LogModelFilter observer registered by the IndexFilterController. A similar leak happens in BookmarkController, but it never frees itself anyway. Issue: #162 --- .../ui/indexfilter/IndexFilter.java | 9 +++- .../ui/indexfilter/IndexFilterCollection.java | 2 +- .../ui/indexfilter/IndexFilterController.java | 10 +++-- .../ui/indexfilter/IndexFilterTest.java | 42 +++++++++++++++++++ 4 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java index fa219414..a50249d4 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java @@ -26,16 +26,16 @@ import java.awt.Color; import java.util.function.Predicate; -class IndexFilter implements LogModelFilter { +class IndexFilter implements LogModelFilter, AutoCloseable { private final LogModelFilter parent; private final Predicate filter; + private final Observer parentObserver = this::notifyObservers; private final Subject observers = new Subject<>(); public IndexFilter(LogModelFilter parent, Predicate filter) { this.parent = parent; this.filter = filter; - Observer parentObserver = this::notifyObservers; parent.asObservable().addObserver(parentObserver); } @@ -59,4 +59,9 @@ private void notifyObservers() { observer.onModelChange(); } } + + @Override + public void close() { + parent.asObservable().removeObserver(parentObserver); + } } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java index 998b6b14..523270f0 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java @@ -62,7 +62,7 @@ public void addFilter(Filter filter) { @Override public void removeFilter(Filter filter) { if (filter.isEnabled()) { - controllerMap.remove(filter).destroy(); + controllerMap.remove(filter).close(); } } } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java index 0ec62e52..3d8f9693 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java @@ -30,10 +30,11 @@ import javax.inject.Inject; import javax.swing.JTable; -public class IndexFilterController extends AbstractIndexController { +public class IndexFilterController extends AbstractIndexController implements AutoCloseable { private final FilterModel filterModel; private final Filter filter; private final IndexFrame frame; + private final IndexFilter indexLogFilter; IndexFilterController(FilterModel filterModel, MainFrameDependencies dependencies, JTable mainTable, LogModelFilter mainFilter, Filter filter) { @@ -41,10 +42,11 @@ public class IndexFilterController extends AbstractIndexController { this.filterModel = filterModel; this.filter = filter; + indexLogFilter = new IndexFilter(mainFilter, filter); IndexFrameDi.IndexFrameComponent component = DaggerIndexFrameDi_IndexFrameComponent.builder() .mainFrameDependencies(dependencies) .setIndexController(this) - .setIndexFilter(new IndexFilter(mainFilter, filter)) + .setIndexFilter(indexLogFilter) .build(); frame = component.createFrame(); } @@ -56,8 +58,10 @@ public void show() { EventQueue.invokeLater(() -> frame.setVisible(true)); } - public void destroy() { + @Override + public void close() { frame.dispose(); + indexLogFilter.close(); } @Override diff --git a/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java new file mode 100644 index 00000000..63373e63 --- /dev/null +++ b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java @@ -0,0 +1,42 @@ +/* + * Copyright 2024 the Andlogview authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package name.mlopatkin.andlogview.ui.indexfilter; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import name.mlopatkin.andlogview.ui.logtable.LogModelFilter; +import name.mlopatkin.andlogview.utils.events.Subject; + +import org.junit.jupiter.api.Test; + +class IndexFilterTest { + + @Test + void unsubscribesAfterClose() { + var parent = mock(LogModelFilter.class); + var subject = new Subject(); + when(parent.asObservable()).thenReturn(subject.asObservable()); + + try (var ignored = new IndexFilter(parent, r -> true)) { + assertThat(subject).isNotEmpty(); + } + + assertThat(subject).isEmpty(); + } +} From a969dd8c48d5d205fff2bbae170fb5026e6b6565 Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 13 May 2024 18:56:54 +0200 Subject: [PATCH 2/5] Remove dependency on main LogModelFilter in the IndexFilter Issue: #162 --- .../andlogview/filters/FilterChain.java | 28 ++++++++++++++++++- .../andlogview/filters/FilterCollection.java | 7 +++-- .../ui/filters/FilterPanelModelAdapter.java | 3 +- .../ui/indexfilter/IndexFilter.java | 19 ++++++++----- .../ui/indexfilter/IndexFilterCollection.java | 3 +- .../ui/indexfilter/IndexFilterController.java | 7 ++--- .../ui/indexfilter/IndexFilterTest.java | 6 ++-- 7 files changed, 53 insertions(+), 20 deletions(-) diff --git a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterChain.java b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterChain.java index c0bd2172..9329e994 100644 --- a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterChain.java +++ b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterChain.java @@ -16,6 +16,8 @@ package name.mlopatkin.andlogview.filters; import name.mlopatkin.andlogview.logmodel.LogRecord; +import name.mlopatkin.andlogview.utils.events.Observable; +import name.mlopatkin.andlogview.utils.events.Subject; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.SetMultimap; @@ -30,8 +32,20 @@ * The order in which filters are added to/removed from FilterChain doesn't matter. */ public class FilterChain implements FilterCollection { + /** + * An observer to be notified when the set of filters in this chain changes. + */ + @FunctionalInterface + public interface Observer { + /** + * Called when the set of filters in the {@link FilterChain} changes + */ + void onFiltersChanged(); + } + private final SetMultimap filters = MultimapBuilder.enumKeys(FilteringMode.class).hashSetValues().build(); + private final Subject observers = new Subject<>(); private boolean include(FilteringMode mode, LogRecord record) { var filtersForMode = filters.get(mode); @@ -59,6 +73,7 @@ private boolean include(FilteringMode mode, LogRecord record) { public void addFilter(Filter filter) { if (filter.isEnabled()) { filters.put(filter.getMode(), filter); + notifyObservers(); } } @@ -68,7 +83,18 @@ public boolean shouldShow(LogRecord record) { @Override public void removeFilter(Filter filter) { - filters.remove(filter.getMode(), filter); + if (filters.remove(filter.getMode(), filter)) { + notifyObservers(); + } } + private void notifyObservers() { + for (Observer observer : observers) { + observer.onFiltersChanged(); + } + } + + public Observable asObservable() { + return observers.asObservable(); + } } diff --git a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterCollection.java b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterCollection.java index a0bd4b94..49b37740 100644 --- a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterCollection.java +++ b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterCollection.java @@ -16,6 +16,8 @@ package name.mlopatkin.andlogview.filters; +import name.mlopatkin.andlogview.utils.events.ScopedObserver; + import org.checkerframework.checker.nullness.qual.Nullable; /** @@ -74,7 +76,7 @@ default void replaceFilter(T oldFilter, T newFilter) { * @param model the model to connect this collection to * @return the observer that is subscribed to the provided model. It can be used to unsubscribe. */ - default FilterModel.Observer setModel(FilterModel model) { + default ScopedObserver setModel(FilterModel model) { for (var filter : model.getFilters()) { var transformed = transformFilter(filter); if (transformed != null) { @@ -99,7 +101,6 @@ protected void onMyFilterReplaced(FilterModel model, T oldFilter, T newFilter) { } }; - model.asObservable().addObserver(observer); - return observer; + return model.asObservable().addScopedObserver(observer); } } diff --git a/src/name/mlopatkin/andlogview/ui/filters/FilterPanelModelAdapter.java b/src/name/mlopatkin/andlogview/ui/filters/FilterPanelModelAdapter.java index e4d72cce..8e7a2237 100644 --- a/src/name/mlopatkin/andlogview/ui/filters/FilterPanelModelAdapter.java +++ b/src/name/mlopatkin/andlogview/ui/filters/FilterPanelModelAdapter.java @@ -24,6 +24,7 @@ import name.mlopatkin.andlogview.ui.filterdialog.FilterFromDialog; import name.mlopatkin.andlogview.ui.filterpanel.FilterPanel; import name.mlopatkin.andlogview.ui.filterpanel.FilterPanelModel; +import name.mlopatkin.andlogview.utils.events.ScopedObserver; import name.mlopatkin.andlogview.utils.events.Subject; import com.google.common.collect.ImmutableList; @@ -63,7 +64,7 @@ class FilterPanelModelAdapter implements FilterCollection, FilterPa @Inject @Override - public FilterModel.Observer setModel(FilterModel model) { + public ScopedObserver setModel(FilterModel model) { this.model = model; return FilterCollection.super.setModel(model); } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java index a50249d4..675e2aa2 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java @@ -16,9 +16,12 @@ package name.mlopatkin.andlogview.ui.indexfilter; +import name.mlopatkin.andlogview.filters.FilterChain; +import name.mlopatkin.andlogview.filters.FilterModel; import name.mlopatkin.andlogview.logmodel.LogRecord; import name.mlopatkin.andlogview.ui.logtable.LogModelFilter; import name.mlopatkin.andlogview.utils.events.Observable; +import name.mlopatkin.andlogview.utils.events.ScopedObserver; import name.mlopatkin.andlogview.utils.events.Subject; import org.checkerframework.checker.nullness.qual.Nullable; @@ -27,21 +30,23 @@ import java.util.function.Predicate; class IndexFilter implements LogModelFilter, AutoCloseable { - private final LogModelFilter parent; + private final FilterChain parent; private final Predicate filter; - private final Observer parentObserver = this::notifyObservers; + private final ScopedObserver parentObserver; private final Subject observers = new Subject<>(); - public IndexFilter(LogModelFilter parent, Predicate filter) { - this.parent = parent; + public IndexFilter(FilterModel parentFilters, Predicate filter) { + this.parent = new FilterChain(); + this.parentObserver = this.parent.setModel(parentFilters); + this.parent.asObservable().addObserver(this::notifyObservers); + this.filter = filter; - parent.asObservable().addObserver(parentObserver); } @Override public boolean shouldShowRecord(LogRecord record) { - return parent.shouldShowRecord(record) && filter.test(record); + return parent.shouldShow(record) && filter.test(record); } @Override @@ -62,6 +67,6 @@ private void notifyObservers() { @Override public void close() { - parent.asObservable().removeObserver(parentObserver); + parentObserver.close(); } } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java index 523270f0..2f69accb 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java @@ -20,6 +20,7 @@ import name.mlopatkin.andlogview.filters.FilterCollection; import name.mlopatkin.andlogview.filters.FilterModel; import name.mlopatkin.andlogview.filters.FilteringMode; +import name.mlopatkin.andlogview.utils.events.ScopedObserver; import org.checkerframework.checker.nullness.qual.Nullable; @@ -39,7 +40,7 @@ public IndexFilterCollection(IndexFilterController.Factory controllerFactory) { @Override @Inject - public FilterModel.Observer setModel(FilterModel model) { + public ScopedObserver setModel(FilterModel model) { // This is a late call to subscribe to model changes. return FilterCollection.super.setModel(model); } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java index 3d8f9693..48917841 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java @@ -22,7 +22,6 @@ import name.mlopatkin.andlogview.ui.indexframe.DaggerIndexFrameDi_IndexFrameComponent; import name.mlopatkin.andlogview.ui.indexframe.IndexFrame; import name.mlopatkin.andlogview.ui.indexframe.IndexFrameDi; -import name.mlopatkin.andlogview.ui.logtable.LogModelFilter; import name.mlopatkin.andlogview.ui.mainframe.MainFrameDependencies; import java.awt.EventQueue; @@ -37,12 +36,12 @@ public class IndexFilterController extends AbstractIndexController implements Au private final IndexFilter indexLogFilter; IndexFilterController(FilterModel filterModel, MainFrameDependencies dependencies, JTable mainTable, - LogModelFilter mainFilter, Filter filter) { + Filter filter) { super(mainTable); this.filterModel = filterModel; this.filter = filter; - indexLogFilter = new IndexFilter(mainFilter, filter); + indexLogFilter = new IndexFilter(filterModel, filter); IndexFrameDi.IndexFrameComponent component = DaggerIndexFrameDi_IndexFrameComponent.builder() .mainFrameDependencies(dependencies) .setIndexController(this) @@ -85,7 +84,7 @@ public IndexFilterController create(IndexFilterCollection owner, Filter filter) // We probably can break this by factoring Filter out of MainFilterController and making both Factory and // MainFilterController to depend on this new Filter return new IndexFilterController( - filterModel, dependencies, dependencies.getLogTable(), dependencies.getFilter(), filter); + filterModel, dependencies, dependencies.getLogTable(), filter); } } } diff --git a/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java index 63373e63..7bd487b9 100644 --- a/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java +++ b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java @@ -20,7 +20,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import name.mlopatkin.andlogview.ui.logtable.LogModelFilter; +import name.mlopatkin.andlogview.filters.FilterModel; import name.mlopatkin.andlogview.utils.events.Subject; import org.junit.jupiter.api.Test; @@ -29,8 +29,8 @@ class IndexFilterTest { @Test void unsubscribesAfterClose() { - var parent = mock(LogModelFilter.class); - var subject = new Subject(); + FilterModel parent = mock(); + var subject = new Subject(); when(parent.asObservable()).thenReturn(subject.asObservable()); try (var ignored = new IndexFilter(parent, r -> true)) { From 79300c5bd0cd0d577dd836a3f91126c0fd75d983 Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 13 May 2024 19:19:46 +0200 Subject: [PATCH 3/5] Simplify dependencies of IndexFilterController classes Issue: #162 --- .../ui/bookmarks/BookmarkController.java | 23 ++++++---- .../andlogview/ui/bookmarks/BookmarksDi.java | 6 ++- .../ui/indexfilter/IndexFilterCollection.java | 2 +- .../ui/indexfilter/IndexFilterController.java | 44 +++++++++---------- .../ui/indexframe/IndexFrameDi.java | 10 +++-- 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarkController.java b/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarkController.java index 549f0433..1a4d9291 100644 --- a/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarkController.java +++ b/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarkController.java @@ -19,6 +19,8 @@ import name.mlopatkin.andlogview.ui.indexframe.AbstractIndexController; import name.mlopatkin.andlogview.ui.indexframe.IndexController; import name.mlopatkin.andlogview.ui.indexframe.IndexFrame; +import name.mlopatkin.andlogview.ui.logtable.LogRecordTableModel; +import name.mlopatkin.andlogview.ui.mainframe.DialogFactory; import name.mlopatkin.andlogview.ui.mainframe.MainFrameDependencies; import name.mlopatkin.andlogview.ui.mainframe.MainFrameScoped; import name.mlopatkin.andlogview.widgets.UiHelper; @@ -36,8 +38,12 @@ public class BookmarkController extends AbstractIndexController implements Index private final IndexFrame indexFrame; @Inject - public BookmarkController(MainFrameDependencies mainFrameDependencies, BookmarkModel bookmarksModel, - BookmarksLogModelFilter logModelFilter, @Named(MainFrameDependencies.FOR_MAIN_FRAME) JTable mainLogTable) { + public BookmarkController( + BookmarkModel bookmarksModel, + LogRecordTableModel logModel, + BookmarksLogModelFilter logModelFilter, + @Named(MainFrameDependencies.FOR_MAIN_FRAME) JTable mainLogTable, + DialogFactory dialogFactory) { super(mainLogTable); this.mainLogTable = mainLogTable; @@ -57,12 +63,13 @@ public void onBookmarkRemoved() { }; bookmarksModel.asObservable().addObserver(bookmarkChangeObserver); - BookmarksDi.BookmarksFrameComponent.Builder builder = DaggerBookmarksDi_BookmarksFrameComponent.builder(); - builder.mainFrameDependencies(mainFrameDependencies); - builder.setIndexController(this); - builder.setIndexFilter(logModelFilter); - - BookmarksDi.BookmarksFrameComponent indexFrameComponent = builder.build(); + var builder = DaggerBookmarksDi_BookmarksFrameComponent.builder() + .bookmarkModel(bookmarksModel) + .logRecordTableModel(logModel) + .dialogFactory(dialogFactory) + .setIndexController(this) + .setIndexFilter(logModelFilter); + var indexFrameComponent = (BookmarksDi.BookmarksFrameComponent) builder.build(); indexFrame = indexFrameComponent.createFrame(); indexFrame.setTitle("Bookmarks"); diff --git a/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarksDi.java b/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarksDi.java index 68ccd473..b1238b34 100644 --- a/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarksDi.java +++ b/src/name/mlopatkin/andlogview/ui/bookmarks/BookmarksDi.java @@ -28,7 +28,6 @@ import name.mlopatkin.andlogview.ui.logtable.LogTableScoped; import name.mlopatkin.andlogview.ui.logtable.PopupMenu; import name.mlopatkin.andlogview.ui.logtable.TableRow; -import name.mlopatkin.andlogview.ui.mainframe.MainFrameDependencies; import dagger.BindsInstance; import dagger.Component; @@ -43,11 +42,14 @@ final class BookmarksDi { @IndexFrameScoped - @Component(dependencies = MainFrameDependencies.class, + @Component( modules = {IndexFrameDi.TableColumnsModule.class, TableModule.class}) interface BookmarksFrameComponent extends IndexFrameDi.IndexFrameComponent { @Component.Builder interface Builder extends IndexFrameDi.IndexFrameComponent.Builder { + @BindsInstance + Builder bookmarkModel(BookmarkModel bookmarkModel); + @Override @SuppressWarnings("ClassEscapesDefinedScope") BookmarksFrameComponent build(); diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java index 2f69accb..5fa4a884 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterCollection.java @@ -55,7 +55,7 @@ public void addFilter(Filter filter) { if (!filter.isEnabled()) { return; } - var controller = controllerFactory.create(this, filter); + var controller = controllerFactory.create(filter); controllerMap.put(filter, controller); controller.show(); } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java index 48917841..3bb93771 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java @@ -16,17 +16,24 @@ package name.mlopatkin.andlogview.ui.indexfilter; +import static name.mlopatkin.andlogview.ui.mainframe.MainFrameDependencies.FOR_MAIN_FRAME; + import name.mlopatkin.andlogview.filters.Filter; import name.mlopatkin.andlogview.filters.FilterModel; import name.mlopatkin.andlogview.ui.indexframe.AbstractIndexController; import name.mlopatkin.andlogview.ui.indexframe.DaggerIndexFrameDi_IndexFrameComponent; import name.mlopatkin.andlogview.ui.indexframe.IndexFrame; import name.mlopatkin.andlogview.ui.indexframe.IndexFrameDi; -import name.mlopatkin.andlogview.ui.mainframe.MainFrameDependencies; +import name.mlopatkin.andlogview.ui.logtable.LogRecordTableModel; +import name.mlopatkin.andlogview.ui.mainframe.DialogFactory; + +import dagger.assisted.Assisted; +import dagger.assisted.AssistedFactory; +import dagger.assisted.AssistedInject; import java.awt.EventQueue; -import javax.inject.Inject; +import javax.inject.Named; import javax.swing.JTable; public class IndexFilterController extends AbstractIndexController implements AutoCloseable { @@ -35,15 +42,21 @@ public class IndexFilterController extends AbstractIndexController implements Au private final IndexFrame frame; private final IndexFilter indexLogFilter; - IndexFilterController(FilterModel filterModel, MainFrameDependencies dependencies, JTable mainTable, - Filter filter) { + @AssistedInject + IndexFilterController( + LogRecordTableModel logModel, + DialogFactory dialogFactory, + FilterModel filterModel, + @Named(FOR_MAIN_FRAME) JTable mainTable, + @Assisted Filter filter) { super(mainTable); this.filterModel = filterModel; this.filter = filter; indexLogFilter = new IndexFilter(filterModel, filter); IndexFrameDi.IndexFrameComponent component = DaggerIndexFrameDi_IndexFrameComponent.builder() - .mainFrameDependencies(dependencies) + .logRecordTableModel(logModel) + .dialogFactory(dialogFactory) .setIndexController(this) .setIndexFilter(indexLogFilter) .build(); @@ -68,23 +81,8 @@ public void onWindowClosed() { filterModel.replaceFilter(filter, filter.disabled()); } - public static class Factory { - private final MainFrameDependencies dependencies; - private final FilterModel filterModel; - - @Inject - public Factory(MainFrameDependencies dependencies, FilterModel filterModel) { - this.dependencies = dependencies; - this.filterModel = filterModel; - } - - public IndexFilterController create(IndexFilterCollection owner, Filter filter) { - // TODO dependency cycle, dangerous - // MainFilterController -> IndexFilterCollection -> Factory -> MainFrameDependencies -> MainFilterController - // We probably can break this by factoring Filter out of MainFilterController and making both Factory and - // MainFilterController to depend on this new Filter - return new IndexFilterController( - filterModel, dependencies, dependencies.getLogTable(), filter); - } + @AssistedFactory + public interface Factory { + IndexFilterController create(Filter filter); } } diff --git a/src/name/mlopatkin/andlogview/ui/indexframe/IndexFrameDi.java b/src/name/mlopatkin/andlogview/ui/indexframe/IndexFrameDi.java index 31a62c3e..8815b706 100644 --- a/src/name/mlopatkin/andlogview/ui/indexframe/IndexFrameDi.java +++ b/src/name/mlopatkin/andlogview/ui/indexframe/IndexFrameDi.java @@ -27,7 +27,7 @@ import name.mlopatkin.andlogview.ui.logtable.PopupMenuPresenter; import name.mlopatkin.andlogview.ui.logtable.PopupMenuViewImpl; import name.mlopatkin.andlogview.ui.logtable.TableRow; -import name.mlopatkin.andlogview.ui.mainframe.MainFrameDependencies; +import name.mlopatkin.andlogview.ui.mainframe.DialogFactory; import com.google.common.collect.ImmutableList; @@ -48,14 +48,18 @@ public final class IndexFrameDi { private IndexFrameDi() {} - @Component(dependencies = MainFrameDependencies.class, modules = {TableColumnsModule.class, TableModule.class}) + @Component(modules = {TableColumnsModule.class, TableModule.class}) @IndexFrameScoped public interface IndexFrameComponent { IndexFrame createFrame(); @Component.Builder interface Builder { - Builder mainFrameDependencies(MainFrameDependencies deps); + @BindsInstance + Builder dialogFactory(DialogFactory dialogFactory); + + @BindsInstance + Builder logRecordTableModel(LogRecordTableModel tableModel); @BindsInstance Builder setIndexController(IndexController indexController); From 1ab76316bac6fa8d67371a5024fcd940a686c7ea Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 13 May 2024 20:52:43 +0200 Subject: [PATCH 4/5] Build the full FilterModel for the index window Issue: #162 --- filters/build.gradle.kts | 6 +- .../andlogview/filters/FilterModelImpl.java | 4 +- .../andlogview/filters/FilterModelTest.java | 10 +-- .../andlogview/filters/FilterModelAssert.java | 50 +++++++++++ .../andlogview/filters/TestFilterModel.java | 26 ++++++ .../ui/indexfilter/IndexFilter.java | 25 ++---- .../ui/indexfilter/IndexFilterController.java | 10 +-- .../ui/indexfilter/IndexFilterModel.java | 87 +++++++++++++++++++ .../ui/indexfilter/IndexFilterModelTest.java | 58 +++++++++++++ .../ui/indexfilter/IndexFilterTest.java | 42 --------- 10 files changed, 242 insertions(+), 76 deletions(-) create mode 100644 filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/FilterModelAssert.java create mode 100644 filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/TestFilterModel.java create mode 100644 src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModel.java create mode 100644 test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModelTest.java delete mode 100644 test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java diff --git a/filters/build.gradle.kts b/filters/build.gradle.kts index fc3f997c..0a381d7d 100644 --- a/filters/build.gradle.kts +++ b/filters/build.gradle.kts @@ -24,6 +24,10 @@ dependencies { api(project(":logmodel")) { because("Exposes LogRecord as a generic argument") } + api(project(":base")) { + because("Exposes Observable in API") + } - implementation(project(":base")) + testFixturesApi(platform(libs.test.assertj.bom)) + testFixturesApi(libs.test.assertj.core) } diff --git a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java index dd5505c4..105467bb 100644 --- a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java +++ b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java @@ -19,6 +19,7 @@ import name.mlopatkin.andlogview.utils.events.Observable; import name.mlopatkin.andlogview.utils.events.Subject; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -30,7 +31,8 @@ import java.util.Objects; class FilterModelImpl implements FilterModel { - private final Subject observers = new Subject<>(); + @VisibleForTesting + protected final Subject observers = new Subject<>(); private final Map filterPositions = new HashMap<>(); private final List filters = new ArrayList<>(); diff --git a/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java b/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java index a1263dff..27b31b12 100644 --- a/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java +++ b/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java @@ -16,22 +16,21 @@ package name.mlopatkin.andlogview.filters; +import static name.mlopatkin.andlogview.filters.FilterModelAssert.assertThatFilters; + import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; -import org.assertj.core.api.AbstractCollectionAssert; import org.assertj.core.api.ListAssert; -import org.assertj.core.api.ObjectAssert; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import java.util.ArrayList; -import java.util.Collection; import java.util.List; @ExtendWith(MockitoExtension.class) @@ -66,11 +65,6 @@ private ListAssert assertThatObserverSawFilters() { return assertThat(observedModel); } - private AbstractCollectionAssert, Filter, ObjectAssert> assertThatFilters( - FilterModel model) { - return assertThat(model.getFilters()); - } - @Test void addingFilterNotifiesObservers() { var model = createModel(); diff --git a/filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/FilterModelAssert.java b/filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/FilterModelAssert.java new file mode 100644 index 00000000..ffac9b68 --- /dev/null +++ b/filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/FilterModelAssert.java @@ -0,0 +1,50 @@ +/* + * Copyright 2024 the Andlogview authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package name.mlopatkin.andlogview.filters; + +import org.assertj.core.api.AbstractAssert; +import org.assertj.core.api.AbstractCollectionAssert; +import org.assertj.core.api.Assertions; +import org.assertj.core.api.ObjectAssert; +import org.assertj.core.util.CheckReturnValue; + +import java.util.Collection; + +public class FilterModelAssert extends AbstractAssert { + protected FilterModelAssert(TestFilterModel testFilterModel) { + super(testFilterModel, FilterModelAssert.class); + } + + public FilterModelAssert hasNoObservers() { + if (actual.hasObservers()) { + throw failure("still has observers"); + } + return this; + } + + @CheckReturnValue + public static FilterModelAssert assertThat(TestFilterModel filterModel) { + return new FilterModelAssert(filterModel); + } + + @CheckReturnValue + public static AbstractCollectionAssert< + ?, Collection, Filter, ObjectAssert + > assertThatFilters(FilterModel model) { + return Assertions.assertThat(model.getFilters()); + } +} diff --git a/filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/TestFilterModel.java b/filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/TestFilterModel.java new file mode 100644 index 00000000..1e567166 --- /dev/null +++ b/filters/src/testFixtures/java/name/mlopatkin/andlogview/filters/TestFilterModel.java @@ -0,0 +1,26 @@ +/* + * Copyright 2024 the Andlogview authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package name.mlopatkin.andlogview.filters; + +/** + * Test implementation of {@link FilterModel} to verify that all observers are unsubscribed. + */ +public class TestFilterModel extends FilterModelImpl { + public boolean hasObservers() { + return !observers.isEmpty(); + } +} diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java index 675e2aa2..442d8069 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilter.java @@ -21,32 +21,26 @@ import name.mlopatkin.andlogview.logmodel.LogRecord; import name.mlopatkin.andlogview.ui.logtable.LogModelFilter; import name.mlopatkin.andlogview.utils.events.Observable; -import name.mlopatkin.andlogview.utils.events.ScopedObserver; import name.mlopatkin.andlogview.utils.events.Subject; import org.checkerframework.checker.nullness.qual.Nullable; import java.awt.Color; -import java.util.function.Predicate; -class IndexFilter implements LogModelFilter, AutoCloseable { - private final FilterChain parent; - private final Predicate filter; - private final ScopedObserver parentObserver; +class IndexFilter implements LogModelFilter { + private final FilterChain filters; private final Subject observers = new Subject<>(); - public IndexFilter(FilterModel parentFilters, Predicate filter) { - this.parent = new FilterChain(); - this.parentObserver = this.parent.setModel(parentFilters); - this.parent.asObservable().addObserver(this::notifyObservers); - - this.filter = filter; + public IndexFilter(FilterModel filters) { + this.filters = new FilterChain(); + this.filters.setModel(filters); + this.filters.asObservable().addObserver(this::notifyObservers); } @Override public boolean shouldShowRecord(LogRecord record) { - return parent.shouldShow(record) && filter.test(record); + return filters.shouldShow(record); } @Override @@ -64,9 +58,4 @@ private void notifyObservers() { observer.onModelChange(); } } - - @Override - public void close() { - parentObserver.close(); - } } diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java index 3bb93771..4583e06c 100644 --- a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterController.java @@ -40,25 +40,24 @@ public class IndexFilterController extends AbstractIndexController implements Au private final FilterModel filterModel; private final Filter filter; private final IndexFrame frame; - private final IndexFilter indexLogFilter; @AssistedInject IndexFilterController( LogRecordTableModel logModel, DialogFactory dialogFactory, - FilterModel filterModel, + FilterModel parentFilterModel, @Named(FOR_MAIN_FRAME) JTable mainTable, @Assisted Filter filter) { super(mainTable); - this.filterModel = filterModel; + this.filterModel = parentFilterModel; this.filter = filter; - indexLogFilter = new IndexFilter(filterModel, filter); + IndexFrameDi.IndexFrameComponent component = DaggerIndexFrameDi_IndexFrameComponent.builder() .logRecordTableModel(logModel) .dialogFactory(dialogFactory) .setIndexController(this) - .setIndexFilter(indexLogFilter) + .setIndexFilter(new IndexFilter(IndexFilterModel.createIndexFilterModel(parentFilterModel, filter))) .build(); frame = component.createFrame(); } @@ -73,7 +72,6 @@ public void show() { @Override public void close() { frame.dispose(); - indexLogFilter.close(); } @Override diff --git a/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModel.java b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModel.java new file mode 100644 index 00000000..2c528e00 --- /dev/null +++ b/src/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModel.java @@ -0,0 +1,87 @@ +/* + * Copyright 2024 the Andlogview authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package name.mlopatkin.andlogview.ui.indexfilter; + +import name.mlopatkin.andlogview.filters.Filter; +import name.mlopatkin.andlogview.filters.FilterModel; +import name.mlopatkin.andlogview.filters.FilteringMode; +import name.mlopatkin.andlogview.logmodel.LogRecord; + +import java.util.function.Predicate; + +/** + * Utility class to build child FilterModel used by the index window. + */ +final class IndexFilterModel { + private IndexFilterModel() {} + + public static FilterModel createIndexFilterModel(FilterModel parentModel, Predicate filter) { + var combinedModel = FilterModel.create(); + parentModel.getFilters().forEach(combinedModel::addFilter); + parentModel.asObservable().addObserver(new FilterModel.Observer() { + @Override + public void onFilterAdded(FilterModel model, Filter newFilter) { + combinedModel.addFilter(newFilter); + } + + @Override + public void onFilterRemoved(FilterModel model, Filter removedFilter) { + // TODO(mlopatkin): this self-caused unsubscribe is not ideal, but it works for now + if (removedFilter == filter) { + model.asObservable().removeObserver(this); + } + combinedModel.removeFilter(removedFilter); + } + + @Override + public void onFilterReplaced(FilterModel model, Filter oldFilter, Filter newFilter) { + if (oldFilter == filter) { + model.asObservable().removeObserver(this); + } + combinedModel.replaceFilter(oldFilter, newFilter); + } + }); + + combinedModel.addFilter(new Filter() { + @Override + public FilteringMode getMode() { + return FilteringMode.HIDE; + } + + @Override + public boolean isEnabled() { + return true; + } + + @Override + public Filter enabled() { + return this; + } + + @Override + public Filter disabled() { + throw new UnsupportedOperationException("Cannot disable filter through child model"); + } + + @Override + public boolean test(LogRecord logRecord) { + return !filter.test(logRecord); + } + }); + return combinedModel; + } +} diff --git a/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModelTest.java b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModelTest.java new file mode 100644 index 00000000..23541f69 --- /dev/null +++ b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterModelTest.java @@ -0,0 +1,58 @@ +/* + * Copyright 2024 the Andlogview authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package name.mlopatkin.andlogview.ui.indexfilter; + +import static name.mlopatkin.andlogview.filters.FilterModelAssert.assertThat; +import static name.mlopatkin.andlogview.filters.FilterModelAssert.assertThatFilters; + +import name.mlopatkin.andlogview.filters.Filter; +import name.mlopatkin.andlogview.filters.FilteringMode; +import name.mlopatkin.andlogview.filters.TestFilterModel; +import name.mlopatkin.andlogview.filters.ToggleFilter; + +import org.junit.jupiter.api.Test; + +class IndexFilterModelTest { + + @Test + void indexModelAddsOneFilter() { + var parent = new TestFilterModel(); + var indexFilter = createFilter(); + parent.addFilter(indexFilter); + + var indexFilterModel = IndexFilterModel.createIndexFilterModel(parent, indexFilter); + + assertThatFilters(indexFilterModel).hasSize(2); + } + + @Test + void indexModelUnsubscribesItselfFromParentWhenFilterRemoved() { + var parent = new TestFilterModel(); + var indexFilter = createFilter(); + parent.addFilter(indexFilter); + + var indexFilterModel = IndexFilterModel.createIndexFilterModel(parent, indexFilter); + parent.removeFilter(indexFilter); + + assertThatFilters(indexFilterModel).hasSize(1); + assertThat(parent).hasNoObservers(); + } + + private Filter createFilter() { + return new ToggleFilter(FilteringMode.WINDOW, true, r -> true); + } +} diff --git a/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java b/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java deleted file mode 100644 index 7bd487b9..00000000 --- a/test/name/mlopatkin/andlogview/ui/indexfilter/IndexFilterTest.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright 2024 the Andlogview authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package name.mlopatkin.andlogview.ui.indexfilter; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import name.mlopatkin.andlogview.filters.FilterModel; -import name.mlopatkin.andlogview.utils.events.Subject; - -import org.junit.jupiter.api.Test; - -class IndexFilterTest { - - @Test - void unsubscribesAfterClose() { - FilterModel parent = mock(); - var subject = new Subject(); - when(parent.asObservable()).thenReturn(subject.asObservable()); - - try (var ignored = new IndexFilter(parent, r -> true)) { - assertThat(subject).isNotEmpty(); - } - - assertThat(subject).isEmpty(); - } -} From 06aab7aebac2dceac4957e1cf9636ac1a9360518 Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 13 May 2024 22:32:46 +0200 Subject: [PATCH 5/5] Do not use tricky position calculation in FilterModelImpl This doesn't work because we don't update other filters when removing. Fixing this makes removal linear, though it is already because the array list has to be compacted. It is unlikely that we're going to have tens of thousands of filters, so having a linear complexity of any filter operation is fine. The only exception is the initialization, where adding the filters one by one may exhibit quadratic behavior. To work around that the loading code is now rewritten. Issue: #374 --- .../andlogview/filters/FilterModel.java | 9 +++++++ .../andlogview/filters/FilterModelImpl.java | 25 ++++++++--------- .../andlogview/filters/FilterModelTest.java | 27 +++++++++++++++++++ .../andlogview/ui/filters/FilterModule.java | 6 +---- .../andlogview/ui/filters/StoredFilters.java | 25 ++++++++++++----- 5 files changed, 68 insertions(+), 24 deletions(-) diff --git a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModel.java b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModel.java index f0ee78ab..05b5e46b 100644 --- a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModel.java +++ b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModel.java @@ -101,4 +101,13 @@ interface Observer { static FilterModel create() { return new FilterModelImpl(); } + + /** + * Creates a new FilterModel with provided filters as its content. + * + * @return the new FilterModel + */ + static FilterModel create(Collection filters) { + return new FilterModelImpl(filters); + } } diff --git a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java index 105467bb..d5fcc5e1 100644 --- a/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java +++ b/filters/src/main/java/name/mlopatkin/andlogview/filters/FilterModelImpl.java @@ -22,24 +22,28 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Objects; class FilterModelImpl implements FilterModel { @VisibleForTesting protected final Subject observers = new Subject<>(); - private final Map filterPositions = new HashMap<>(); + private final List filters = new ArrayList<>(); + public FilterModelImpl() {} + + public FilterModelImpl(Collection filters) { + this.filters.addAll(ImmutableSet.copyOf(filters)); + } + @Override public void addFilter(Filter filter) { - var prevPosition = filterPositions.putIfAbsent(filter, filters.size()); - if (prevPosition == null) { + if (!filters.contains(filter)) { filters.add(filter); for (var observer : observers) { observer.onFilterAdded(this, filter); @@ -49,9 +53,7 @@ public void addFilter(Filter filter) { @Override public void removeFilter(Filter filter) { - var position = filterPositions.remove(filter); - if (position != null) { - filters.remove(position.intValue()); + if (filters.remove(filter)) { for (var observer : observers) { observer.onFilterRemoved(this, filter); } @@ -60,17 +62,16 @@ public void removeFilter(Filter filter) { @Override public void replaceFilter(Filter toReplace, Filter newFilter) { - Preconditions.checkArgument(filterPositions.containsKey(toReplace), + var position = filters.indexOf(toReplace); + Preconditions.checkArgument(position >= 0, String.format("Filter %s is not in the model", toReplace)); if (Objects.equals(toReplace, newFilter)) { // Replacing the filter with itself, do nothing. return; } - var position = filterPositions.get(toReplace); - if (filterPositions.putIfAbsent(newFilter, position) != null) { + if (filters.contains(newFilter)) { throw new IllegalArgumentException(String.format("Filter %s is already in the model", newFilter)); } - filterPositions.remove(toReplace); filters.set(position, newFilter); for (var observer : observers) { diff --git a/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java b/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java index 27b31b12..e9060cc5 100644 --- a/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java +++ b/filters/src/test/java/name/mlopatkin/andlogview/filters/FilterModelTest.java @@ -24,6 +24,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; +import com.google.common.collect.ImmutableList; + import org.assertj.core.api.ListAssert; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -189,6 +191,21 @@ void replacementFilterKeepsPositionOfReplaced() { assertThatFilters(model).containsExactly(filter1, filter2Replacement, filter3); } + @Test + void canReplaceFilterAfterRemovingFilterInFront() { + var model = createModel(); + var filter1 = createFilter("filter1"); + var filter2 = createFilter("filter2"); + model.addFilter(filter1); + model.addFilter(filter2); + + var replacement = createFilter("replacement"); + model.removeFilter(filter1); + model.replaceFilter(filter2, replacement); + + assertThatFilters(model).containsExactly(replacement); + } + @Test void removingAndAddingFilterMovesItToBack() { var model = createModel(); @@ -203,6 +220,16 @@ void removingAndAddingFilterMovesItToBack() { assertThatFilters(model).containsExactly(filter2, filter1); } + @Test + void canAddMultipleFiltersAtOnce() { + var filter1 = createFilter("filter1"); + var filter2 = createFilter("filter2"); + + var model = new FilterModelImpl(ImmutableList.of(filter1, filter2, filter1)); + + assertThatFilters(model).containsExactly(filter1, filter2); + } + FilterModel createModel() { return new FilterModelImpl(); } diff --git a/src/name/mlopatkin/andlogview/ui/filters/FilterModule.java b/src/name/mlopatkin/andlogview/ui/filters/FilterModule.java index 6ddfe05a..703976ff 100644 --- a/src/name/mlopatkin/andlogview/ui/filters/FilterModule.java +++ b/src/name/mlopatkin/andlogview/ui/filters/FilterModule.java @@ -39,11 +39,7 @@ public abstract class FilterModule { @Provides @MainFrameScoped static FilterModel getFiltersModel(StoredFilters filters) { - var filterModel = FilterModel.create(); - - filters.setModel(filterModel); - - return filterModel; + return filters.getStorageBackedModel(); } @Provides diff --git a/src/name/mlopatkin/andlogview/ui/filters/StoredFilters.java b/src/name/mlopatkin/andlogview/ui/filters/StoredFilters.java index b41815b4..819543a1 100644 --- a/src/name/mlopatkin/andlogview/ui/filters/StoredFilters.java +++ b/src/name/mlopatkin/andlogview/ui/filters/StoredFilters.java @@ -16,6 +16,8 @@ package name.mlopatkin.andlogview.ui.filters; +import static com.google.common.collect.ImmutableSet.toImmutableSet; + import name.mlopatkin.andlogview.config.ConfigStorage; import name.mlopatkin.andlogview.config.Preference; import name.mlopatkin.andlogview.filters.Filter; @@ -23,11 +25,13 @@ import name.mlopatkin.andlogview.search.RequestCompilationException; import name.mlopatkin.andlogview.ui.filterdialog.FilterFromDialog; import name.mlopatkin.andlogview.ui.mainframe.MainFrameScoped; +import name.mlopatkin.andlogview.utils.LazyInstance; import org.apache.log4j.Logger; import org.checkerframework.checker.nullness.qual.Nullable; import java.util.List; +import java.util.Objects; import java.util.stream.Collectors; import javax.inject.Inject; @@ -42,19 +46,24 @@ public class StoredFilters { private static final Logger logger = Logger.getLogger(StoredFilters.class); private final Preference> preference; + private final LazyInstance model = LazyInstance.lazy(this::createModel); @Inject public StoredFilters(ConfigStorage storage) { this.preference = storage.preference(new FilterListSerializer()); } - public void setModel(FilterModel model) { - for (SavedFilterData data : preference.get()) { - var filter = decode(data); - if (filter != null) { - model.addFilter(filter); - } - } + public FilterModel getStorageBackedModel() { + return model.get(); + } + + private FilterModel createModel() { + var model = FilterModel.create( + preference.get().stream() + .map(this::decode) + .filter(Objects::nonNull) + .collect(toImmutableSet()) + ); model.asObservable().addObserver(new FilterModel.Observer() { @Override @@ -72,6 +81,8 @@ public void onFilterReplaced(FilterModel model, Filter oldFilter, Filter newFilt saveFilters(model); } }); + + return model; } private @Nullable Filter decode(SavedFilterData serializedForm) {