Skip to content

Commit

Permalink
Don't crash when second editor for the same filter closes
Browse files Browse the repository at this point in the history
It turns out that the exception was silently swallowed by the Competable
Future so first an exception handler had to be added to actually crash
the application.

The solution to the original problem is similar though, it just ignores
a second edit result but more explicitly.
  • Loading branch information
mlopatkin committed Apr 6, 2020
1 parent b2d67dd commit 9c07061
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.bitbucket.mlopatkin.android.logviewer.ui.indexfilter.IndexFilterCollection;
import org.bitbucket.mlopatkin.android.logviewer.ui.mainframe.MainFrameScoped;
import org.bitbucket.mlopatkin.android.logviewer.ui.mainframe.popupmenu.MenuFilterCreator;
import org.bitbucket.mlopatkin.utils.Threads;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -88,7 +89,9 @@ public void setBufferEnabled(LogRecord.Buffer buffer, boolean enabled) {

@Override
public void createFilterWithDialog() {
dialogFactory.startCreateFilterDialog().thenAccept(newFilter -> newFilter.ifPresent(this::addNewDialogFilter));
dialogFactory.startCreateFilterDialog()
.thenAccept(newFilter -> newFilter.ifPresent(this::addNewDialogFilter))
.exceptionally(Threads::uncaughtException);
}

private DialogPanelFilter createDialogPanelFilter(FilterFromDialog filter) {
Expand Down Expand Up @@ -132,7 +135,9 @@ public void addFilter(FilterFromDialog filter) {

@Override
public void createFilterWithDialog(FilterFromDialog baseData) {
dialogFactory.startEditFilterDialog(baseData).thenAccept(result -> result.ifPresent(this::addFilter));
dialogFactory.startEditFilterDialog(baseData)
.thenAccept(result -> result.ifPresent(this::addFilter))
.exceptionally(Threads::uncaughtException);
}

/**
Expand Down Expand Up @@ -175,7 +180,15 @@ public void delete() {
protected void replaceMeWith(BaseToggleFilter<T> replacement) {
replacement.isEnabled = isEnabled;
int myPos = filters.indexOf(this);
assert myPos >= 0;
if (myPos == -1) {
// Ignore edit result if |this| filter isn't alive anymore. This can happen if the editor was opened
// twice for the same filter. If both edits complete successfully then second one won't find 'this'
// here, because it was replaced with the result of the first edit. Not so much can be done in this
// case. Prior to 0.19 the result of the second edit was added as a new filter, 0.19 just crashes.
// TODO(mlopatkin) proper solution is to disallow opening a second editor but this is much more involved
// fix which I don't like to push within 0.20 timeframe.
return;
}
filters.set(myPos, replacement);
filterPanelModel.replaceFilter(this, replacement);
if (collection.equals(replacement.collection) && mode == replacement.mode) {
Expand Down Expand Up @@ -209,7 +222,7 @@ public void openFilterEditor() {
DialogPanelFilter newPanelFilter = createDialogPanelFilter(newFilter.get());
replaceMeWith(newPanelFilter);
}
});
}).exceptionally(Threads::uncaughtException);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,25 @@ public void testSavingAndInitializngFromSaved() throws Exception {
assertEquals(Color.BLACK, filterImpl.getHighlightColor(RECORD2));
}

@Test
public void editFilterTwiceDoesntCrash() {
MainFilterController controller = new MainFilterController(
filterPanelModel, indexFilterCollection, dialogFactory, new FakeDefaultConfigStorage(), filterImpl);

order = inOrder(dialogFactory, filterPanelModel);
FilterFromDialog initialFilter = createColoringFilter(Color.BLACK, MATCH_ALL);

PanelFilter initialPanel = createFilterWithDialog(controller, initialFilter);

CompletableFuture<Optional<FilterFromDialog>> firstDialog = openFilterDialog(initialPanel);
CompletableFuture<Optional<FilterFromDialog>> secondDialog = openFilterDialog(initialPanel);

firstDialog.complete(Optional.of(createColoringFilter(Color.BLUE, MATCH_ALL)));
secondDialog.complete(Optional.of(createColoringFilter(Color.RED, MATCH_ALL)));

assertEquals(Color.BLUE, filterImpl.getHighlightColor(RECORD1));
}

private PanelFilter createFilterWithDialog(MainFilterController controller, FilterFromDialog dialogResult) {
when(dialogFactory.startCreateFilterDialog()).thenReturn(
CompletableFuture.completedFuture(Optional.of(dialogResult)));
Expand All @@ -178,6 +197,14 @@ private PanelFilter editFilterWithDialog(FilterFromDialog newFilter, PanelFilter
return panelFilterCaptor.getValue();
}

private CompletableFuture<Optional<FilterFromDialog>> openFilterDialog(PanelFilter filter) {
CompletableFuture<Optional<FilterFromDialog>> future = new CompletableFuture<>();
when(dialogFactory.startEditFilterDialog(any())).thenReturn(future);
filter.openFilterEditor();

return future;
}

private FilterFromDialog createMockFilter(FilteringMode mode, final Predicate<LogRecord> predicate) {
FilterFromDialog result = new FilterFromDialog() {
@Override
Expand Down
16 changes: 16 additions & 0 deletions utils/org/bitbucket/mlopatkin/utils/Threads.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@

package org.bitbucket.mlopatkin.utils;

import org.checkerframework.checker.nullness.qual.Nullable;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ThreadFactory;
import java.util.function.Function;

public final class Threads {
private Threads() {}
Expand All @@ -30,4 +34,16 @@ private Threads() {}
public static ThreadFactory withName(final String name) {
return r -> new Thread(r, name);
}

/**
* Helper for {@link CompletableFuture#exceptionally(Function)} that forwards exception to thread's default
* exception handler.
* @param th the throwable to forward
* @return {@code null}
*/
public static @Nullable Void uncaughtException(Throwable th) {
Thread thread = Thread.currentThread();
thread.getUncaughtExceptionHandler().uncaughtException(thread, th);
return null;
}
}

0 comments on commit 9c07061

Please sign in to comment.